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 1etZHM-0001Pr-Nb for pgadmin-hackers@arkaria.postgresql.org; Wed, 07 Mar 2018 13:38:49 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1etZHL-0003Rx-CU for pgadmin-hackers@arkaria.postgresql.org; Wed, 07 Mar 2018 13:38:47 +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 1etZHL-0003Rm-11 for pgadmin-hackers@lists.postgresql.org; Wed, 07 Mar 2018 13:38:47 +0000 Received: from mail-wm0-x242.google.com ([2a00:1450:400c:c09::242]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1etZHG-0008VI-TW for pgadmin-hackers@postgresql.org; Wed, 07 Mar 2018 13:38:46 +0000 Received: by mail-wm0-x242.google.com with SMTP id q83so4848361wme.5 for ; Wed, 07 Mar 2018 05:38:42 -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=VEz5QU4naBBg6s+VEhzzDNxHA1HYBLJH3bqSKcVHP2k=; b=uz04R5EK0ltEAkPV/+wmAC20ss5NvHCHEPGf1kbK8sZXArJi+g2oIYEbtunCzeg6g4 4JaIKD5OGb/6HUGqc0OgYhdiCpildl/3fnweDcDAUE5NcBun3zdrk65OUucFPPeND69c RM/EPEslHnYfrkVuK7vMPhHsxW0YkAf9Mdv0iT4H/N301lCagOM8/mAat8tD8ASvySQ7 ygX7NWR6LFk0EMsx53+lm+tywLeDgejl4DQwuDZhb8+OucaaCaZpuf3vOtcTGh+QU2DX YcCTBk13Jg90JADPeRROg1cc5cJeUxd57RpfE85MGaNBerZHOfLJm2MMd2ZrzdyRcNeD /HLQ== 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=VEz5QU4naBBg6s+VEhzzDNxHA1HYBLJH3bqSKcVHP2k=; b=naXO8nYWcUwoTVFn2MtT1uyKoiciyl2KP3esnqCz+VpaDCEU3vqqZU3m9EUi/usjYg VobsW/8O+1XlIl5thkNKFsdzPgd9nA2q8F0kbzDZCQZWlwSsrmW08yobGgu3YzJjM8lw r+LJcxujvDyt19pHbtjO2MVZ3WIJe1d5umymep4s7xuq+3DnEDvraJ6wpUlwACPVh9xs MvXte5MLIhyyen76kkiPW0bZ+yL7kF3tYz4KAmL1X3BsFgtSjtAQZzJZSvT6HSG8ImOq Z2AsyPWIrD8To/wwekTvedgS1j8mgxYJWLwbG2PGbfNCnYbmPQDqPVBvi+aCG4TvlKOu HE5g== X-Gm-Message-State: AElRT7HAtBEcbqGsDGzsw/xq9lp/F/bwdULQnrbOA5ZHJz5nXdsWwcd8 XNHd3rVBQZ/4QcK+8zKmHoaSCzoL/wx5Q/g7ZorD2g== X-Google-Smtp-Source: AG47ELsO/EdeYy0nBkVnagwJvSpgro+3xsVk3iAbjrcBn8cWGwa+XchU9UhYZnawaTJ3+JIt1NVNBmM3h1T/2r6KI0g= X-Received: by 10.28.5.75 with SMTP id 72mr15279221wmf.154.1520429921629; Wed, 07 Mar 2018 05:38:41 -0800 (PST) MIME-Version: 1.0 Received: by 10.28.109.7 with HTTP; Wed, 7 Mar 2018 05:38:40 -0800 (PST) In-Reply-To: References: From: Dave Page Date: Wed, 7 Mar 2018 13:38:40 +0000 Message-ID: Subject: Re: pgAdmin 4 commit: Ensure we pick up the messages from the current query To: Joao De Almeida Pereira Cc: Khushboo Vashi , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="001a114429c86290a70566d2ad55" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --001a114429c86290a70566d2ad55 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks - patch applied. On Tue, Mar 6, 2018 at 2:57 PM, Joao De Almeida Pereira < jdealmeidapereira@pivotal.io> wrote: > 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 hav= e >>>>>>> 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 Tes= t >>>>>>> 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 wa= nt. >>>>>>> >>>>>> 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 wi= th the >>>>>> amount of messages (few, none or more). Also, with the updated patc= h, I >>>>>> have made sure that all these queries run with the single transactio= n 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 w= e >>>>>>> want because it is "easier" to write and ultimately it is what user= s see, >>>>>>> but while 1 Feature Test runs we can run 200 Unit Tests that give u= s 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 a= t 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 ch= ecking the >>>>>>>>> output. >>>>>>>>> >>>>>>>> Added. With this regression test, the current code is failing whic= h >>>>>>>> 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=3D >>>>>>>>>>>> commitdiff;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 >>>>> >>>> --=20 Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --001a114429c86290a70566d2ad55 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks - patch applied.
On Tue, Mar 6, 2018 at 2:57 PM, Joao De Almeid= a Pereira <jdealmeidapereira@pivotal.io> wrote:
Hello Khushboo,
Thank= s for the changes here. Now everything looks good, and the tests all pass.<= /div>

Thanks
Joao

On Tue, Mar 6, 201= 8 at 5:09 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrot= e:
On Mon, Mar 5, 2018 at 8:42 PM, Joao= De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
<= div class=3D"gmail_quote">
H= ello Khushboo,
Looks like we are almost doen, just missing one PE= P-8 issue:
=E2=86=92 pycodestyle --config=3D.pycodestyle pga= dmin/tools
pgadmin/tools/sqleditor/tests/test_poll_query_too= l.py:46: [E126] continuation line over-indented for hanging indent
1=C2=A0 =C2=A0 =C2=A0 =C2=A0E126 continuation line over-indented for hang= ing indent
1


Thanks Joao.=C2=A0
Please find t= he attached updated patch.=C2=A0
The tests run successfully in our = CI pipeline.

Thanks
Joao=

Thanks,
Khushboo=C2=A0
On Su= n, Mar 4, 2018 at 11:51 PM Khushboo Vashi <khushboo.vashi@enterprisedb.co= m> wrote:
<= div class=3D"gmail_extra">
On Fri, Mar 2, 2018 at= 6:55 PM, Dave Page <dpage@pgadmin.org> wrote:
Could you rebase this please? It no l= onger applies.

Please = find the attached updated patch.=C2=A0
Thanks.

On Thu, Mar 1, 2018 at 5:56 AM, Khush= boo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi Joao,
<= br>
Thanks for reviewing.

On Wed, Feb 28, 2018 at 8:55 PM, Joao De Almeid= a 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 cond= itions around the polling.
I think that this deserve Unit tests a= round it, When I say Unit Test I am not talking about executing queries aga= inst the database, but do some stubbing of the database so that we can cont= rol the flow that we want.
Y= ou are right. It needs more unit testing. I have checked below scenarios:
1. Returns 2 notices with data output
2. Returns 1000 no= tices with data output
3. No notices with data output=C2=A0
=

By running above, I have checked, each time returned no= tices are accurate, no old notices are getting appended, it does not affect= with the amount of messages (few, none or more).=C2=A0 Also, with the upda= ted patch, I have made sure that all these queries run with the single tran= saction id (same connection).

So, please let me kn= ow if you think 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 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 character= s)
pgadmin/utils/driver/psycopg2/__init__.py:1282: [E501] li= ne too long (81 > 79 characters)
pgadmin/utils/driver/psycopg2= /__init__.py:1283: [E501] line too long (91 > 79 characters)
<= div>4=C2=A0 =C2=A0 =C2=A0 =C2=A0E501 line too long (81 > 79 characters)<= /div>

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

Thanks
Joao


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

Khushboo;

Please l= ook at the following:

- Fix the patch so it doesn&= #39;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 t= o append more messages. Currently I have kept the maximum limit at a time o= f the notice attribute is=C2=A0100000 (in a single poll).=C2=A0
=
=
- 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.
=C2=A0
Thanks.
=

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

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


On Mon, Feb 26, 2018 at 9:48 PM, Murtuza Zabua= wala <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:=C2=A0http://www.enterpris= edb.com
The Enterprise PostgreSQL Company
<= div><= img src=3D"https://drive.google.com/a/enterprisedb.com/uc?id=3D0B6jGeB3BfKR= MV0t4MEp0YnZCTTA&export=3Ddownload" width=3D"420" height=3D"31">

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=3Dcommitdiff;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

Enterpri= seDB UK: http://w= ww.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



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

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