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

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