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

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.

> - static bool publish_no_origin;
>
> This flag is also local to pgoutput instance, and we didn't reset the flag in
> output shutdown callback, so if we consume changes from different slots, then
> the second call would reuse the flag value that is set in the first call which
> is unexpected. To completely avoid this issue, we think we'd better move this
> flag to output plugin private data structure.

Yep, that's incorrect.

> - static bool in_streaming;
>
> While on it, I feel we can also move this flag to private data, although I didn't
> see problems for this one.

Moving this one to the private state data makes sense to me, as it
tracks the streaming of one PGOutputData.

Note that we name twice RelSchemaSyncCache in the code, but it does
not exist..
--
Michael