Обсуждение: Move global variables of pgoutput to plugin private scope.

Hi,

Per complain in another thread[1], I started to look into the global
variables in pgoutput.

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:

- 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 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.

Example:
 SELECT data FROM pg_logical_slot_peek_binary_changes('isolation_slot_1', NULL, NULL, 'proto_version', '1',
'publication_names','pub', 'origin', 'none'); --- Set origin in this call. 
 SELECT data FROM pg_logical_slot_peek_binary_changes('isolation_slot_2', NULL, NULL, 'proto_version', '1',
'publication_names','pub');  -- Didn't set origin, but will reuse the origin flag in the first call. 

- 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.

- 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.

Attach the patches to change in_streaming, publish_no_origin and RelationSyncCache.
Suggestions and comments are welcome.

[1] https://www.postgresql.org/message-id/20230821182732.t3qc75i5s5xvovls%40awork3.anarazel.de
[2] https://www.postgresql.org/message-id/CAA4eK1LJ%3DCSsxETs5ydqP58OiWPiwodx%3DJqw89LQ7fMrRWqK9w%40mail.gmail.com

Best Regards,
Hou Zhijie

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

Hi Hou-san.

Given there are some issues raised about the 0001 patch [1] I am
skipping that one until I see the replies.

Meanwhile, here are some review comments for the patches v1-0002 and v1-0003

////////////////////
v1-0002

======
Commit message

1.
The pgoutput module uses a global variable(publish_no_origin) to cache the
action for the origin filter. But we only initialize publish_no_origin when
user specifies the "origin" in the output paramters which means we could refer
to an uninitialized variable if user didn't specify the paramter.

~

1a.

typos
/variable(publish_no_origin)/variable (publish_no_origin)/
/paramters/parameters/
/paramter./paramter./

~

1b.
"...we could refer to an uninitialized variable"

I'm not sure what this means. Previously it was static, so it wouldn't
be "uninitialised"; it would be false. Perhaps there might be a stale
value from a previous pgoutput, but IIUC that's the point made by your
next paragraph ("Besides, we don't...")

~~~

2.
To improve it, the patch stores the map within the private data of the output
plugin so that it will get initialized and reset along with the output plugin
context.

2a.
/To improve it,/To fix this/

~

2b.
"stores the map"

What map? This might be a cut/paste error from the v1-0001 patch comment.

////////////////////
v1-0003

======
Commit message

1.
Missing patch comment.

======
src/backend/replication/pgoutput/pgoutput.c

2. maybe_send_schema

- if (in_streaming)
+ if (data->in_streaming)
  set_schema_sent_in_streamed_txn((PGOutputData *) ctx->output_plugin_private,
  relentry, topxid);
~

Since you added a new 'data' variable, you might as well make use of
it here instead of doing "(PGOutputData *) ctx->output_plugin_private"
again.

======
src/include/replication/pgoutput.h

3.
  MemoryContext cachectx; /* private memory context for cache data */

+ bool in_streaming;
+

Even though there was no comment previously when this was static, IMO
it is better to comment on all the structure fields where possible.

------
[1] https://www.postgresql.org/message-id/ZQk1Ca_eFDTmBiZy%40paquier.xyz

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Sep 19, 2023 at 12:48 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> - 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.
>
> Example:
>  SELECT data FROM pg_logical_slot_peek_binary_changes('isolation_slot_1', NULL, NULL, 'proto_version', '1',
'publication_names','pub', 'origin', 'none'); --- Set origin in this call. 
>  SELECT data FROM pg_logical_slot_peek_binary_changes('isolation_slot_2', NULL, NULL, 'proto_version', '1',
'publication_names','pub');  -- Didn't set origin, but will reuse the origin flag in the first call. 
>

  char    *origin;
+ bool publish_no_origin;
 } PGOutputData;

