Re: START_REPLICATION SLOT causing a crash in an assert build

Hi,

On 2022-10-07 15:30:43 +0900, Kyotaro Horiguchi wrote:
> 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.

Thanks for thinking through this!

> 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.

I don't agree with this aspect. I think it's better to ensure that the stats
object exists when acquiring a slot, rather than later, when reporting. It's a
lot simpler to think through the lock nesting etc that way.

I'd also eventually like to remove the stats that are currently kept
separately in ReorderBuffer, and that will be easier/cheaper if we can rely on
the stats objects to have already been created.

Greetings,

Andres Freund