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