RE: Move global variables of pgoutput to plugin private scope.

On Tuesday, September 19, 2023 1:44 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Sep 19, 2023 at 04:10:39AM +0000, Zhijie Hou (Fujitsu) wrote:
> > Currently we have serval global variables in pgoutput, but each of
> > them is inherently local to an individual pgoutput instance. This
> > could cause issues if we switch to different output plugin instance in
> > one session and could miss to reset their value in case of errors. The
> > analysis for each variable is as
> > follows:
>
> (Moved the last block of the message as per the relationship between
> RelationSyncCache and publications_valid).
>
> > - static HTAB *RelationSyncCache = NULL;
> >
> > pgoutput creates this hash table under cacheMemoryContext to remember
> > the relation schemas that have been sent, but it's local to an
> > individual pgoutput instance, and because it's under global memory
> > context, the hashtable is not properly cleared in error paths which
> > means it has a risk of being accessed in a different output plugin instance.
> This was also mentioned in another thread[2].
> >
> > So I think we'd better allocate this under output plugin private context.
> >
> > But note that, instead of completely moving the hash table into the
> > output plugin private data, we need to to keep the static pointer
> > variable for the map to be accessed by the syscache callbacks. This is
> > because syscache callbacks won't be un-registered even after shutting
> > down the output plugin, so we need a static pointer to cache the map pointer
> so that callbacks can check it.
> >
> > - static bool publications_valid;
> >
> > I thought we need to move this to private data as well, but we need to
> > access this in a syscache callback, which means we need to keep the static
> variable.
>
> FWIW, I think that keeping publications_valid makes the code kind of confusing
> once 0001 is applied, because this makes the handling of the cached data for
> relations and publications even more inconsistent than it is now, with a mixed
> bag of two different logics caused by the relationship between the synced
> relation cache and the publication
> cache: RelationSyncCache tracks if relations should be rebuilt, while
> publications_valid does it for the publication data, but both are still static and
> could be shared by multiple pgoutput contexts.  On top of that,
> publications_valid is hidden at the top of pgoutput.c within a bunch of
> declarations and no comments to explain why it's here (spoiler: to handle the
> cache rebuilds with its reset in the cache callback).
>
> I agree that CacheMemoryContext is not really a good idea to cache the data
> only proper to a pgoutput session and that tracking a context in the output
> data makes the whole cleanup attractive, but it also seems to me that we
> should avoid entirely the use of relcache callbacks if the intention is to have one
> RelationSyncEntry per pgoutput.  The patch does something different than
> HEAD and than having one RelationSyncEntry per pgoutout: RelationSyncEntry
> can reference *everything*, with its data stored in multiple memory contexts as
> of one per pgoutput.  It looks like RelationSyncEntry should be a list or a hash
> table, at least, so as it can refer to multiple pgoutput states.  Knowing that a
> session can only use one replication slot with MyReplicationSlot, not sure that's
> worth bothering with.  As a whole,
> 0001 with its changes for RelationSyncCache don't seem like an improvement
> to me.
>

Thanks for your comments. Currently, I am not sure how to avoid the use of the
syscache callback functions, So I think the change for RelationSyncCache needs
more thought and I will retry later if I find another way.

Best Regards,
Hou zj