Can you rebase this please? On Wed, Jan 10, 2018 at 9:47 AM, Harshal Dhumal < [email protected]> 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 < > [email protected]> 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 < >> [email protected]> wrote: >> >>> On Mon, Jan 8, 2018 at 7:26 PM, Dave Page <[email protected]> wrote: >>> >>>> >>>> >>>> On Mon, Jan 8, 2018 at 1:18 PM, Harshal Dhumal < >>>> [email protected]> wrote: >>>> >>>>> >>>>> On Mon, Jan 8, 2018 at 6:11 PM, Dave Page <[email protected]> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Mon, Jan 8, 2018 at 12:37 PM, Harshal Dhumal < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> On Mon, Jan 8, 2018 at 5:15 PM, Dave Page <[email protected]> wrote: >>>>>>> >>>>>>>> HI >>>>>>>> >>>>>>>> On Mon, Jan 8, 2018 at 11:41 AM, Harshal Dhumal < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> On Mon, Jan 8, 2018 at 4:34 PM, Dave Page <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Hi >>>>>>>>>> >>>>>>>>>> On Fri, Jan 5, 2018 at 7:50 AM, Harshal Dhumal < >>>>>>>>>> [email protected]> 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
