Re: Accept recovery conflict interrupt on blocked writing

Hi,

On 2025-01-17 13:03:35 -0500, Andres Freund wrote:
> I don't see anything implementing the promotion of ERRORs to FATAL? You're
> preventing the error message being sent to the client, but I don't think that
> causes the connection to be terminated. The pre-existing code doesn't have
> that problem, because it's only active when ProcDiePending is already set.
>
> In fact, the test your patch added goes through
> ProcessRecoveryConflictInterrupts() multiple times:
>
> 2025-01-17 12:52:47.842 EST [3376411] LOG: recovery still waiting after 20.709 ms: recovery conflict on buffer pin
> 2025-01-17 12:52:47.842 EST [3376411] CONTEXT: WAL redo at 0/3462288 for Heap2/PRUNE_VACUUM_SCAN: , isCatalogRel: F, nplans: 0, nredirected: 0, ndead: 0, nun>
> 3376451: recovery conflict interrupt while blocked
> 3376451: recovery conflict processing done
> write(8192) = -1: 11/Resource temporarily unavailable
> 3376451: recovery conflict interrupt while blocked
> 3376451: recovery conflict processing done
> ...
> write(8192) = -1: 11/Resource temporarily unavailable
> 3376451: recovery conflict interrupt while blocked
> 3376451: recovery conflict processing done
> write(8192) = -1: 11/Resource temporarily unavailable
> 3376451: recovery conflict interrupt while blocked
> 2025-01-17 12:52:48.072 EST [3376451] 031_recovery_conflict.pl ERROR: canceling statement due to conflict with recovery
> 2025-01-17 12:52:48.072 EST [3376451] 031_recovery_conflict.pl DETAIL: User was holding shared buffer pin for too long.
> 2025-01-17 12:52:48.072 EST [3376451] 031_recovery_conflict.pl STATEMENT:
> BEGIN;
> DECLARE test_recovery_conflict_cursor CURSOR FOR SELECT b FROM test_recovery_conflict_table1;
> FETCH FORWARD FROM test_recovery_conflict_cursor;
> SELECT generate_series(1, 100000);
>
> backend 3376451> 2025-01-17 12:52:48.072 EST [3376411] LOG: recovery finished waiting after 250.681 ms: recovery conflict on buffer pin
>
> I don't actually know why the conflict ends up being resolved after a bunch of
> retries.

It's because the test sets deadlock_timeout lower than
max_standby_streaming_delay.

> Note also the "backend>" (to which I added the PID to identify it) which gets
> emitted. Just setting whereToSendOutput = DestNone has side effects when not
> actually in a process exit status...

I the the only reason the patch works at all is because we end up in
InteractiveBackend()'s EOF handling, because InteractiveBackend reads from
stdin. That's closed here, but I don't think we have any guarantee that stdin
isn't something that can be read from.

Greetings,

Andres Freund