Do we really need a new parameter in above structure? Can't we just
use the existing origin in the same structure? Please remember if this
needs to be backpatched then it may not be good idea to add new
parameter in the structure but apart from that having two members to
represent similar information doesn't seem advisable to me. I feel for
backbranch we can just use PGOutputData->origin for comparison and for
HEAD, we can remove origin and just use a boolean to avoid any extra
cost for comparisions for each change.

Can we add a test case to cover this case?

--
With Regards,
Amit Kapila.

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

On Tuesday, September 26, 2023 4:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Tue, Sep 19, 2023 at 12:48 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > - 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.
> >
> > Example:
> >  SELECT data FROM pg_logical_slot_peek_binary_changes('isolation_slot_1',
> NULL, NULL, 'proto_version', '1', 'publication_names', 'pub', 'origin', 'none'); ---
> Set origin in this call.
> >  SELECT data FROM pg_logical_slot_peek_binary_changes('isolation_slot_2',
> NULL, NULL, 'proto_version', '1', 'publication_names', 'pub');  -- Didn't set
> origin, but will reuse the origin flag in the first call.
> >
> 
>   char    *origin;
> + bool publish_no_origin;
>  } PGOutputData;
> 
> Do we really need a new parameter in above structure? Can't we just use the
> existing origin in the same structure? Please remember if this needs to be
> backpatched then it may not be good idea to add new parameter in the
> structure but apart from that having two members to represent similar
> information doesn't seem advisable to me. I feel for backbranch we can just use
> PGOutputData->origin for comparison and for HEAD, we can remove origin
> and just use a boolean to avoid any extra cost for comparisions for each
> change.

OK, I agree to remove the origin string on HEAD and we can add that back
when we support other origin value. I also modified to use the string for comparison
as suggested for back-branch. I will also test it locally to confirm it doesn't affect
the perf.

> 
> Can we add a test case to cover this case?

Added one in replorigin.sql.

Attach the patch set for publish_no_origin and in_streaming including the
patch(v2-PG16-0001) for back-branches. Since the patch for hash table need
more thoughts, I didn't post it this time.


Best Regards,
Hou zj

On Tue, Sep 26, 2023 at 01:55:10PM +0000, Zhijie Hou (Fujitsu) wrote:
> On Tuesday, September 26, 2023 4:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> Do we really need a new parameter in above structure? Can't we just use the
>> existing origin in the same structure? Please remember if this needs to be
>> backpatched then it may not be good idea to add new parameter in the
>> structure but apart from that having two members to represent similar
>> information doesn't seem advisable to me. I feel for backbranch we can just use
>> PGOutputData->origin for comparison and for HEAD, we can remove origin
>> and just use a boolean to avoid any extra cost for comparisions for each
>> change.
>
> OK, I agree to remove the origin string on HEAD and we can add that back
> when we support other origin value. I also modified to use the string for comparison
> as suggested for back-branch. I will also test it locally to confirm it doesn't affect
> the perf.

Err, actually, I am going to disagree here for the patch of HEAD.  It
seems to me that there is zero need for pgoutput.h and we don't need
to show PGOutputData to the world.  The structure is internal to
pgoutput.c and used only by its internal static routines.

Doing a codesearch in the Debian repos or just github shows that it is
used nowhere else, as well, something not really surprising as the
structure is filled and maintained in the file.
--
Michael

On Wed, Sep 27, 2023 at 9:10 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Sep 26, 2023 at 01:55:10PM +0000, Zhijie Hou (Fujitsu) wrote:
> > On Tuesday, September 26, 2023 4:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> Do we really need a new parameter in above structure? Can't we just use the
> >> existing origin in the same structure? Please remember if this needs to be
> >> backpatched then it may not be good idea to add new parameter in the
> >> structure but apart from that having two members to represent similar
> >> information doesn't seem advisable to me. I feel for backbranch we can just use
> >> PGOutputData->origin for comparison and for HEAD, we can remove origin
> >> and just use a boolean to avoid any extra cost for comparisions for each
> >> change.
> >
> > OK, I agree to remove the origin string on HEAD and we can add that back
> > when we support other origin value. I also modified to use the string for comparison
> > as suggested for back-branch. I will also test it locally to confirm it doesn't affect
> > the perf.
>
> Err, actually, I am going to disagree here for the patch of HEAD.  It
> seems to me that there is zero need for pgoutput.h and we don't need
> to show PGOutputData to the world.  The structure is internal to
> pgoutput.c and used only by its internal static routines.
>

