On Friday, March 1, 2024 2:11 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> On Fri, Mar 1, 2024 at 12:42 PM Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com>
> wrote:
> >
> > On Friday, March 1, 2024 10:17 AM Zhijie Hou (Fujitsu)
> <houzj.f...@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
