Re: pgAdmin 4 commit: Ensure we pick up the messages from the current query

Thanks - patch applied.

On Tue, Mar 6, 2018 at 2:57 PM, Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)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(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> On Mon, Mar 5, 2018 at 8:42 PM, Joao De Almeida Pereira <
>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>
>>> Hello Khushboo,
>>>
>> Looks like we are almost doen, just missing one PEP-8 issue:
>>> → pycodestyle --config=.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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>> On Fri, Mar 2, 2018 at 6:55 PM, Dave Page <dpage(at)pgadmin(dot)org> 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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi Joao,
>>>>>>
>>>>>> Thanks for reviewing.
>>>>>>
>>>>>> On Wed, Feb 28, 2018 at 8:55 PM, Joao De Almeida Pereira <
>>>>>> jdealmeidapereira(at)pivotal(dot)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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> On Mon, Feb 26, 2018 at 10:02 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>> 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(dot)zabuawala(at)enterprisedb(dot)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(dot)zabuawala(at)enterprisedb(dot)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 <dpage(at)pgadmin(dot)org>
>>>>>>>>>>> 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 <khushboo(dot)vashi(at)enterprisedb(dot)com>
>>>>>>>>>>>>
>>>>>>>>>>>> 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
>>>>>
>>>>

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company