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

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
>>>>
>>>