On Thu, Dec 21, 2023 at 11:30 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, December 21, 2023 12:25 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Here is a minor comment for v51-0001
> >
> > ======
> > src/backend/replication/slot.c
> >
> > 1.
> > +void
> > +RereadConfigAndReInitSlotList(List **standby_slots) {
> > + char *pre_standby_slot_names;
> > +
> > + /*
> > + * If we are running on a standby, there is no need to reload
> > + * standby_slot_names since we do not support syncing slots to
> > + cascading
> > + * standbys.
> > + */
> > + if (RecoveryInProgress())
> > + {
> > + ProcessConfigFile(PGC_SIGHUP);
> > + return;
> > + }
> > +
> > + pre_standby_slot_names = pstrdup(standby_slot_names);
> > +
> > + ProcessConfigFile(PGC_SIGHUP);
> > +
> > + if (strcmp(pre_standby_slot_names, standby_slot_names) != 0) {
> > + list_free(*standby_slots); *standby_slots = GetStandbySlotList(true);
> > + }
> > +
> > + pfree(pre_standby_slot_names);
> > +}
> >
> > Consider below, which seems a simpler way to do that but with just one return
> > point and without duplicating the ProcessConfigFile calls:
> >
> > SUGGESTION
> > {
> > char *pre_standby_slot_names = pstrdup(standby_slot_names);
> >
> > ProcessConfigFile(PGC_SIGHUP);
> >
> > if (!RecoveryInProgress())
> > {
> > if (strcmp(pre_standby_slot_names, standby_slot_names) != 0)
> > {
> > list_free(*standby_slots);
> > *standby_slots = GetStandbySlotList(true);
> > }
> > }
> >
> > pfree(pre_standby_slot_names);
> > }
>
> Thanks for the suggestion. I also thought about this, but I'd like to avoid
> allocating/freeing memory for the pre_standby_slot_names if not needed.
>
> Best Regards,
> Hou zj
>
>
PFA v52. Changes are:
1) Addressed comments given for v48-002 in [1] and v50-002 in [2]
2) Merged patch003 (test improvement) to patch002 itself.
3) Restructured code around ReplicationSlotDrop to remove extra arg 'user_cmd'
4) Fixed a bug wherein promotion flow was breaking. The pid of
slot-sync worker was nullified in slotsync_worker_onexit() before the
worker can release the acquired slot in ReplicationSlotShmemExit().
Due to this, the startup process which relies on worker's pid tried to
drop the 'i' state slots assuming the slot sync worker has stopped
whereas the slot sync worker was trying to modify the slot
concurrently, resulting into the problem. This was due to the fact
that slotsync_worker_onexit() was registered with before_shmem_exit().
It should instead be registered using on_shmem_exit(). Corrected it
now. Thanks Hou-San for working on this.
[1]: https://www.postgresql.org/message-id/CAA4eK1J5zTmm4NE4os59WgU4AZPNb74X-n67pY8SkoDfzsN_jA%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAHut%2BPvocO_bwwz7kD-4mLnFRCLOK3i0ocLyGDvLQKzkhzEjTg%40mail.gmail.com