Hi,
On Thu, Apr 30, 2020 at 11:55 AM Khushboo Vashi <
khushboo.va...@enterprisedb.com> wrote:
>
>
> On Thu, Apr 30, 2020 at 10:55 AM Aditya Toshniwal <
> aditya.toshni...@enterprisedb.com> wrote:
>
>> Hi,
>>
>> On Thu, Apr 30, 2020 at 10:48 AM Khushboo Vashi <
>> khushboo.va...@enterprisedb.com> wrote:
>>
>>>
>>>
>>> On Thu, Apr 30, 2020 at 10:14 AM Aditya Toshniwal <
>>> aditya.toshni...@enterprisedb.com> wrote:
>>>
>>>> Hi,
>>>>
>>>>
>>>> On Thu, Apr 30, 2020 at 9:41 AM Satish V <satis...@enterprisedb.com>
>>>> wrote:
>>>>
>>>>> Hi Kushboo,
>>>>>
>>>>> Thanks for the update. I will check the same and make
>>>>> appropriate changes.
>>>>>
>>>>> Thanks,
>>>>> Sathish
>>>>>
>>>>> On Thu, Apr 30, 2020 at 9:20 AM Khushboo Vashi <
>>>>> khushboo.va...@enterprisedb.com> wrote:
>>>>>
>>>>>> Hi Satish,
>>>>>>
>>>>>> As per the RM, the fix is supposed to be at the front-end but it
>>>>>> seems difficult at the moment as on the selection of the database, we
>>>>>> connect it and at the same time the context menu is being called.
>>>>>> As you have tried to fix at the backend, some of the review comments
>>>>>> are below.
>>>>>>
>>>>>> 1. If the database is already connected, no need to call
>>>>>> conn.connect again.
>>>>>>
>>>>>>
>>>>>> info_already_connected = conn.connected()
>>>>>>
>>>>>> status, errmsg = conn.connect()
>>>>>>
>>>>>> I've noticed conn.connected() is misleading sometimes. Let's say if
>>>> the PG server is stopped and no query is fired from pgadmin after that,
>>>> then conn.connected() will still give True. It is updated only when a
>>>> query is fired to the PG server. I would suggest let it connect again as
>>>> there is no harm and this function is very important. We don't want to mess
>>>> it up for the sake of a message.
>>>>
>>> It's true that conn.connected() is misleading but we already get the
>>> connection before checking conn.connected() with conn =
>>> manager.connection(did=did, auto_reconnect=True).
>>> So, if the database server is stopped, it will throw the error.
>>>
>> I just checked the manager.connection function, and it is not connecting
>> or checking. It will just give the connection object stored in the memory.
>>
> :).
>
>
> databases/__init__.py : def connect()
>
>
> manager = get_driver(PG_DEFAULT_DRIVER).connection_manager(sid)
>
> conn = manager.connection(did=did, auto_reconnect=True)
>
>
>
> Above 2 lines will handle which I just just mentioned above.
>
I didn't find any code which will call the connect, apart
from auto_reconnect being used when we execute some query. Nevertheless, I
still believe we should not remove the connect call for the sake of a
message, for which I think the user won't even bother.
>
> 2. If you want to raise an error at the client side, use the appropriate
> function or status (check file ajax.py) at the server side and send the
> appropriate response.
>>>>>>
>>>>>> Below code needs to be changed.
>>>>>>
>>>>>> if(res.data.info_already_connected){
>>>>>>
>>>>>> Alertify.error(res.info);
>>>>>>
>>>>>> } else {
>>>>>>
>>>>>> Alertify.success(res.info);
>>>>>>
>>>>>> }
>>>>>>
>>>>>> The problem is test cases. Almost all the test cases have checked for
>>>> response "Database connected.". If the function is changed at the server
>>>> side then all the test cases (around 85+) would have to be changed to
>>>> handle this.
>>>>
>>> It's a valid point, so I would suggest to show the warning/info at the
>>> client side, no need to show the error message.
>>>
>> Yes, info is a good option.
>>
>>> Thanks,
>>>>>>
>>>>>> Khushboo
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Apr 29, 2020 at 8:20 PM Satish V <satis...@enterprisedb.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Hi Hackers,
>>>>>>>
>>>>>>> In the patch attached, we are gracefully informing the end user,
>>>>>>> using an alert message, that the database is already connected when they
>>>>>>> click "Connect Database..." after right clicking on a disconnected
>>>>>>> database.
>>>>>>>
>>>>>>> As this problem deals with racing conditions, it is highly complex
>>>>>>> to show the "Disconnect database" option in the menu upon right click.
>>>>>>> So
>>>>>>> we are alerting the end user with the information of "Database already
>>>>>>> connected".
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sathish V
>>>>>>>
>>>>>>
>>>>
>>>> --
>>>> Thanks and Regards,
>>>> Aditya Toshniwal
>>>> pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune
>>>> "Don't Complain about Heat, Plant a TREE"
>>>>
>>>
>>
>> --
>> Thanks and Regards,
>> Aditya Toshniwal
>> pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune
>> "Don't Complain about Heat, Plant a TREE"
>>
>
--
Thanks and Regards,
Aditya Toshniwal
pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune
"Don't Complain about Heat, Plant a TREE"
