Re: RM2815: Relogin to pgAdmin from sqleditor/datadrid if session exprires

Hi Dave,

Please find rebased patch.

Also I have made changes to existing code about showing connection status
in query tool to make this work.

Thanks,

--
*Harshal Dhumal*
*Sr. Software Engineer*

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

On Fri, Jan 12, 2018 at 3:19 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Can you rebase this please?
>
> On Wed, Jan 10, 2018 at 9:47 AM, Harshal Dhumal <
> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>
>> Hi,
>>
>> Please find rebased patch.
>>
>> --
>> *Harshal Dhumal*
>> *Sr. Software Engineer*
>>
>> EnterpriseDB India: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> On Tue, Jan 9, 2018 at 9:21 PM, Harshal Dhumal <
>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Dave,
>>>
>>> Please find updated patch where we are notifying user about connection
>>> reset.
>>>
>>> --
>>> *Harshal Dhumal*
>>> *Sr. Software Engineer*
>>>
>>> EnterpriseDB India: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>> On Mon, Jan 8, 2018 at 7:39 PM, Harshal Dhumal <
>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>
>>>> On Mon, Jan 8, 2018 at 7:26 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Mon, Jan 8, 2018 at 1:18 PM, Harshal Dhumal <
>>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>>
>>>>>> On Mon, Jan 8, 2018 at 6:11 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Jan 8, 2018 at 12:37 PM, Harshal Dhumal <
>>>>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> On Mon, Jan 8, 2018 at 5:15 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> HI
>>>>>>>>>
>>>>>>>>> On Mon, Jan 8, 2018 at 11:41 AM, Harshal Dhumal <
>>>>>>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> On Mon, Jan 8, 2018 at 4:34 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Jan 5, 2018 at 7:50 AM, Harshal Dhumal <
>>>>>>>>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Further details:
>>>>>>>>>>>>
>>>>>>>>>>>> 1. If session is expired and user performs any action from
>>>>>>>>>>>> sqleditor that makes ajax call
>>>>>>>>>>>> then in ajax error call back user can check and handle login
>>>>>>>>>>>> related error using code snippet.
>>>>>>>>>>>>
>>>>>>>>>>>> if (pgAdmin.Browser.UserManagement.is_pga_login_required(xhr)) {
>>>>>>>>>>>> return pgAdmin.Browser.UserManagement.pga_login();
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> Where is xhr is standard xhr or jqxhr object.
>>>>>>>>>>>>
>>>>>>>>>>>> 2. Similarly for connection lost (only maintenance db
>>>>>>>>>>>> connection as we can recover or reconnect other
>>>>>>>>>>>> connections if maintenance db connection is alive). It will
>>>>>>>>>>>> attempt to create/reconnect connection without
>>>>>>>>>>>> asking password (to handle passwordless connection, or saveed
>>>>>>>>>>>> password or password from pgpass file)
>>>>>>>>>>>> If connection to database still fails then it'll prompt for
>>>>>>>>>>>> password.
>>>>>>>>>>>>
>>>>>>>>>>>> Code snippet:
>>>>>>>>>>>> SqlEditorController.handle_connection_lost();
>>>>>>>>>>>> once connection lost is detected one can call
>>>>>>>>>>>> handle_connection_lost() to reconnect.
>>>>>>>>>>>>
>>>>>>>>>>>> 3. We maintain some additional data in session to maintain
>>>>>>>>>>>> affinity between
>>>>>>>>>>>> each sqleditor/datagrid instance to backend database
>>>>>>>>>>>> connection. However if session expires and user
>>>>>>>>>>>> re-loggins then we need to first restore affinity between
>>>>>>>>>>>> sqleditor to backend database before we can start
>>>>>>>>>>>> using query tool.
>>>>>>>>>>>>
>>>>>>>>>>>> Code snippet:
>>>>>>>>>>>>
>>>>>>>>>>>> if(is_new_transaction_required(xhr)) {
>>>>>>>>>>>> SqlEditorController.init_transaction();
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> (note: I haven't looked at the code yet)
>>>>>>>>>>>
>>>>>>>>>>> How does this handle re-establishment of the connection
>>>>>>>>>>> mid-transaction, or, if the user has modified any session variables?
>>>>>>>>>>>
>>>>>>>>>>> ServeManager and Connection Manager are written in a such way
>>>>>>>>>> that if any connection is lost except maintenance db connection
>>>>>>>>>> then we can re-connect or create new connection without prompting
>>>>>>>>>> for database password and if maintenance db connection is lost
>>>>>>>>>> then It prompts for password.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Right.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Regarding session variables as long as flask session is not
>>>>>>>>>> expired we uses same session variables. But in case of user logout (due to
>>>>>>>>>> flask session expire) we create new transaction id and sets new
>>>>>>>>>> session variables for that particular Sql editor /datagrid instance.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I mean DB session variables (and related things). For example, if
>>>>>>>>> the user executed queries such as the following, then they absolutely need
>>>>>>>>> to know if the session got reset:
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ok.
>>>>>>>> Then in this case can we notify user about the same. That we're
>>>>>>>> unable to restore old database connection and created new one and therefore
>>>>>>>> any DB session variables were set/modified (like SET
>>>>>>>> CLIENT_ENCODING..., SET DATESTYLE...) are lost (or similar message).
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> CREATE TEMPORARY TABLE ....
>>>>>>>>> SET ROLE ...
>>>>>>>>> SET [various other options]
>>>>>>>>>
>>>>>>>>> If the user has done any of those things (or similar things that I
>>>>>>>>> haven't thought of), then we cannot just blindly reset the database
>>>>>>>>> connection.
>>>>>>>>>
>>>>>>>>
>>>>>>>> We only create new connection if flask session was expired as we
>>>>>>>> lost transaction id associated with Sql editor/datagrid and therefore
>>>>>>>> unique connection id
>>>>>>>> given to connection which was associated to Sql editor/datagrid. In
>>>>>>>> this case we can notify about same (as stated above).
>>>>>>>>
>>>>>>>
>>>>>>> If it's only the flask session we're resetting, not the database
>>>>>>> connection, we won't need to warn the user will we?
>>>>>>>
>>>>>>
>>>>>> The problem here is that if flask session is reset then we lose the
>>>>>> information about which connection was associate with which query
>>>>>> tool/datagrid.
>>>>>>
>>>>>> lets say If user have opened 3 query tools with same database
>>>>>> therefore we'll have 3 separate connections
>>>>>> (each will have unique connection id). Now this information that
>>>>>> which connection is associate with which query
>>>>>> tool is lost and also unique connection id. So there is no way that
>>>>>> we can get that connection
>>>>>> from connection manager ( *manager.connection(did=<some did>,
>>>>>> conn_id=<unique connection id>)* ).
>>>>>>
>>>>>> So even database connection was not lost and only flask session was
>>>>>> reset we need to create new connection. So I think we'll need to warn user.
>>>>>>
>>>>>
>>>>> Agreed... and ensure the database connection is fully reset.
>>>>>
>>>>>
>>>>>>
>>>>>> Also If we save connection id to client side (in browser in JS) still
>>>>>> we won't be able to get same connection
>>>>>> even though we know connection id in case of flask session reset. As
>>>>>> for each logged in user (pgAdmin user)
>>>>>> but for same database server we create separate ServerManager (and
>>>>>> therefore separate connection pool)
>>>>>> and flask session reset is same as if same user is logged in from
>>>>>> another browser.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> But... what if the database connection has also been lost in the
>>>>>>> meantime. Would we handle that?
>>>>>>>
>>>>>> We create new connection irrespective of old connection state if
>>>>>> flask session was reset as explained above.
>>>>>>
>>>>>
>>>>> OK.
>>>>>
>>>>> So... we need to always warn the user that the connection has been
>>>>> reset, so they know if they've lost previous GUC changes or temp tables
>>>>> etc, and conversely, we need to reset the database connection to ensure
>>>>> thatGUC changes or temp tables don't end up getting re-associated with the
>>>>> wrong session. Sound right?
>>>>>
>>>>> Yes
>>>>
>>>>
>>>>> I assume updates to the patch are required?
>>>>>
>>>> Yes. I'll send updated one.
>>>>
>>>>
>>>>>
>>>>>
>>>>> --
>>>>> 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
>