RE: Synchronizing slots from primary to standby

On Friday, March 1, 2024 2:11 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> 
> On Fri, Mar 1, 2024 at 12:42 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > On Friday, March 1, 2024 10:17 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> > >
> > >
> > > Attach the V102 patch set which addressed Amit and Shveta's comments.
> > > Thanks Shveta for helping addressing the comments off-list.
> >
> > The cfbot reported a compile warning, here is the new version patch
> > which fixed it, Also removed some outdate comments in this version.
> >
> 
> I've reviewed the v102-0001 patch. Here are some comments:

Thanks for the comments !

> 
> ---
> I got a compiler warning:
> 
> walsender.c:1829:6: warning: variable 'wait_event' is used uninitialized
> whenever '&&' condition is false [-Wsometimes-uninitialized]
>         if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> walsender.c:1871:7: note: uninitialized use occurs here
>                 if (wait_event ==
> WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL)
>                     ^~~~~~~~~~
> walsender.c:1829:6: note: remove the '&&' if its condition is always true
>         if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> walsender.c:1818:20: note: initialize the variable 'wait_event' to silence this
> warning
>         uint32          wait_event;
>                                   ^
>                                    = 0
> 1 warning generated.


Thanks for reporting, it was fixed in V102_2.

> 
> ---
> +void
> +assign_standby_slot_names(const char *newval, void *extra) {
> +        List      *standby_slots;
> +        MemoryContext oldcxt;
> +        char      *standby_slot_names_cpy = extra;
> +
> 
> Given that the newval and extra have the same data (standby_slot_names
> value), why do we not use newval instead? I think that if we use
> newval, we don't need to guc_strdup() in check_standby_slot_names(),
> we might need to do list_copy_deep() instead, though. It's not clear
> to me as there is no comment.

I think SplitIdentifierString will modify the passed in string, so we'd better
not pass the newval to it, otherwise the stored guc string(standby_slot_names)
will be changed. I can see we are doing similar thing in other GUC check/assign
function as well. (check_wal_consistency_checking/
assign_wal_consistency_checking, check_createrole_self_grant/
assign_createrole_self_grant ...).

> ---
> +        /*
> +         * Switch to the memory context under which GUC variables are
> allocated
> +         * (GUCMemoryContext).
> +         */
> +        oldcxt =
> MemoryContextSwitchTo(GetMemoryChunkContext(standby_slot_names_cpy
> ));
> +        standby_slot_names_list = list_copy(standby_slots);
> +        MemoryContextSwitchTo(oldcxt);
> 
> Why do we not explicitly switch to GUCMemoryContext?

I think it's because the GUCMemoryContext is not exposed.

Best Regards,
Hou zj