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

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?

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?

If okay, I can work on the patch for this.

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