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 1esrmn-0006VM-Dk for pgadmin-hackers@arkaria.postgresql.org; Mon, 05 Mar 2018 15:12:21 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1esrml-00012b-Va for pgadmin-hackers@arkaria.postgresql.org; Mon, 05 Mar 2018 15:12:19 +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 1esrml-00012R-P1 for pgadmin-hackers@lists.postgresql.org; Mon, 05 Mar 2018 15:12:19 +0000 Received: from mail-it0-x22f.google.com ([2607:f8b0:4001:c0b::22f]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1esrmg-0003tR-Pj for pgadmin-hackers@postgresql.org; Mon, 05 Mar 2018 15:12:19 +0000 Received: by mail-it0-x22f.google.com with SMTP id d13so9790349itf.0 for ; Mon, 05 Mar 2018 07:12:13 -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=oZMh8TshckwzmHktrzDFGDvatbC4pfsOaqgOCayC0JQ=; b=arnLPm6VnZ049qkWHYZvSJ2IsWx1VRAP5ayZfiYyztqaVA2ws8rTdUyJVcInCku4tl bK46+uGvJe9BgTbx92zhbXwbX9+3S2OLBmiHOyxxh4M76Kc2N1CvwE8dlK/XKbFUy0rf o/s2RHydXtnm+8Mlrkq/SAEyplBR9pFtpt9JT4vlmeSE6iQNgrq3wkWSHJk9V8g6npLh uXFZwK9ygJQ+643FyA/AQ76F3bCJcgKCHZZOZP2HUkwq4liEfQ+NzkmcBzCkhQeCR7N5 OmYeuEWocGDnhqya8NWJ4w8kkDaoL1S4W831YyabALfrTFYs51SADHDF1oQqQ9IITB+1 bx9w== 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=oZMh8TshckwzmHktrzDFGDvatbC4pfsOaqgOCayC0JQ=; b=qi1f73YrxkXwFnd4cjgYSELSVVmkkMDz726qEBK9l6EjM5srSZhiq3f/Bs/uZHZsjN OrSRT9vOIUYYnXsLGLVSdH6I5Too2pk5+z0LbMiiJidVwnqrDr53mZjFG2mb9TmJbhh4 oQfTffytvXLuTc3K7eIizeBwCXYIQDeIaWNXtwqkCPx+AqTSTrMaAK9Y7tFQy+siwEeE B9m3/PUlVcz+SnQpixHmodbnNT6h80tbroujbmnno5d5htPwYi+ItVJqepNGqr74mYbr yy3K5Q3T/iapxEY19sjZps3k5uORjtNI5OpazuXdvZRGwVog4WNlRlQVN3m3/pm46JYY Pcjw== X-Gm-Message-State: AElRT7GtqlfaHxq94St3EqsqTr4a4e6DMjDIYtLW70+Vsk1fW74M2BXq Gm1GPIv0JGKdBd/DYcDt5QnCt7t0ahHUEztp97CBBg== X-Google-Smtp-Source: AG47ELsDQQYICV8FMiR2fdCsS3r5SzW9mjgwlK5Qyd71oUR9gNH+iB55rD5vfz2KRp059Qs2NlrqrYmQ4B3rKy5EjGk= X-Received: by 10.36.145.139 with SMTP id i133mr14906050ite.69.1520262731858; Mon, 05 Mar 2018 07:12:11 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Mon, 05 Mar 2018 15:12:01 +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="94eb2c0ef36a1923950566abc057" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --94eb2c0ef36a1923950566abc057 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 The tests run successfully in our CI pipeline. Thanks Joao 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 whi= le >>>> 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 so= me >>>> 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 i= d >>> (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 s= ee, >>>> but while 1 Feature Test runs we can run 200 Unit Tests that give us m= uch >>>> 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 si= nce >>>> 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 ti= me 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 outpu= t. >>>>>> >>>>> 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=3Dcommitdiff= ;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 >> > --94eb2c0ef36a1923950566abc057 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Khushboo,
Looks like we are almost doen, just mi= ssing one PEP-8 issue:
=E2=86=92 pycodestyle --config=3D.pyc= odestyle pgadmin/tools
pgadmin/tools/sqleditor/tests/test_poll_qu= ery_tool.py:46: [E126] continuation line over-indented for hanging indent
1=C2=A0 =C2=A0 =C2=A0 =C2=A0E126 continuation line over-indented f= or hanging indent
1


The tests run successfully in our CI pipeline.

Th= anks
Joao

On Sun, Mar 4, 2018 at 11:51 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrot= e:
On Fri, Mar 2, 2018 at 6:55 PM, Dave= Page <dpage@pgadmin.org> wrote:
Could you rebase this please? It no longer applies.=

Please find the attach= ed updated patch.=C2=A0
=
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 Almei= da Pereira <jdealmeidapereira@pivotal.io> wrote:<= br>
Hello Khushboo,
Afte= r reviewing the patch I have the gut feeling that we do not have enough tes= t coverage on this issue, specially due to the intricate while loop and con= ditions around the polling.
I think that this deserve Unit tests = around it, When I say Unit Test I am not talking about executing queries ag= ainst the database, but do some stubbing of the database so that we can con= trol the flow that we want.
= You are right. It needs more unit testing. I have checked below scenarios:<= /div>
1. Returns 2 notices with data output
2. Returns 1000 n= otices with data output
3. No notices with data output=C2=A0

By running above, I have checked, each time returned n= otices are accurate, no old notices are getting appended, it does not affec= t with the amount of messages (few, none or more).=C2=A0 Also, with the upd= ated patch, I have made sure that all these queries run with the single tra= nsaction id (same connection).

So, please let me k= now if you think I can add more things to this.
=C2=A0=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
It is a temp= tation to try to always do a Feature Test to test what we want because it i= s "easier" to write and ultimately it is what users see, but whil= e 1 Feature Test runs we can run 200 Unit Tests that give us much more conf= idence 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 tests pass. Running pycodestyle fails due to some line= sizes on the psycopg2/__init__py. I believe that it is not what you change= d, but since you were changing the file it can be fixed it is just:

pgadmin/utils/driver/psycopg2/__init__.py:1276: [E50= 1] line too long (81 > 79 characters)
pgadmin/utils/driver/psy= copg2/__init__.py:1277: [E501] line too long (91 > 79 characters)
<= div>pgadmin/utils/driver/psycopg2/__init__.py:1282: [E501] line too long (8= 1 > 79 characters)
pgadmin/utils/driver/psycopg2/__init__.py:1= 283: [E501] line too long (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


On Wed, Feb 28, 2018 at 6:49 AM Khushboo Vashi <khushboo.vashi@ente= rprisedb.com> wrote:
On Mon, Feb= 26, 2018 at 10:02 PM, Dave Page <dpage@pgadmin.org> wrote:<= br>
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 pa= tch so it doesn't drop messages.
=
Fixed.
By default, the notice attribute of the connection ob= ject of psycopg 2 only stores 50 notices. Once it reaches to 50 it starts f= rom 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 l= imit 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 Zabuawala <murtuza.zabuawala= @enterprisedb.com> wrote:
Sent bit early,=C2=A0

You can run 'VACUUM FULL VERBOSE' in q= uery tool and verify the populated messages (pgAdmin3 vs. pgAdmin4).=C2=A0<= /div>


On Mon, Fe= b 26, 2018 at 9:48 PM, Murtuza Zabuawala <murtuza.zabuawa= la@enterprisedb.com> wrote:
Hi Khushboo/Dave,

With given commit, I'm again= seeing the issue raised in https://redmine.postgresql.org/issues/1523 :(=




<= div>
--
Regards,
Murtuza Zabuawala
EnterpriseD= B:=C2=A0http://www.enterprisedb.com
The Enterprise PostgreSQL Co= mpany


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
Twitter: @pgs= nake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Co= mpany




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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Co= mpany
--94eb2c0ef36a1923950566abc057--