Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit - Mailing list pgsql-hackers
From | Fujii Masao |
---|---|
Subject | Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit |
Date | |
Msg-id | 6e9c9d45-c612-639a-f741-7798013c0ae4@oss.nttdata.com Whole thread Raw |
In response to | Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
Responses |
Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit
(Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
List | pgsql-hackers |
On 2021/01/29 14:12, Bharath Rupireddy wrote: > On Fri, Jan 29, 2021 at 10:28 AM Fujii Masao > <masao.fujii@oss.nttdata.com> wrote: >> On 2021/01/29 11:09, Tom Lane wrote: >>> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: >>>> On Fri, Jan 29, 2021 at 1:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=trilobite&dt=2021-01-26%2019%3A59%3A40 >>>>> This is a CLOBBER_CACHE_ALWAYS build, so I suspect what it's >>>>> telling us is that the patch's behavior is unstable in the face >>>>> of unexpected cache flushes. >>> >>>> Thanks a lot! It looks like the syscache invalidation messages are >>>> generated too frequently with -DCLOBBER_CACHE_ALWAYS build due to >>>> which pgfdw_inval_callback gets called many times in which the cached >>>> entries are marked as invalid and closed if they are not used in the >>>> txn. The new function postgres_fdw_get_connections outputs the >>>> information of the cached connections such as name if the connection >>>> is still open and their validity. Hence the output of the >>>> postgres_fdw_get_connections became unstable in the buildfarm member. >>>> I will further analyze making tests stable, meanwhile any suggestions >>>> are welcome. >>> >>> I do not think you should regard this as "we need to hack the test >>> to make it stable". I think you should regard this as "this is a >>> bug". A cache flush should not cause user-visible state changes. >>> In particular, the above analysis implies that you think a cache >>> flush is equivalent to end-of-transaction, which it absolutely >>> is not. >>> >>> Also, now that I've looked at pgfdw_inval_callback, it scares >>> the heck out of me. Actually disconnecting a connection during >>> a cache inval callback seems quite unsafe --- what if that happens >>> while we're using the connection? >> >> If the connection is still used in the transaction, pgfdw_inval_callback() >> marks it as invalidated and doesn't close it. So I was not thinking that >> this is so unsafe. >> >> The disconnection code in pgfdw_inval_callback() was added in commit >> e3ebcca843 to fix connection leak issue, and it's back-patched. If this >> change is really unsafe, we need to revert it immediately at least from back >> branches because the next minor release is scheduled soon. > > I think we can remove disconnect_pg_server in pgfdw_inval_callback and > make entries only invalidated. Anyways, those connections can get > closed at the end of main txn in pgfdw_xact_callback. Thoughts? But this revives the connection leak issue. So isn't it better to to do that after we confirm that the current code is really unsafe? > > If okay, I can make a patch for this. > >> BTW, even if we change pgfdw_inval_callback() so that it doesn't close >> the connection at all, ISTM that the results of postgres_fdw_get_connections() >> would not be stable because entry->invalidated would vary based on >> whether CLOBBER_CACHE_ALWAYS is used or not. > > Yes, after the above change (removing disconnect_pg_server in > pgfdw_inval_callback), our tests don't get stable because > postgres_fdw_get_connections shows the valid state of the connections. > I think we can change postgres_fdw_get_connections so that it only > shows the active connections server name but not valid state. Because, > the valid state is something dependent on the internal state change > and is not consistent with the user expectation but we are exposing it > to the user. Thoughts? I don't think that's enough because even the following simple queries return the different results, depending on whether CLOBBER_CACHE_ALWAYS is used or not. SELECT * FROM ft6; -- ft6 is the foreign table SELECT server_name FROM postgres_fdw_get_connections(); When CLOBBER_CACHE_ALWAYS is used, postgres_fdw_get_connections() returns no records because the connection is marked as invalidated, and then closed at xact callback in SELECT query. Otherwise, postgres_fdw_get_connections() returns at least one connection that was established in the SELECT query. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
pgsql-hackers by date: