Re: START_REPLICATION SLOT causing a crash in an assert build

On 10/5/22 8:44 PM, Andres Freund wrote:
> Hi,
>
> On 2022-10-05 13:00:53 -0400, Jonathan S. Katz wrote:
>> On 9/27/22 1:52 AM, Kyotaro Horiguchi wrote:
>>> Thanks!
>>>
>>> At Mon, 26 Sep 2022 19:53:02 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in
>>>> I wonder if the correct fix here wouldn't be to move the slotname out of
>>>> PgStat_StatReplSlotEntry?
>>>
>>> Ugh. Right. I thought its outer struct as purely the part for the
>>> common header. But we can freely place anything after the header
>>> part. I moved it to the outer struct. I didn't clear that part in
>>> pgstat_create_relation() because it is filled in immediately.
>>>
>>> The attached is that.
>>
>> This is still listed as an open item[1] for v15. Does this fix proposed
>> address the issue?
>
> Unfortunately not - it doesn't even pass the existing tests
> (test_decoding/001_repl_stats fails) :(.

Thanks for checking.

> The reason for that is that with the patch nothing restores the slotname when
> reading stats from disk. That turns out not to cause immediate issues, but at
> the next shutdown the name won't be set, and we'll serialize the stats data
> with an empty string as the name.

Ah.

> I have two ideas how to fix it. As a design constraint, I'd be interested in
> the RMTs opinion on the following:
> Is a cleaner fix that changes the stats format (i.e. existing stats will be
> thrown away when upgrading) or one that doesn't change the stats format
> preferrable?

[My opinion, will let Michael/John chime in]

Ideally we would keep the stats on upgrade -- I think that's the better
user experience.

Thanks,

Jonathan