Re: START_REPLICATION SLOT causing a crash in an assert build

At Fri, 7 Oct 2022 12:14:40 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in 
> > What about if we go the other direction - simply remove the name from the
> > stats entry at all. I don't actually think we need it anymore. Unless I am
> > missing something right now - entirely possible! - the danger that
> > pgstat_acquire_replslot() mentions doesn't actually exist [anymore]. After a
> > crash we throw away the old stats data and if a slot is dropped while shut
> > down, we'll not load the slot data at startup.

The key point  of this is this:

+     * XXX: I think there cannot actually be data from an older slot
+     * here. After a crash we throw away the old stats data and if a slot is
+     * dropped while shut down, we'll not load the slot data at startup.

I think this is true.  Assuming that we don't recreate or rename
objects that have stats after writing out stats, we won't have stats
for a different object with the same name.  If we can rely on that
fact, the existing check in pgstat_acquire_replslot() becomes
useless. Thus we don't need to store object name in stats entry. I
agree to that.

> +1. I think it works. Since the replication slot index doesn't change
> during server running we can fetch the name from
> ReplicationSlotCtl->replication_slots.

That access seems safe in a bit different aspect, too. Both
checkpointer (and walsender) properly initialize ReplicationSlotCtl.


> If we don't need the name in stats entry, pgstat_acquire_replslot() is
> no longer necessary?

I think so. The entry will be created at the first report.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center