On Sat, Jun 21, 2025 at 08:56:50PM -0400, Tom Lane wrote:
> Hmm. My theory about what's happening is that we are writing a WAL
> record that spans across a page boundary, and the asynchronous
> immediate-stop request comes in and kills that operation, so that
> the first half of the record is on disk but the second is not.
> (This outcome would obviously be very timing-dependent.) Then
> we restart the server, and crash recovery copes with that situation
> successfully. But pg_logical_slot_get_changes() fails to, because
> it's coded in such a way that it will wait forever for the second
> half of the WAL record to appear.
>
> The main hole in this theory is: if crash recovery thought that the
> partial record was invalid, wouldn't it set the end-of-WAL position
> to the start of that record? And shouldn't that stop
> pg_logical_slot_get_changes() from trying to process the partial
> record? So I'm sure there's some details not quite right about
> this picture. But maybe it's within hailing distance of the truth.
Obviously pg_logical_slot_get_changes() should not try to read the
record if it's not complete yet. XLogDecodeNextRecord() may be
trusting too much the LSN given by its caller, though.
>> The new tests just spotted the existing bug.
>
> Yeah, that's what I think too. The unintentional omission of a
> pre-shutdown delay in the 046 test has exposed some pre-existing
> fragility in pg_logical_slot_get_changes(). So I'm not in favor
> of changing 046 till we understand this better.
Yes, better to understand what's going on before changing the test,
and perhaps change 046 so as it provide stable coverage for this case,
even if discovered accidentally.
So, is it only pg_logical_slot_get_changes_guts() that's messed up in
this context, because XLogDecodeNextRecord() is trying to read pages
as it has a logic too permissive? How did any of you reproduce the
failure? Just by running the test in a loop? It is one of these
patterns where a hardcoded sleep should do the trick and help with a
bisect.
By the way, At the bottom of test 046_checkpoint_logical_slot.pl, if
you expect the checkpoint to complete before sending the immediate
shutdown request, I would suggest to use a wait_for_log() based on
"checkpoint complete" or an equivalent loop. What you are doing in
the test is unstable as written.
--
Michael