Do you disagree with the approach for the PG16 patch or HEAD? You
mentioned HEAD but your argument sounds like you disagree with a
different approach for PG16.

--
With Regards,
Amit Kapila.

On Wed, Sep 27, 2023 at 09:39:19AM +0530, Amit Kapila wrote:
> On Wed, Sep 27, 2023 at 9:10 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Err, actually, I am going to disagree here for the patch of HEAD.  It
>> seems to me that there is zero need for pgoutput.h and we don't need
>> to show PGOutputData to the world.  The structure is internal to
>> Pgoutput.c and used only by its internal static routines.
>
> Do you disagree with the approach for the PG16 patch or HEAD? You
> mentioned HEAD but your argument sounds like you disagree with a
> different approach for PG16.

Only HEAD where the structure should be moved from pgoutput.h to
pgoutput.c, IMO.  The proposed patch for PG16 is OK as the size of the
structure should not change in a branch already released.
--
Michael

On Wed, Sep 27, 2023 at 9:46 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Sep 27, 2023 at 09:39:19AM +0530, Amit Kapila wrote:
> > On Wed, Sep 27, 2023 at 9:10 AM Michael Paquier <michael@paquier.xyz> wrote:
> >> Err, actually, I am going to disagree here for the patch of HEAD.  It
> >> seems to me that there is zero need for pgoutput.h and we don't need
> >> to show PGOutputData to the world.  The structure is internal to
> >> Pgoutput.c and used only by its internal static routines.
> >
> > Do you disagree with the approach for the PG16 patch or HEAD? You
> > mentioned HEAD but your argument sounds like you disagree with a
> > different approach for PG16.
>
> Only HEAD where the structure should be moved from pgoutput.h to
> pgoutput.c, IMO.
>

It's like that from the beginning. Now, even if we want to move, your
suggestion is not directly related to this patch as we are just
changing one field, and that too to fix a bug. We should start a
separate thread to gather a broader consensus if we want to move the
exposed structure to an internal file.

--
With Regards,
Amit Kapila.

On Wednesday, September 27, 2023 12:45 PM Amit Kapila <amit.kapila16@gmail.com>
> 
> On Wed, Sep 27, 2023 at 9:46 AM Michael Paquier <michael@paquier.xyz>
> wrote:
> >
> > On Wed, Sep 27, 2023 at 09:39:19AM +0530, Amit Kapila wrote:
> > > On Wed, Sep 27, 2023 at 9:10 AM Michael Paquier <michael@paquier.xyz>
> wrote:
> > >> Err, actually, I am going to disagree here for the patch of HEAD.
> > >> It seems to me that there is zero need for pgoutput.h and we don't
> > >> need to show PGOutputData to the world.  The structure is internal
> > >> to Pgoutput.c and used only by its internal static routines.
> > >
> > > Do you disagree with the approach for the PG16 patch or HEAD? You
> > > mentioned HEAD but your argument sounds like you disagree with a
> > > different approach for PG16.
> >
> > Only HEAD where the structure should be moved from pgoutput.h to
> > pgoutput.c, IMO.
> >
> 
> It's like that from the beginning. Now, even if we want to move, your
> suggestion is not directly related to this patch as we are just changing one field,
> and that too to fix a bug. We should start a separate thread to gather a broader
> consensus if we want to move the exposed structure to an internal file.

