Re: START_REPLICATION SLOT causing a crash in an assert build

At Fri, 7 Oct 2022 12:14:40 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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