On Wednesday, March 6, 2024 11:04 AM Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com> wrote: > > On Wednesday, March 6, 2024 9:30 AM Masahiko Sawada > <sawada.m...@gmail.com> wrote: > > Hi, > > > On Fri, Mar 1, 2024 at 4:21 PM Zhijie Hou (Fujitsu) > > <houzj.f...@fujitsu.com> > > wrote: > > > > > > On Friday, March 1, 2024 2:11 PM Masahiko Sawada > > <sawada.m...@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. Best Regards, Hou zj
v107-0002-Document-the-steps-to-check-if-the-standby-is-r.patch
Description: v107-0002-Document-the-steps-to-check-if-the-standby-is-r.patch
v107-0001-Allow-logical-walsenders-to-wait-for-the-physic.patch
Description: v107-0001-Allow-logical-walsenders-to-wait-for-the-physic.patch