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

On 2021/01/22 1:17, Bharath Rupireddy wrote:
> On Thu, Jan 21, 2021 at 8:58 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> My opinion is to check "!all", but if others prefer using such boolean flag,
>> I'd withdraw my opinion.
>
> I'm really sorry, actually if (!all) is enough there, my earlier
> understanding was wrong.
>
>> + if ((all || entry->server_hashvalue == hashvalue) &&
>>
>> What about making disconnect_cached_connections() accept serverid instead
>> of hashvalue, and perform the above comparison based on serverid? That is,
>> I'm thinking "if (all || entry->serverid == serverid)". If we do that, we can
>> simplify postgres_fdw_disconnect() a bit more by getting rid of the calculation
>> of hashvalue.
>
> That's a good idea. I missed this point. Thanks.
>
>> + if ((all || entry->server_hashvalue == hashvalue) &&
>> + entry->conn)
>>
>> I think that it's better to make the check of "entry->conn" independent
>> like other functions in postgres_fdw/connection.c. What about adding
>> the following check before the above?
>>
>> /* Ignore cache entry if no open connection right now */
>> if (entry->conn == NULL)
>> continue;
>
> Done.
>
>> + /*
>> + * If the server has been dropped in the current explicit
>> + * transaction, then this entry would have been invalidated
>> + * in pgfdw_inval_callback at the end of drop sever
>> + * command. Note that this connection would not have been
>> + * closed in pgfdw_inval_callback because it is still being
>> + * used in the current explicit transaction. So, assert
>> + * that here.
>> + */
>> + Assert(entry->invalidated);
>>
>> As this comment explains, even when the connection is used in the transaction,
>> its server can be dropped in the same transaction. The connection can remain
>> until the end of transaction even though its server has been already dropped.
>> I'm now wondering if this behavior itself is problematic and should be forbid.
>> Of course, this is separate topic from this patch, though..
>>
>> BTW, my just idea for that is;
>> 1. change postgres_fdw_get_connections() return also serverid and xact_depth.
>> 2. make postgres_fdw define the event trigger on DROP SERVER command so that
>> an error is thrown if the connection to the server is still in use.
>> The event trigger function uses postgres_fdw_get_connections() to check
>> if the server connection is still in use or not.
>>
>> I'm not sure if this just idea is really feasible or not, though...
>
> I'm not quite sure if we can create such a dependency i.e. blocking
> "drop foreign server" when at least one session has an in use cached
> connection on it?

Maybe my explanation was not clear... I was thinking to prevent the server whose connection is used *within the current transaction* from being dropped. IOW, I was thinking to forbid the drop of server if xact_depth of its connection is more than one. So one session can drop the server even when its connection is open in other session if it's not used within the transaction (i.e., xact_depth == 0).

BTW, for now, if the connection is used within the transaction, other session cannot drop the corresponding server because the transaction holds the lock on the relations that depend on the server. Only the session running that transaction can drop the server. This can cause the issue in discussion.

So, my just idea is to disallow even that session running the transaction to drop the server. This means that no session can drop the server while its connection is used within the transaction (xact_depth > 0).

> What if a user wants to drop a server from one
> session, all other sessions one after the other keep having in-use
> connections related to that server, (though this use case sounds
> impractical) will the drop server ever be successful? Since we can
> have hundreds of sessions in real world postgres environment, I don't
> know if it's a good idea to create such dependency.
>
> As you suggested, this point can be discussed in a separate thread and
> if any of the approaches proposed by you above is finalized we can
> extend postgres_fdw_get_connections anytime.
>
> Thoughts?

I will consider more before starting separate discussion!

>
> Attaching v16 patch set, addressing above review comments and also
> added a test case suggested upthread that postgres_fdw_disconnect()
> with existing server name returns false that is when the cache doesn't
> have active connection.
>
> Please review the v16 patch set further.

Thanks! Will review that later.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION