On Mon, Nov 6, 2023 at 7:01 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, November 3, 2023 7:32 PM Amit Kapila <amit.kapila16@gmail.com>
> >
> > 5.
> > @@ -228,6 +230,28 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo
> > fcinfo, bool confirm, bool bin
> > NameStr(MyReplicationSlot->data.plugin),
> > format_procedure(fcinfo->flinfo->fn_oid))));
> > ..
> > + if (XLogRecPtrIsInvalid(upto_lsn))
> > + wal_to_wait = end_of_wal;
> > + else
> > + wal_to_wait = Min(upto_lsn, end_of_wal);
> > +
> > + /* Initialize standby_slot_names_list */ SlotSyncInitConfig();
> > +
> > + /*
> > + * Wait for specified streaming replication standby servers (if any)
> > + * to confirm receipt of WAL upto wal_to_wait.
> > + */
> > + WalSndWaitForStandbyConfirmation(wal_to_wait);
> > +
> > + /*
> > + * The memory context used to allocate standby_slot_names_list will be
> > + * freed at the end of this call. So free and nullify the list in
> > + * order to avoid usage of freed list in the next call to this
> > + * function.
> > + */
> > + SlotSyncFreeConfig();
> >
> > What if there is an error in WalSndWaitForStandbyConfirmation() before calling
> > SlotSyncFreeConfig()? I think the problem you are trying to avoid by freeing it
> > here can occur. I think it is better to do this in a logical decoding context and
> > free the list along with it as we are doing in commit c7256e6564(see PG15).
>
> I will analyze more about this case and update in next version.
>
Okay, thanks for considering it.
> > Also,
> > it is better to allocate this list somewhere in
> > WalSndWaitForStandbyConfirmation(), probably in WalSndGetStandbySlots,
> > that will make the code look neat and also avoid allocating this list when
> > failover is not enabled for the slot.
>
> Changed as suggested.
>
After doing this, do we need to call SlotSyncInitConfig() from other
places as below?
+ SlotSyncInitConfig();
+ WalSndGetStandbySlots(&standby_slot_cpy, false);
Can we entirely get rid of calling SlotSyncInitConfig() from all
places except WalSndGetStandbySlots()? Also, after that or otherwise,
the comments atop also need modification.
--
With Regards,
Amit Kapila.