On Wednesday, March 6, 2024 9:13 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, March 6, 2024 11:04 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Wednesday, March 6, 2024 9:30 AM Masahiko Sawada
> > <sawada.mshk@gmail.com> wrote:
> >
> > Hi,
> >
> > > 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?
> >
> > I thought the List type is convenient to use here, as we have existing
> > list build function(SplitIdentifierString), and have convenient list
> > macro to loop the
> > list(foreach_ptr) which can save some codes.
> >
> > > 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.
> >
> > Yes, we could do follow it by allocating an array and copy each slot
> > name into it, but it also requires some codes to build and scan the
> > array. So, is it possible to expose the GucMemorycontext or have an API like
> guc_copy_list instead ?
> > If we don't want to touch the guc api, I am ok with using an array as well.
>
> I rethink about this and realize that it's not good to do the memory allocation in
> assign hook function. As the "src/backend/utils/misc/README" said, we'd
> better do that in check hook function and pass it via extra to assign hook
> function. And thus array is a good choice in this case rather than a List which
> cannot be passed to *extra.
>
> Here is the V107 patch set which parse and cache the standby slot names in an
> array instead of a List.
The patch needs to be rebased due to recent commit.
Attach the V107_2 path set. There are no code changes in this version.
Best Regards,
Hou zj