Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1erkgm-0008SH-EB for pgadmin-hackers@arkaria.postgresql.org; Fri, 02 Mar 2018 13:25:32 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1erkgl-00044J-2w for pgadmin-hackers@arkaria.postgresql.org; Fri, 02 Mar 2018 13:25:31 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1erkgk-000449-Oe for pgadmin-hackers@lists.postgresql.org; Fri, 02 Mar 2018 13:25:30 +0000 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1erkgg-0001dj-V8 for pgadmin-hackers@postgresql.org; Fri, 02 Mar 2018 13:25:30 +0000 Received: by mail-wm0-x231.google.com with SMTP id z9so3023422wmb.3 for ; Fri, 02 Mar 2018 05:25:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=YQuUuq3c1j2Yh3FH74PAsp5v/1lESGNg7q+zp0uaZXc=; b=jz8AYPJjImAQAhustGzBxSf5DH0UQ0g85LRri9mkYo8PSQ86HLQ8tVaPHzvFUt6s1Q 5kiTjG+MmoyCVqDmoQL6NQ7VidnSmD+LR44rWFximccyNq5EAEvHq0N9S77QglBa0nLd HBByrOeZpqUyGJXNz/jX/LxQ2/DQgs3FBvO42Q1Xiq0sJWVqPgi/mv/saD8Zg2chWgHh YIaxy1K5ZYeGqiqHO+kOFkI+ewp9psRebYr+yNbvsul62BC9Za7oiqZsv3PGQrUBqEsd umAjTe+iTbpwPdjEOuJNV+npF56uy3e53AHsY9aBcrHWKNDE73Oqy716+0gAsnWYlZUC 9g4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=YQuUuq3c1j2Yh3FH74PAsp5v/1lESGNg7q+zp0uaZXc=; b=AGo4p8qIK5T/Qlv6Zq9nA1nO/2bAjsDC+cPPpitlfRexsxuPoSyitbgy/oIh6svlWP R2ffBwrBbf/v9QivlEdzI0cXG32AWF0yx2C78vocX280MzKQjqS6U2gLa3MkqKImo/Q9 E36I8/vRJyuo22u6vL7iDG7iOBByIY9ie4Lj5ZrornSigNdgRW5aj6H2iiaiHAyRDfGV MybKMJ/rxRZ9bYY6V/d/kKaEx82kD7FTJEmzf3VmQFmI8sp98f6u60RIMvTY1fpldX1y 1cjgSFCQaFiBV7Vn1p/bQiDCJ9TKqbtAbUj6EUZ+cw+8x3GdUTTtVyv6Jkc2qFlaELQu lPUA== X-Gm-Message-State: AElRT7EO4V/s9b9oqnLgxWEauuBHcJJX/w2SMA7KIAdL3rv6fHV05rZ+ kGhA5anPMs/kfWsnOX1igANFLD+NIaJQovvqRtYJnA== X-Google-Smtp-Source: AG47ELtGBBa0kBdWudLRahrzERm7QzY+j/3N8LEHD4kt1LGMVa00J/W2ShZFph1btZXTnSihwRMF0NMla1sY58iGRCk= X-Received: by 10.28.130.9 with SMTP id e9mr1446567wmd.161.1519997126038; Fri, 02 Mar 2018 05:25:26 -0800 (PST) MIME-Version: 1.0 Received: by 10.28.109.7 with HTTP; Fri, 2 Mar 2018 05:25:25 -0800 (PST) In-Reply-To: References: From: Dave Page Date: Fri, 2 Mar 2018 13:25:25 +0000 Message-ID: Subject: Re: pgAdmin 4 commit: Ensure we pick up the messages from the current query To: Khushboo Vashi Cc: Joao De Almeida Pereira , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="001a11444b44c1ed7805666de847" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --001a11444b44c1ed7805666de847 Content-Type: text/plain; charset="UTF-8" Could you rebase this please? It no longer applies. Thanks. On Thu, Mar 1, 2018 at 5:56 AM, Khushboo Vashi < khushboo.vashi@enterprisedb.com> wrote: > Hi Joao, > > Thanks for reviewing. > > On Wed, Feb 28, 2018 at 8:55 PM, Joao De Almeida Pereira < > jdealmeidapereira@pivotal.io> wrote: > >> Hello Khushboo, >> After reviewing the patch I have the gut feeling that we do not have >> enough test coverage on this issue, specially due to the intricate while >> loop and conditions around the polling. >> I think that this deserve Unit tests around it, When I say Unit Test I am >> not talking about executing queries against the database, but do some >> stubbing of the database so that we can control the flow that we want. >> > You are right. It needs more unit testing. I have checked below scenarios: > 1. Returns 2 notices with data output > 2. Returns 1000 notices with data output > 3. No notices with data output > > By running above, I have checked, each time returned notices are accurate, > no old notices are getting appended, it does not affect with the amount of > messages (few, none or more). Also, with the updated patch, I have made > sure that all these queries run with the single transaction id (same > connection). > > So, please let me know if you think I can add more things to this. > >> >> > It is a temptation to try to always do a Feature Test to test what we want >> because it is "easier" to write and ultimately it is what users see, but >> while 1 Feature Test runs we can run 200 Unit Tests that give us much more >> confidence that the code is doing what we expect it to do. >> >> Right, so added regression tests instead of feature tests. > > This being said, I run the tests on the CI Pipeline and all tests pass. >> Running pycodestyle fails due to some line sizes on the >> psycopg2/__init__py. I believe that it is not what you changed, but since >> you were changing the file it can be fixed it is just: >> >> pgadmin/utils/driver/psycopg2/__init__.py:1276: [E501] line too long (81 >> > 79 characters) >> pgadmin/utils/driver/psycopg2/__init__.py:1277: [E501] line too long (91 >> > 79 characters) >> pgadmin/utils/driver/psycopg2/__init__.py:1282: [E501] line too long (81 >> > 79 characters) >> pgadmin/utils/driver/psycopg2/__init__.py:1283: [E501] line too long (91 >> > 79 characters) >> 4 E501 line too long (81 > 79 characters) >> >> Fixed. Thanks for pointing out. > >> >> Thanks >> Joao >> >> >> On Wed, Feb 28, 2018 at 6:49 AM Khushboo Vashi < >> khushboo.vashi@enterprisedb.com> wrote: >> >>> On Mon, Feb 26, 2018 at 10:02 PM, Dave Page wrote: >>> >>>> Argh, I ran some tests, but didn't spot any lost messages in the tests >>>> I ran. I'll revert the patch. >>>> >>>> Khushboo; >>>> >>>> Please look at the following: >>>> >>>> - Fix the patch so it doesn't drop messages. >>>> >>> Fixed. >>> By default, the notice attribute of the connection object of psycopg 2 >>> only stores 50 notices. Once it reaches to 50 it starts from 1 again. >>> To fix this I have changed the notice attribute from list to deque to >>> append more messages. Currently I have kept the maximum limit at a time of >>> the notice attribute is 100000 (in a single poll). >>> >>>> - Add regression tests to make sure it doesn't break in the future. >>>> This may require creating one or more functions the spew out a whole lot of >>>> notices, and then running a couple of queries and checking the output. >>>> >>> Added. With this regression test, the current code is failing which has >>> been taken care in this patch. >>> >>>> - Check the messages panel on the history tab. I just noticed it seems >>>> to only be showing an even smaller subset of the messages. >>>> >>> Tested and no issues found. >>> >>>> >>>> >>> Thanks. >>>> >>>> On Mon, Feb 26, 2018 at 4:23 PM, Murtuza Zabuawala < >>>> murtuza.zabuawala@enterprisedb.com> wrote: >>>> >>>>> Sent bit early, >>>>> >>>>> You can run 'VACUUM FULL VERBOSE' in query tool and verify the >>>>> populated messages (pgAdmin3 vs. pgAdmin4). >>>>> >>>>> >>>>> On Mon, Feb 26, 2018 at 9:48 PM, Murtuza Zabuawala < >>>>> murtuza.zabuawala@enterprisedb.com> wrote: >>>>> >>>>>> Hi Khushboo/Dave, >>>>>> >>>>>> With given commit, I'm again seeing the issue raised in >>>>>> https://redmine.postgresql.org/issues/1523 :( >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Regards, >>>>>> Murtuza Zabuawala >>>>>> EnterpriseDB: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>>> >>>>>> On Mon, Feb 26, 2018 at 7:49 PM, Dave Page wrote: >>>>>> >>>>>>> Ensure we pick up the messages from the current query and not a >>>>>>> previous one. Fixes #3094 >>>>>>> >>>>>>> Branch >>>>>>> ------ >>>>>>> master >>>>>>> >>>>>>> Details >>>>>>> ------- >>>>>>> https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdif >>>>>>> f;h=08b3ccc01a4d57e8ea3657f8882a53dcd1b99386 >>>>>>> Author: Khushboo Vashi >>>>>>> >>>>>>> Modified Files >>>>>>> -------------- >>>>>>> web/pgadmin/utils/driver/abstract.py | 1 + >>>>>>> web/pgadmin/utils/driver/psycopg2/__init__.py | 64 >>>>>>> +++++++++------------------ >>>>>>> 2 files changed, 21 insertions(+), 44 deletions(-) >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --001a11444b44c1ed7805666de847 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Could you rebase this please? It no longer applies.
Thanks.