While searching the code, I noticed one postgres fork where the PGoutputData is
used in other places, although it's a separate fork, but it seems better to
discuss the removal separately.

[1]
https://github.com/Tencent/TBase/blob/7cf7f8afbcab7290538ad5e65893561710be3dfa/src/backend/replication/walsender.c#L100

Best Regards,
Hou zj

On Wed, Sep 27, 2023 at 10:15:24AM +0530, Amit Kapila wrote:
> It's like that from the beginning. Now, even if we want to move, your
> suggestion is not directly related to this patch as we are just
> changing one field, and that too to fix a bug. We should start a
> separate thread to gather a broader consensus if we want to move the
> exposed structure to an internal file.

As you wish.  You are planning to take care of the patches 0001 and
0002 posted on this thread, I guess?
--
Michael

On Wed, Sep 27, 2023 at 10:26 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Sep 27, 2023 at 10:15:24AM +0530, Amit Kapila wrote:
> > It's like that from the beginning. Now, even if we want to move, your
> > suggestion is not directly related to this patch as we are just
> > changing one field, and that too to fix a bug. We should start a
> > separate thread to gather a broader consensus if we want to move the
> > exposed structure to an internal file.
>
> As you wish.
>

Thanks.

>
>  You are planning to take care of the patches 0001 and
> 0002 posted on this thread, I guess?
>

I have tested and reviewed
v2-0001-Maintain-publish_no_origin-in-output-plugin-priv and
v2-PG16-0001-Maintain-publish_no_origin-in-output-plugin-priva patches
posted in the email [1]. I'll push those unless there are more
comments on them. I have briefly looked at
v2-0002-Move-in_streaming-to-output-private-data in the same email [1]
but didn't think about it in detail (like whether there is any live
bug that can be fixed or is just an improvement). If you wanted to
look and commit v2-0002-Move-in_streaming-to-output-private-data, I am
fine with that?

[1] -
https://www.postgresql.org/message-id/OS0PR01MB57164B085332DB677DBFA8E994C3A%40OS0PR01MB5716.jpnprd01.prod.outlook.com

--
With Regards,
Amit Kapila.

On Wed, Sep 27, 2023 at 04:51:29AM +0000, Zhijie Hou (Fujitsu) wrote:
> While searching the code, I noticed one postgres fork where the PGoutputData is
> used in other places, although it's a separate fork, but it seems better to
> discuss the removal separately.
>
> [1]
https://github.com/Tencent/TBase/blob/7cf7f8afbcab7290538ad5e65893561710be3dfa/src/backend/replication/walsender.c#L100

Indeed.  Interesting.
--
Michael

On Wed, Sep 27, 2023 at 10:51:52AM +0530, Amit Kapila wrote:
> I have briefly looked at
> v2-0002-Move-in_streaming-to-output-private-data in the same email [1]
> but didn't think about it in detail (like whether there is any live
> bug that can be fixed or is just an improvement).

This looks like an improvement to me, as at the startup of a stream
the flag is forcibly reset to a false state.  So, you cannot really
reach a state where a second stream could be started within the same
session but with a flag incorrectly set to true.  Tracking that in the
state data of pgoutput is cleaner, definitely.

> If you wanted to
> look and commit v2-0002-Move-in_streaming-to-output-private-data, I am
> fine with that?

Sure.  I found the concept behind 0002 sound.  Feel free to go ahead
with 0001, and I can always look at the second.  Always happy to help.
--
Michael

On Wed, Sep 27, 2023 at 04:57:06PM +0900, Michael Paquier wrote:
> Sure.  I found the concept behind 0002 sound.  Feel free to go ahead
> with 0001, and I can always look at the second.  Always happy to help.

For the sake of the archives:
- Amit has applied 0001 down to 16 as of 54ccfd65868c.
- I've applied 0002 after on HEAD as of 9210afd3bcd6.
--
Michael