Horton

Re: Infinite loop in XLogPageRead() on standby

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Alexander Kukushkin <cyberdemn@gmail.com>
Cc: Ants Aasma <ants.aasma@cybertec.at>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, pgsql-hackers@postgresql.org, thomas.munro@gmail.com
Date: 2025-01-16T00:42:49Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix header check for continuation records where standbys could be stuck

  2. Move routines to manipulate WAL into PostgreSQL::Test::Cluster

  3. Prevent archive recovery from scanning non-existent WAL files.

  4. Fix scenario where streaming standby gets stuck at a continuation record.

Attachments

On Wed, Jan 15, 2025 at 10:35:42AM +0100, Alexander Kukushkin wrote:
>  Thank you for picking it up. I briefly looked at both patches. The actual
> fix in XLogPageRead() looks good to me.
> I also agree with suggested refactoring, where there is certainly some room
> for improvement - $WAL_SEGMENT_SIZE, $WAL_BLOCK_SIZE variables and
> get_int_setting(), start_of_page() funcs are still duplicated in both test
> files.
> Maybe we can have something like the following in Cluster.pm? It'll allow
> further simplify tests and reduce code duplication.

Yeah, I was looking at that, but disliked a bit this option compared
to the routines doing WAL manipulations which are more complex with
their own ways of getting close to page limits, because I feel that we
should be smarter with the interfaces of these routines with more
options.  For example, we have other things scanning control file
data like system_identifier in 040_pg_createsubscriber.p, so we could
have a SQL that wraps around the pg_control_* functions with a custom
field, like we do for "sub lsn" in Cluster.pm.  Same comment for the
pg_settings queries, which are not limited to only what we do here.
The wrapper around integers is useful, of course, but we could make a
refactored routine apply a cast after checking its unit internally,
or something like that.

The routines for the start page position would not be fit within
Cluster.pm as it does not depend on a $node.  Perhaps Utils.pm?  Here
again, living with this small duplication felt OK in the scope of this
bug fix.  I'm of course open to tuning all that, though my primary
goal is to wrap the fix :D

I've applied the first refactoring bits down to v13 (see for example a
s/emit_message/emit_wal/ tweaked for consistency, with more comment
tweaks).  Attached are patches for each branch for the bug fix, that
I'm still testing and looking at more.  The readability of
043_no_contrecord_switch.pl looks rather OK here.
--
Michael