On Thu, Mar 1, 2018 at 5:56 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi Joao,

Thanks for= reviewing.

On Wed, Feb 28, 2018 at 8:55 PM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hello Khushboo,
After reviewing = the patch I have the gut feeling that we do not have enough test coverage o= n this issue, specially due to the intricate while loop and conditions arou= nd the polling.
I think that this deserve Unit tests around it, W= hen I say Unit Test I am not talking about executing queries against the da= tabase, but do some stubbing of the database so that we can control the flo= w that we want.
You are righ= t. It needs more unit testing. I have checked below scenarios:
1.= Returns 2 notices with data output
2. Returns 1000 notices with = data output
3. No notices with data output=C2=A0

By running above, I have checked, each time returned notices are a= ccurate, no old notices are getting appended, it does not affect with the a= mount of messages (few, none or more).=C2=A0 Also, with the updated patch, = I have made sure that all these queries run with the single transaction id = (same connection).

So, please let me know if you t= hink I can add more things to this.
=C2=A0=C2=A0
It is a tempt= ation to try to always do a Feature Test to test what we want because it is= "easier" to write and ultimately it is what users see, but while= 1 Feature Test runs we can run 200 Unit Tests that give us much more confi= dence that the code is doing what we expect it to do.

<= /div>
Right, so added regression tests instead of f= eature tests.=C2=A0

