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 1etE2Z-0004DG-6B for pgadmin-hackers@arkaria.postgresql.org; Tue, 06 Mar 2018 14:58:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1etE2X-0008To-PW for pgadmin-hackers@arkaria.postgresql.org; Tue, 06 Mar 2018 14:58:05 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1etE2X-0008Tc-CI for pgadmin-hackers@lists.postgresql.org; Tue, 06 Mar 2018 14:58:05 +0000 Received: from mail-io0-x236.google.com ([2607:f8b0:4001:c06::236]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1etE2T-0001OW-77 for pgadmin-hackers@postgresql.org; Tue, 06 Mar 2018 14:58:03 +0000 Received: by mail-io0-x236.google.com with SMTP id g21so22324923ioj.5 for ; Tue, 06 Mar 2018 06:58:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pivotal-io.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nZWoZt/+ptVZqP+5bgH1RIZRVZEku0GIxgSWj0rZb04=; b=qpGHOU4fbm0XZ3WO1lOpq8lxYmKiBtRf7cyZe0cVjZC6lQl3up6ytYHc39dkGcutdv XBJmfru3wJGKY7chwZDjiPkXTF2f1BdulRnO8ARQYbvP1NjK6Gc+g6+kfvsPLXm1XyEe lp/WUoRpnkIKRRxdhEuBcpqH23qfBG+rjxYeGVzRCzV8pF9HXyXr1maVE6Rfo66erbxT XG/UO9KcURDhxtUb4sWxH9TIGB4G9pf5WwT1q7nSxxXxbPvs53TdpBem/coM7m8e1l29 q+OAVxxHIXw/GVBeSIHMXI3poIglJmeC0koi0fIf2WjAhJ16ndwOtJB/G0hkD0alnid1 5WDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=nZWoZt/+ptVZqP+5bgH1RIZRVZEku0GIxgSWj0rZb04=; b=ugMbG+KqXjyAQYQoVqD2bLvakT64BCCsZp3po5MUV9RA9A07Y4yVaAVrTqEbCEWUt7 5P4YNjRa4xQfha2YqPvZ7jVXlbIsk9DHX1CPk/zE6M4BqVrHEWOU2x1n9tILBhApIzSt LxfP8sFxt5wVJP0xNDxZN7BxMCfo2LI5mF9nKp0ytOSd2Y4whHzzgfV+YTIcop8G+mER sipU41Xb0jEmqULQ1kU6/M5/tRmzh1eTjfF5Vx58Acj38pwo7SyaWWyHin2379t1G2uG /bC1dpGvMaEwUM5JDjVKv8yuZFl2DvoNsXLkGjDs1SpX/zDV3HRKWg1MW3gVDEkSEaog bVqg== X-Gm-Message-State: AElRT7FODb/yE3euvy1SzhKAMI4u974bfbXTAC+EPtDJqvr0SCKyi0Un l2g1zjQm7HxJRwhMYc/LXJ00NGhfHYF38Ku9bPpEuA== X-Google-Smtp-Source: AG47ELuKkLWBln/TlI18CQxEsg1FlqH3nFeCOv+qSRD0LTludgp4SKrrt2hzfLL7E9IyFspSk7p5pcc3fd0kF5XBPRY= X-Received: by 10.107.39.5 with SMTP id n5mr21496547ion.189.1520348280144; Tue, 06 Mar 2018 06:58:00 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Tue, 06 Mar 2018 14:57:49 +0000 Message-ID: Subject: Re: pgAdmin 4 commit: Ensure we pick up the messages from the current query To: Khushboo Vashi Cc: Dave Page , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="001a11406f182c64730566bfaba8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --001a11406f182c64730566bfaba8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hello Khushboo, Thanks for the changes here. Now everything looks good, and the tests all pass. Thanks Joao On Tue, Mar 6, 2018 at 5:09 AM Khushboo Vashi < khushboo.vashi@enterprisedb.com> wrote: > On Mon, Mar 5, 2018 at 8:42 PM, Joao De Almeida Pereira < > jdealmeidapereira@pivotal.io> wrote: > >> Hello Khushboo, >> > Looks like we are almost doen, just missing one PEP-8 issue: >> =E2=86=92 pycodestyle --config=3D.pycodestyle pgadmin/tools >> pgadmin/tools/sqleditor/tests/test_poll_query_tool.py:46: [E126] >> continuation line over-indented for hanging indent >> 1 E126 continuation line over-indented for hanging indent >> 1 >> >> >> Thanks Joao. > Please find the attached updated patch. > >> The tests run successfully in our CI pipeline. >> >> Thanks >> Joao >> >> Thanks, > Khushboo > >> On Sun, Mar 4, 2018 at 11:51 PM Khushboo Vashi < >> khushboo.vashi@enterprisedb.com> wrote: >> >>> On Fri, Mar 2, 2018 at 6:55 PM, Dave Page wrote: >>> >>>> Could you rebase this please? It no longer applies. >>>> >>>> Please find the attached updated patch. >>> >>>> 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 w= hile >>>>>> 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 d= o some >>>>>> stubbing of the database so that we can control the flow that we wan= t. >>>>>> >>>>> 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 wit= h 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 psycop= g >>>>>>> 2 only stores 50 notices. Once it reaches to 50 it starts from 1 ag= ain. >>>>>>> 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 who= le lot of >>>>>>>> notices, and then running a couple of queries and checking the out= put. >>>>>>>> >>>>>>> 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=3Dpgadmin4.git;a=3Dcommitdi= ff;h=3D08b3ccc01a4d57e8ea3657f8882a53dcd1b99386 >>>>>>>>>>> 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 >>>> >>> --001a11406f182c64730566bfaba8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Khushboo,
Thanks for the changes here. Now every= thing looks good, and the tests all pass.

Thanks
Joao

On= Tue, Mar 6, 2018 at 5:09 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
<= /div>
On Mon, Mar 5, 2018 at 8:42 PM, Joao De Alm= eida Pereira <jdealmeidapereira@pivotal.io> wrote= :
Hello Kh= ushboo,
Looks like we are almost doen, just missing one PEP-8 iss= ue:
=E2=86=92 pycodestyle --config=3D.pycodestyle pgadmin/to= ols
pgadmin/tools/sqleditor/tests/test_poll_query_tool.py:46: [E1= 26] continuation line over-indented for hanging indent
1=C2=A0 = =C2=A0 =C2=A0 =C2=A0E126 continuation line over-indented for hanging indent=
1


Thanks Joao.=C2=A0
Please find the attache= d updated patch.=C2=A0
=
The tests run successfully in our CI pipel= ine.

Thanks
Joao

Thanks,
Khushboo=C2=A0
On Sun, Mar 4, 2018 at 11:51 PM Khushboo Vashi &l= t;khus= hboo.vashi@enterprisedb.com> wrote:
On Fri, Mar 2, 2018 at 6:55 PM, Dave Page <dpage@pgadmin.org> wrote:
Could you r= ebase this please? It no longer applies.

<= /div>
Please find the attached updated patch.=C2=A0
=
Tha= nks.

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 <j= dealmeidapereira@pivotal.io> wrote:
Hello Khushboo,
After reviewing the patch I h= ave 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 pollin= g.
I think that this deserve Unit tests around it, When I say Uni= t Test I am not talking about executing queries against the database, but d= o some stubbing of the database so that we can control the flow that we wan= t.
You are right. It needs m= ore unit testing. I have checked below scenarios:
1. Returns 2 no= tices with data output
2. Returns 1000 notices with data output
3. No notices with data output=C2=A0

By r= unning above, I have checked, each time returned notices are accurate, no o= ld notices are getting appended, it does not affect with the amount of mess= ages (few, none or more).=C2=A0 Also, with the updated patch, I have made s= ure that all these queries run with the single transaction id (same connect= ion).

So, please let me know if you think I can ad= d more things to this.
=C2=A0=C2=A0
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 w= e 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.=C2=A0

This being said, I run the tests on the CI Pipeline and all te= sts 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 c= hanging the file it can be fixed it is just:

= pgadmin/utils/driver/psycopg2/__init__.py:1276: [E501] line too long (81 &g= t; 79 characters)
pgadmin/utils/driver/psycopg2/__init__.py:1277:= [E501] line too long (91 > 79 characters)
pgadmin/utils/drive= r/psycopg2/__init__.py:1282: [E501] line too long (81 > 79 characters)
pgadmin/utils/driver/psycopg2/__init__.py:1283: [E501] line too lo= ng (91 > 79 characters)
4=C2=A0 =C2=A0 =C2=A0 =C2=A0E501 line = too long (81 > 79 characters)

Fixed. Thanks for pointing out.=C2=A0

Thanks
Joao

<= /span>

On Wed, Feb 28, 2018 at 6:49 AM Khushboo Vashi <khushboo.vashi@ent= erprisedb.com> wrote:
On Mon, Fe= b 26, 2018 at 10:02 PM, Dave Page <dpage@pgadmin.org> wrote:=
Argh= , I ran some tests, but didn't spot any lost messages in the tests I ra= n. I'll revert the patch.

Khushboo;

Please look at the following:

- Fix the p= atch so it doesn't drop messages.
<= /div>
Fixed.
By default, the notice attribute of the connection o= bject 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 fro= m 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
<= div class=3D"gmail_quote">
- Add regression tests to make sure it doesn't b= reak in the future. This may require creating one or more functions the spe= w out a whole lot of notices, and then running a couple of queries and chec= king the output.
Added. With thi= s 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 m= essages.
Tested and n= o issues found.
=C2=A0
Thanks.
=
On Mon, Feb 26, 2018 at 4:23 PM, Murtuza Zab= uawala <murtuza.zabuawala@enterprisedb.com>= wrote:
Sent b= it early,=C2=A0

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


O= n Mon, Feb 26, 2018 at 9:48 PM, Murtuza Zabuawala <murtuz= a.zabuawala@enterprisedb.com> wrote:
Hi Khushboo/Dave,

With given commit, I= 9;m again seeing the issue raised in https://redmine.postgresql.org/issues/15= 23 :(




= --
Regards,
Murtuza Zabuawala
EnterpriseDB:=C2=A0http://www.ent= erprisedb.com
The Enterprise PostgreSQL Company
<= /div>
<= br>

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=3Dcommitdi= ff;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
Tw= itter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Po= stgreSQL Company




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

EnterpriseDB UK: http://www.enterprised= b.com
The Enterprise PostgreSQL Company
--001a11406f182c64730566bfaba8--