Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

On Wed, Dec 30, 2020 at 11:11 PM Alexey Kondratov
<a(dot)kondratov(at)postgrespro(dot)ru> wrote:
> On 2020-12-30 17:59, Bharath Rupireddy wrote:
> > On Wed, Dec 30, 2020 at 5:20 PM Alexey Kondratov
> > <a(dot)kondratov(at)postgrespro(dot)ru> wrote:
> >>
> >> On 2020-12-30 09:10, Bharath Rupireddy wrote:
> >> I still have some doubts that it is worth of allowing to call
> >> postgres_fdw_disconnect() in the explicit transaction block, since it
> >> adds a lot of things to care and check for. But otherwise current
> >> logic
> >> looks solid.
> >>
> >> + errdetail("Such connections get
> >> closed either in the next use or
> >> at the end of the current transaction.")
> >> + : errdetail("Such connection gets
> >> closed either in the next use or
> >> at the end of the current transaction.")));
> >>
> >> Does it really have a chance to get closed on the next use? If foreign
> >> server is dropped then user mapping should be dropped as well (either
> >> with CASCADE or manually), but we do need user mapping for a local
> >> cache
> >> lookup. That way, if I understand all the discussion up-thread
> >> correctly, we can only close such connections at the end of xact, do
> >> we?
> >
> > The next use of such a connection in the following query whose foreign
> > server would have been dropped fails because of the cascading that can
> > happen to drop the user mapping and the foreign table as well. During
> > the start of the next query such connection will be marked as
> > invalidated because xact_depth of that connection is > 1 and when the
> > fail happens, txn gets aborted due to which pgfdw_xact_callback gets
> > called and in that the connection gets closed. To make it more clear,
> > please have a look at the scenarios [1].
> >
>
> In my understanding 'connection gets closed either in the next use'
> means that connection will be closed next time someone will try to use
> it, i.e. GetConnection() will be called and it closes this connection
> because of a bad state. However, if foreign server is dropped
> GetConnection() cannot lookup the connection because it needs a user
> mapping oid as a key.

Right. We don't reach GetConnection(). The look up in either
GetForeignTable() or GetUserMapping() or GetForeignServer() fails (and
so the query) depending one which one gets called first.

> I had a look on your scenarios. IIUC, under **next use** you mean a
> select attempt from a table belonging to the same foreign server, which
> leads to a transaction abort and connection gets closed in the xact
> callback. Sorry, maybe I am missing something, but this just confirms
> that such connections only get closed in the xact callback (taking into
> account your recently committed patch [1]), so 'next use' looks
> misleading.
>
> [1]
> https://www.postgresql.org/message-id/8b2aa1aa-c638-12a8-cb56-ea0f0a5019cf%40oss.nttdata.com

Right. I meant the "next use" as the select attempt on a foreign table
with that foreign server. If no select query is run, then at the end
of the current txn that connection gets closed. Yes internally such
connection gets closed in pgfdw_xact_callback.

If the errdetail("Such connections get closed either in the next use
or at the end of the current transaction.") looks confusing, how about

1) errdetail("Such connection gets discarded while closing the remote
transaction.")/errdetail("Such connections get discarded while closing
the remote transaction.")
2) errdetail("Such connection is discarded at the end of remote
transaction.")/errdetail("Such connections are discarded at the end of
remote transaction.")

I prefer 2) Thoughts?

Because we already print a message in pgfdw_xact_callback -
elog(DEBUG3, "closing remote transaction on connection %p"

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com