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 1er3cD-00044z-Oy for pgadmin-hackers@arkaria.postgresql.org; Wed, 28 Feb 2018 15:25:57 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1er3cB-0002XW-WF for pgadmin-hackers@arkaria.postgresql.org; Wed, 28 Feb 2018 15:25:56 +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 1er3cB-0002XL-Ps for pgadmin-hackers@lists.postgresql.org; Wed, 28 Feb 2018 15:25:55 +0000 Received: from mail-it0-x244.google.com ([2607:f8b0:4001:c0b::244]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1er3c6-0002x8-2b for pgadmin-hackers@postgresql.org; Wed, 28 Feb 2018 15:25:54 +0000 Received: by mail-it0-x244.google.com with SMTP id u5so3979056itc.1 for ; Wed, 28 Feb 2018 07:25:48 -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=tS2PpIM0TtXoUw9Qz2Vg38rXczcV9V4J6XKYtiFqS90=; b=EeeGqOZZbo1vzVZY6e0OMsxrnYa8nGwSJ18BoknJiYOXVy7oR3fGjyMOJeYIZu1aBY mccwKWTCNIjINhMzUNR4q1jiCRsdFJGToz8oSzo1eH8xxSMHnowYO4DVJ9j9Bjvm+NPX fDdYGy8YoS8zOQPLXaFYVia82cooU4gZhn2vzcqeI3bJDb+h+W4tY98ELQ6VI0v6dwMI c8FbjadQF+cGw0ciZI7v9U7PZzvujDPSAmIwa1g6kp3wOGX0cBvV4KxkKMoca4G8iQbn ZFAwmafSE1kJf+yIz4KLgaOY/E9VgH4Dq2GVzZikG+eLMlfIG+ZbNY1I0BLb2D8UQmW9 UZJA== 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=tS2PpIM0TtXoUw9Qz2Vg38rXczcV9V4J6XKYtiFqS90=; b=PtfXMGxB7ZYLVOWaPAgAyhBE9GtYNoa17Bt7pUYWWzjegATwMw37iI2Psyexgsu3L3 CZRhJss/Hnx0PbyYKeX5OjHN6tBdQDwjWdY1wFrPneRtARCiTpRTbiMLAiFI/cMbI5TZ 2wSY5KCmSPj0DNCSUVfgUJU4MfVVe0BKVXzX2+Kt73799pkj4zyy2gspw5UmyKterE37 m9QiT6ls3H+BBnJkctziiMKOHyzhhweqIZIa6BFbzdpr/BK4YyuXLqiiyGS4JLIVbKwn vBOfM1aC2+Ct9c2yrQJOTPmHE0cCMpVzVbNJ3UNb3cJDUAGgrEcbeKgMb15r8Xykqn2M YVow== X-Gm-Message-State: APf1xPBx61x2dE8ttVVx6RpI9QRuLiKF+pDd5no6o/U68buI9DWOjnrz Z2q0PmB7svcxDPvSt/7HE58o2VHNhQNhERqUAsKjvg== X-Google-Smtp-Source: AG47ELvQbTyE80qwlpEPcFIl5nSz8CmdlGQ5pSnU30eKJsUT1zWQPM9vWW+xHK69mKw3v2NITuEZlv9hfxO7QLDeQWc= X-Received: by 10.36.145.139 with SMTP id i133mr22804391ite.69.1519831547462; Wed, 28 Feb 2018 07:25:47 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Wed, 28 Feb 2018 15:25:36 +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="94eb2c0ef36a81672d0566475bb6" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --94eb2c0ef36a81672d0566475bb6 Content-Type: text/plain; charset="UTF-8" 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. 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. 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) 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=commitdiff;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 >> > --94eb2c0ef36a81672d0566475bb6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Khushboo,
After reviewing the patch I have the g= ut feeling that we do not have enough test coverage on this issue, speciall= y due to the intricate while loop and conditions around the polling.
<= div>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 st= ubbing of the database so that we can control the flow that we want.
<= div>
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 Te= sts that give us much more confidence that the code is doing what we expect= it to do.

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)

Thanks
Joao

=
On Wed, Feb 28, 2018 at 6:4= 9 AM Khushboo Vashi <= khushboo.vashi@enterprisedb.com> wrote:
=
=
=C2=A0
Thanks.

On M= on, Feb 26, 2018 at 4:23 PM, Murtuza Zabuawala <murtuza.z= abuawala@enterprisedb.com> wrote:
Sent bit early,=C2=A0

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

On Mon, Feb 2= 6, 2018 at 9:48 PM, Murtuza Zabuawala <murtuza.zabuawala@= enterprisedb.com> wrote:

=
--
Regards,
Murtuza Zabuawala
EnterpriseDB:=C2=A0h= ttp://www.enterprisedb.com
The Enterprise PostgreSQL Company<= /font>


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: @pgsnake

Enterpri= seDB UK: http://w= ww.enterprisedb.com
The Enterprise PostgreSQL Company
--94eb2c0ef36a81672d0566475bb6--