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

On 2021/01/29 14:12, Bharath Rupireddy wrote:
> On Fri, Jan 29, 2021 at 10:28 AM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> On 2021/01/29 11:09, Tom Lane wrote:
>>> Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> writes:
>>>> On Fri, Jan 29, 2021 at 1:52 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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