This being said, I run the t= ests on the CI Pipeline and all tests pass. Running pycodestyle fails due t= o some line sizes on the psycopg2/__init__py. I believe that it is not what= you changed, but since you were changing the file it can be fixed it is ju= st:

pgadmin/utils/driver/psycopg2/__init= __.py:1276: [E501] line too long (81 > 79 characters)
pgadmin/= utils/driver/psycopg2/__init__.py:1277: [E501] line too long (91 > = 79 characters)
pgadmin/utils/driver/psycopg2/__init__.py:128= 2: [E501] line too long (81 > 79 characters)
pgadmin/utils/dri= ver/psycopg2/__init__.py:1283: [E501] line too long (91 > 79 charac= ters)
4=C2=A0 =C2=A0 =C2=A0 =C2=A0E501 line too long (81 > 79 = characters)

Fixed.= Thanks for pointing out.=C2=A0

Thanks
J= oao


On Wed, Feb 28, 2018 at 6:49 AM Khushboo Vashi <= khushb= oo.vashi@enterprisedb.com> wrote:
On Mon, Feb 26, 2018 at 10:02 PM, Dave Page <dpage@pgadmin.org&= gt; wrote:
Argh, I ran some tests, but didn't spot any lost messages = in the tests I ran. I'll revert the patch.

Khushboo;=

Please look at the following:

- Fix the patch so it doesn't drop messages.
Fixed.
By default, the notice attribute of= the connection object of psycopg 2 only stores 50 notices. Once it reaches= to 50 it starts from 1 again.
To fix this I have changed the not= ice attribute from list to deque to append more messages. Currently I have = kept the maximum limit at a time of the notice attribute is=C2=A0100000 (in= a single poll).=C2=A0
- Add regression tests to make sur= e it doesn't break in the future. This may require creating one or more= functions the spew out a whole lot of notices, and then running a couple o= f queries and checking the output.
Added. With this regression test, the current code is failing which has = been taken care in this patch.
- Check the messages panel o= n the history tab. I just noticed it seems to only be showing an even small= er subset of the messages.
<= /div>
Tested and no issues found.
<= div class=3D"gmail_extra">
=C2=A0
Thanks.

On Mon, Feb 26, 2018 at 4:23 PM, Murt= uza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:
Sent bit early,=C2=A0

You can run 'VACUUM FULL VERBOSE' in query tool and verify t= he populated messages (pgAdmin3 vs. pgAdmin4).=C2=A0


On Mon, Feb 26, 2018 at 9:48 PM, Murtuza Zabuawala <= ;mu= rtuza.zabuawala@enterprisedb.com> wrote:
Hi Khushboo/Dave,

With given com= mit, I'm again seeing the issue raised in https://redmine.postgresql.org<= wbr>/issues/1523 :(




--
Regards,
Murtuza Zabuawala
EnterpriseDB:=C2=A0http://www= .enterprisedb.com
The Enterprise PostgreSQL Company

<= /div>

On Mon, Feb 26, 2018 at 7:49 PM, Dave Page <= span dir=3D"ltr"><dpage@pgadmin.org> wrote:
Ensure we pick up the messages from the current query an= d not a previous one. Fixes #3094

Branch
------
master

Details
-------
https://git.postgresql.org/gitweb?p=3Dpgadmin4.git;a=3Dcom= mitdiff;h=3D08b3ccc01a4d57e8ea3657f8882a53dcd1b99386
Author: Khushboo Vashi <khushboo.vashi@enterprisedb.com>

Modified Files
--------------
web/pgadmin/utils/driver/abstract.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= |=C2=A0 1 +
web/pgadmin/utils/driver/psycopg2/__init__.py | 64 +++++++++----------= --------
2 files changed, 21 insertions(+), 44 deletions(-)






--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
<= br>EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
=




--
Dave Page
Blog: = http://pgsnake.bl= ogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com<= br>The Enterprise PostgreSQL Company
--001a11444b44c1ed7805666de847--