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

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=commitdif
>>>>>>>> f;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
>