On Fri, Mar 1, 2024 at 4:21 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, March 1, 2024 2:11 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> >
> > ---
> > +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 ...).
Why does it have to be a List in the first place? In earlier version
patches, we used to copy the list and delete the element until it
became empty, while waiting for physical wal senders. But we now just
refer to each slot name in the list. The current code assumes that
stnadby_slot_names_cpy is allocated in GUCMemoryContext but once it
changes, it will silently get broken. I think we can check and assign
standby_slot_names in a similar way to check/assign_temp_tablespaces
and check/assign_synchronous_standby_names.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com