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

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.