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