Horton
Menu

Thread

Commits

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

  1. Undo thinko in commit e78d1d6d4.

  2. Fix orphaned processes when startup process fails during PM_STARTUP

  1. Return value of XLogInsertRecord() for XLOG_SWITCH record

    cca5507 <cca5507@qq.com> — 2026-04-13T08:47:38Z

    Hi,

I find that the return value of XLogInsertRecord() for XLOG_SWITCH record
is inconsistent with other records.

For XLOG_SWITCH record:

```
		/*
		 * Even though we reserved the rest of the segment for us, which is
		 * reflected in EndPos, we return a pointer to just the end of the
		 * xlog-switch record.
		 */
		if (inserted)
		{
			EndPos = StartPos + SizeOfXLogRecord;
			if (StartPos / XLOG_BLCKSZ != EndPos / XLOG_BLCKSZ)
			{
				uint64		offset = XLogSegmentOffset(EndPos, wal_segment_size);

				if (offset == EndPos % XLOG_BLCKSZ)
					EndPos += SizeOfXLogLongPHD;
				else
					EndPos += SizeOfXLogShortPHD;
			}
		}
```

It is equivalent to XLogBytePosToRecPtr(), but all other records use XLogBytePosToEndRecPtr().

No actual problem found yet, but I think it's better to keep them consistent. Thoughts?

--
Regards,
ChangAo Chen

  2. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    jihyun bahn <rring0727@gmail.com> — 2026-04-21T04:25:09Z

    Hi ChangAo,

I tested v1 of this patch.

Environment:
- PostgreSQL master (commit 9b43e6793b0)
- Ubuntu 24.04, gcc 13.3.0, x86_64
- Configured with --enable-cassert --enable-debug --enable-tap-tests

The patch applies cleanly and builds without new warnings.

Testing performed:
- make check: PASS
- make check-world: PASS
- make -C src/test/recovery check: PASS (599 tests)
In particular, t/043_no_contrecord_switch.pl and t/039_end_of_wal.pl
both pass, which exercise the XLOG_SWITCH boundary handling.

Manual verification:
I ran pg_switch_wal() twice with some activity in between and observed
that the returned LSN and subsequent pg_current_wal_lsn() values are
consistent with segment boundaries and page headers:

SELECT pg_switch_wal(); -- 0/0178FE38
SELECT pg_current_wal_lsn(); -- 0/02000000 (new segment)

-- ... some DDL and INSERT ...

SELECT pg_switch_wal(); -- 0/02026528
SELECT pg_current_wal_lsn(); -- 0/03000060 (new segment + 0x60
-- == SizeOfXLogLongPHD)

The 0x60 offset matches the long page header size, which is what the
original code was computing via the explicit SizeOfXLogLongPHD /
SizeOfXLogShortPHD branches. The refactored version using
XLogBytePosToEndRecPtr(XLogRecPtrToBytePos(StartPos) +
MAXALIGN(SizeOfXLogRecord)) produces the same result while matching
the pattern used elsewhere in XLogInsertRecord().

One question:
The original code did not apply MAXALIGN() to SizeOfXLogRecord before
adding it. In practice SizeOfXLogRecord is likely already MAXALIGN'd
(given typical record header layout), but could you confirm whether
MAXALIGN() here is a correctness fix, a defensive no-op, or something
that requires a separate note in the commit message?

Otherwise the change looks good to me, and I think it's a reasonable
cleanup.

Tested-by: Jihyun Bahn <rring0727@gmail.com>

Regards,
Jihyun Bahn

  3. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    cca5507 <cca5507@qq.com> — 2026-04-21T11:35:58Z

    Hi, thanks for the test!

> One question:
> The original code did not apply MAXALIGN() to SizeOfXLogRecord before
> adding it. In practice SizeOfXLogRecord is likely already MAXALIGN'd
> (given typical record header layout), but could you confirm whether
> MAXALIGN() here is a correctness fix, a defensive no-op, or something
> that requires a separate note in the commit message?
> 
> Otherwise the change looks good to me, and I think it's a reasonable
> cleanup.

I apply the MAXALIGN() to keep it consistent with ReserveXLogSwitch(), the
value seems unchanged though.

Attach v2 which is more efficient.

--
Regards,
ChangAo Chen

  4. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    jihyun bahn <rring0727@gmail.com> — 2026-04-22T01:23:34Z

    Hi ChangAo,

Thanks for the explanation and v2. I re-tested the new version and
also ran a comparison against unpatched master to better understand
the behavior change.

## Test environment

Commit: 64b2b421248 (today's master)
OS: Ubuntu 24.04 LTS on AWS EC2 (t3.xlarge, 4 vCPU, 16 GB)
Build: --enable-cassert --enable-debug --enable-tap-tests
CFLAGS="-O0 -ggdb -fno-omit-frame-pointer"

## Test results for v2

make check: All 248 tests passed
make -C src/test/recovery check: All 599 tests passed (262s)

Nothing new from the build side — clean compile, no new warnings.

## Manual verification

A few observations from exercising pg_switch_wal() repeatedly:

-- After some activity that lands mid-segment:
SELECT pg_switch_wal(); -> 0/03000078
SELECT pg_current_wal_lsn(); -> 0/04000000

-- Immediately calling switch again (now exactly on segment boundary):
SELECT pg_switch_wal(); -> 0/04000000 (no-op, same LSN)
SELECT pg_switch_wal(); -> 0/04000000 (still no-op)

So pg_switch_wal() is idempotent at segment boundaries — which is
what one would hope for.

## Comparison with unpatched master

I stashed v2, rebuilt, and re-ran the same sequence on current
master. The externally observable LSN behavior is identical:
repeated pg_switch_wal() calls at a segment boundary all return
the same LSN with no further WAL written.

This makes sense: when ReserveXLogSwitch() determines there's no
space worth switching, XLogInsertRecord() enters the branch with
inserted == false, so the EndPos adjustment logic on the
`if (inserted)` path isn't actually reached at segment boundaries
in current code.

## My take on v2

The new `EndPos % XLOG_BLCKSZ != 0` guard doesn't fix an
externally observable bug in current master — ReserveXLogSwitch()
already prevents the problematic case from reaching this code path.
But the guard makes the invariant local to XLogInsertRecord()
rather than depending on the caller's contract, and the added
comment referencing XLogBytePosToEndRecPtr() ties the two paths
together nicely. I think that's a worthwhile defensive improvement.

Going back to the structure of the original code (rather than the
v1 helper-based rewrite) also seems reasonable — no function-call
overhead, and the arithmetic is clear enough with the new comment.

The MAXALIGN() consistency argument with ReserveXLogSwitch() makes
sense to me.

Tested-by: Jihyun Bahn <rring0727@gmail.com>

Regards,
Jihyun Bahn


2026년 4월 21일 (화) 오후 8:36, cca5507 <cca5507@qq.com>님이 작성:

> Hi, thanks for the test!
>
> > One question:
> > The original code did not apply MAXALIGN() to SizeOfXLogRecord before
> > adding it. In practice SizeOfXLogRecord is likely already MAXALIGN'd
> > (given typical record header layout), but could you confirm whether
> > MAXALIGN() here is a correctness fix, a defensive no-op, or something
> > that requires a separate note in the commit message?
> >
> > Otherwise the change looks good to me, and I think it's a reasonable
> > cleanup.
>
> I apply the MAXALIGN() to keep it consistent with ReserveXLogSwitch(), the
> value seems unchanged though.
>
> Attach v2 which is more efficient.
>
> --
> Regards,
> ChangAo Chen
>

  5. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    ZizhuanLiu X-MAN <44973863@qq.com> — 2026-06-10T13:28:13Z

    >Original
>From: cca5507 <cca5507@qq.com>
>Date: 2026-04-21 19:35
>To:반지현<rring0727@gmail.com>, pgsql-hackers <pgsql-hackers@lists.postgresql.org>
>Subject: Re: Return value of XLogInsertRecord() for XLOG_SWITCH record
>
>
>Hi, thanks for the test!
>
>> One question:
>> The original code did not apply MAXALIGN() to SizeOfXLogRecord before
>> adding it. In practice SizeOfXLogRecord is likely already MAXALIGN'd
>> (given typical record header layout), but could you confirm whether
>> MAXALIGN() here is a correctness fix, a defensive no-op, or something
>> that requires a separate note in the commit message 
>> 
>> Otherwise the change looks good to me, and I think it's a reasonable
>> cleanup.
>
>I apply the MAXALIGN() to keep it consistent with ReserveXLogSwitch(), the
>value seems unchanged though.
>
>Attach v2 which is more efficient.
>
>--
>Regards,
>ChangAo Chen


Hi, cca5507,반지현,

Thanks for your patch and test— it’s sparked my deeper dive into the WAL switch logic.

I’ve done a preliminary survey of all top-level callers of XLogInsertRecord() 
that generate XLOG_SWITCH records, and grouped them into three categories:
1.do_pg_backup_start(), do_pg_backup_stop(), ShutdownXLOG()
    These callers completely ignore the return value of XLogInsertRecord(), 
so a semantic change here would have zero internal impact on them.

2.CheckArchiveTimeout()
  It stores the returned LSN into switchpoint only for debug logging:
```c
if (XLogSegmentOffset(switchpoint, wal_segment_size) != 0)
    elog(DEBUG1, "write-ahead log switch forced (\"archive_timeout\"=%d)", 
    XLogArchiveTimeout);
```c
The offset check here is just a trivial debug print, and would not 
introduce functional defects even if the returned LSN changes its meaning.

3.pg_switch_wal(PG_FUNCTION_ARGS) — the critical public entry point
    This function directly returns the LSN from XLogInsertRecord() as 
its SQL-level return value exposed to end users, scripts,  and external tooling.

Risk analysis:
From the above classification, internal PostgreSQL core logic suffers 
no functional breakage if we unify the return value semantics of XLogInsertRecord().

However, there are compatibility risks for external consumers relying on pg_switch_wal()’s output:
Custom applications, backup scripts, and monitoring tools may take 
the returned LSN as the exact position of the XLOG_SWITCH record to 
run space / time range analysis between the switch record and segment end.

Since we cannot guarantee how existing third-party systems interpret this return 
LSN, altering its definition would break established external workflows and trigger 
unpredictable side effects for legacy deployments.

That is the main backward-compatibility risk I can identify for this change.

Happy to hear your thoughts or any corrections to my analysis.


regards,
--
ZizhuanLiu (X-MAN) 
44973863@qq.com

  6. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    jihyun bahn <rring0727@gmail.com> — 2026-06-11T06:53:19Z

    Hi ZizhuanLiu,

Thanks for the caller survey — classifying the call sites by how they
consume the return value is a useful way to frame the risk, and I agree
pg_switch_wal() is the one path where the LSN escapes to external
consumers.

To help bound that risk, I re-ran my April comparison on a second
platform (Windows 11, MSYS2/gcc 16.1.0, commit 9d141466ff, 19beta1),
building unpatched master and v2 side by side and running the same
sequence on identically-initialized clusters:

  before_switch:  0/017CF958          (both builds)
  switch_1:       0/017CF970          (both builds)
  after_1:        0/02000000          (both builds)
  switch_2/3:     0/02000000, no-op   (both builds)

The two builds returned byte-for-byte identical LSNs at every step,
and "make check" passes on the patched build (All 245 tests passed).
Incidentally this also confirms numerically that the MAXALIGN() change
is a no-op: 0/017CF958 + 24 = 0/017CF970, since SizeOfXLogRecord is
already 8-byte aligned.

As far as I can tell, the only input where v2 can return a different
value than current master is when the XLOG_SWITCH record ends exactly
on a page boundary (StartPos at page offset XLOG_BLCKSZ - 24): old code
takes the cross-page branch and adds a page header size even though no
part of the record lies on the next page, while v2 returns the boundary
itself. The v2 value is the one consistent with the end-pointer
convention of XLogBytePosToEndRecPtr(). So any external tool that
observed a different value in that rare alignment was depending on a
value inconsistent with PostgreSQL's own end-pointer semantics — I'd
read v2 as correcting that, rather than breaking compatibility.

So in summary: identical SQL-visible behavior on all common paths
(verified on Linux in April and Windows now), with the single divergent
case being a consistency fix. If it would help, I could try to craft a
reproduction of the exact page-boundary case (padding WAL position via
pg_logical_emit_message), or write a small TAP test pinning down the
boundary behavior of pg_switch_wal().

Regards,
Jihyun Bahn

  7. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    ZizhuanLiu X-MAN <44973863@qq.com> — 2026-06-13T13:33:46Z

    >To: ZizhuanLiu X-MAN <44973863(at)qq(dot)com>, cca5507 <cca5507(at)qq(dot)com>
>Cc: pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
>Subject: Re: Return value of XLogInsertRecord() for XLOG_SWITCH record
>Date: 2026-06-11 06:53:19
>Message-ID: 3CAC439F-E1F0-4F37-BE58-CC9D7CDE889C@gmail.com
>Views: Whole Thread | Raw Message | Download mbox | Resend email
>Lists: pgsql-hackers
>Hi ZizhuanLiu,
>
>
>
>Thanks for the caller survey — classifying the call sites by how they
>consume the return value is a useful way to frame the risk, and I agree
>pg_switch_wal() is the one path where the LSN escapes to external
>consumers.
>
>
>
>To help bound that risk, I re-ran my April comparison on a second
>platform (Windows 11, MSYS2/gcc 16.1.0, commit 9d141466ff, 19beta1),
>building unpatched master and v2 side by side and running the same
>sequence on identically-initialized clusters:
>
>
>
> before_switch: 0/017CF958 (both builds)
> switch_1: 0/017CF970 (both builds)
> after_1: 0/02000000 (both builds)
> switch_2/3: 0/02000000, no-op (both builds)
>
>
>
>The two builds returned byte-for-byte identical LSNs at every step,
>and "make check" passes on the patched build (All 245 tests passed).
>Incidentally this also confirms numerically that the MAXALIGN() change
>is a no-op: 0/017CF958 + 24 = 0/017CF970, since SizeOfXLogRecord is
>already 8-byte aligned.
>
>
>
>As far as I can tell, the only input where v2 can return a different
>value than current master is when the XLOG_SWITCH record ends exactly
>on a page boundary (StartPos at page offset XLOG_BLCKSZ - 24): old code
>takes the cross-page branch and adds a page header size even though no
>part of the record lies on the next page, while v2 returns the boundary
>itself. The v2 value is the one consistent with the end-pointer
>convention of XLogBytePosToEndRecPtr(). So any external tool that
>observed a different value in that rare alignment was depending on a
>value inconsistent with PostgreSQL's own end-pointer semantics — I'd
>read v2 as correcting that, rather than breaking compatibility.
>
>
>
>So in summary: identical SQL-visible behavior on all common paths
>(verified on Linux in April and Windows now), with the single divergent
>case being a consistency fix. If it would help, I could try to craft a
>reproduction of the exact page-boundary case (padding WAL position via
>pg_logical_emit_message), or write a small TAP test pinning down the
>boundary behavior of pg_switch_wal().
>
>
>
>Regards,
>Jihyun Bahn


Hi, 반지현

Thank you very much for your professional and comprehensive testing and analysis.
I’ve learned a great deal more about WAL internals through this discussion, including
details related to pg_logical_emit_message.

After further review and testing on my side, I fully agree with your reasoning.
Patch v2 corrects the inconsistent end-pointer calculation and does not break
compatibility with external tools.

Upon closer reflection, the original condition
```c
if (StartPos / XLOG_BLCKSZ != EndPos / XLOG_BLCKSZ)
```c
is intended to detect whether the XLOG_SWITCH record crosses a page boundary.
This branch then decides whether to add SizeOfXLogLongPHD (crossing WAL segment)
or SizeOfXLogShortPHD (crossing page only, same segment).

As we all know, EndPos points to the start of the next WAL record rather than the end
of the current one. The byte range occupied by any WAL record is [StartPos, EndPos).
Therefore, to correctly judge page crossing for the switch record, the condition should be adjusted to:
```c
if (StartPos / XLOG_BLCKSZ != (EndPos - 1) / XLOG_BLCKSZ)
```c
This expression accurately preserves the original design intent. I have prepared a v3 patch
based on v2, with only this single conditional expression modified.

After additional consideration: the helper functions that reserve space for WAL records
already compute the [StartPos, EndPos) range, including the *EndPos = XLogBytePosToEndRecPtr()
logic we discussed earlier. The XLOG_SWITCH record is a special case because it intentionally
consumes the remaining space in the current segment, forcing us to recalculate EndPos
a second time in this code block.

To avoid duplicate calculations, we can extend the parameters of the local function
ReserveXLogSwitch() by adding an output argument XLogRecPtr *actual_EndPos
to store the value computed via *EndPos = XLogBytePosToEndRecPtr(). With this change,
the logic here becomes cleaner: after inserted the XLOG_SWITCH WAL, we simply restore
EndPos to the precomputed actual_EndPos. All end-pointer calculations are unified in
one place — this is what v4 implements.

Both v3 and v4 compile successfully and pass tests covering 5 scenarios (including multiple corner cases).
The patch files and detailed test logs are attached for your review:
1.An XLOG_SWITCH record fits entirely within a WAL block with ample trailing free space;
2.An XLOG_SWITCH record exactly fills a WAL block with zero remaining free space;
3.Multiple consecutive calls to pg_switch_wal() do not trigger an actual WAL segment rotation;
4.The XLOG_SWITCH record crosses block boundaries within a single WAL segment;
5.The XLOG_SWITCH record crosses both block boundaries and WAL segment files.

Feel free to share your thoughts and point out any mistakes.

regards,
--
ZizhuanLiu (X-MAN) 
44973863@qq.com

  8. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    ZizhuanLiu X-MAN <44973863@qq.com> — 2026-06-14T14:37:58Z

    rebase v4.

Early v2/v3 only fixed the page-cross judgment condition, but still retained duplicated
EndPos calculation logic. v4 refactors ReserveXLogSwitch to compute standard EndPos 
once and reuse it, eliminating redundant code fundamentally.

All behavior remains identical to v2/v3 after fix; v4 just unifies the calculation
point without changing runtime logic.

	
ZizhuanLiu X-MAN
44973863@qq.com

  9. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    ZizhuanLiu X-MAN <44973863@qq.com> — 2026-06-15T01:17:20Z

    I am so sorry.  rebase v3 and v4.

regards,
--
ZizhuanLiu (X-MAN) 
44973863@qq.com


>Original
>From: ZizhuanLiu X-MAN <44973863@qq.com>
>Date: 2026-06-14 22:37
>To: rring0727 <rring0727@gmail.com>, cca5507 <cca5507@qq.com>
>Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
>Subject: Re: Return value of XLogInsertRecord() for XLOG_SWITCH record
>
>
>rebase v4.
>
>Early v2/v3 only fixed the page-cross judgment condition, but still retained duplicated
>EndPos calculation logic. v4 refactors ReserveXLogSwitch to compute standard EndPos 
>once and reuse it, eliminating redundant code fundamentally.
>
>All behavior remains identical to v2/v3 after fix; v4 just unifies the calculation
>point without changing runtime logic.
>
>
>ZizhuanLiu X-MAN
>44973863@qq.com

  10. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    ZizhuanLiu X-MAN <44973863@qq.com> — 2026-06-15T05:27:06Z

    Let us rebase v3 first .

regards,
--
ZizhuanLiu (X-MAN) 
44973863@qq.com


>Original
>From: ZizhuanLiu X-MAN <44973863@qq.com>
>Date: 2026-06-14 22:37
>To: rring0727 <rring0727@gmail.com>, cca5507 <cca5507@qq.com>
>Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
>Subject: Re: Return value of XLogInsertRecord() for XLOG_SWITCH record
>
>Early v2/v3 only fixed the page-cross judgment condition, but still retained duplicated
>EndPos calculation logic. v4 refactors ReserveXLogSwitch to compute standard EndPos 
>once and reuse it, eliminating redundant code fundamentally.
>
>All behavior remains identical to v2/v3 after fix; v4 just unifies the calculation
>point without changing runtime logic.
>
>
>ZizhuanLiu X-MAN
>44973863@qq.com

  11. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    ZizhuanLiu X-MAN <44973863@qq.com> — 2026-06-15T07:18:43Z

    Rebase v4 .

regards,
--
ZizhuanLiu (X-MAN) 
44973863@qq.com


>Original
>From: ZizhuanLiu X-MAN <44973863@qq.com>
>Date: 2026-06-14 22:37
>To: rring0727 <rring0727@gmail.com>, cca5507 <cca5507@qq.com>
>Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
>Subject: Re: Return value of XLogInsertRecord() for XLOG_SWITCH record
>
>Early v2/v3 only fixed the page-cross judgment condition, but still retained duplicated
>EndPos calculation logic. v4 refactors ReserveXLogSwitch to compute standard EndPos 
>once and reuse it, eliminating redundant code fundamentally.
>
>All behavior remains identical to v2/v3 after fix; v4 just unifies the calculation
>point without changing runtime logic.
>
>
>ZizhuanLiu X-MAN
>44973863@qq.com

  12. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    Henson Choi <assam258@gmail.com> — 2026-06-27T00:23:45Z

    Hi ZizhuanLiu,

> Rebase v4 .

Thanks for the rebase. I read through the v4 refactor of
ReserveXLogSwitch() -- computing the standard EndPos once and reusing it
reads much cleaner than the previous duplicated arithmetic. A few small,
behavior-neutral suggestions on the new out-parameter, in case you'd like
to fold them into the next version:

1) Naming. Of the three, this is the one I'd really like to see in the
next version. The parameter is spelled actual_EndPos, which is neither
snake_case nor CamelCase -- an underscore-separated lowercase prefix glued
to a CamelCase tail -- unlike its sibling out-parameters in the same
function (StartPos, EndPos, PrevPtr) that are plain CamelCase; and
"actual" doesn't say what it is actual relative to.
Since it carries the end of the record itself -- as opposed to EndPos,
which is padded out to the segment boundary -- I'd suggest RecEndPos,
which matches the surrounding names:

    static bool ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
                                  XLogRecPtr *PrevPtr, XLogRecPtr
*RecEndPos);

2) Drop the NULL guard on the out-parameter. There is a single caller and
it always passes &..., exactly like the other three out-parameters, which
are dereferenced unconditionally. Guarding only this one is inconsistent:

    /* Store the end position of just the record. */
    *RecEndPos = *EndPos;

To keep that unconditional store safe, the early "already at a segment
boundary" return should set it too:

    if (XLogSegmentOffset(ptr, wal_segment_size) == 0)
    {
        SpinLockRelease(&Insert->insertpos_lck);
        *EndPos = *StartPos = ptr;
        *RecEndPos = ptr;
        return false;
    }

3) With RecEndPos always populated, the `if (inserted)` guard around the
EndPos fixup in XLogInsertRecord() is no longer needed -- when nothing was
inserted RecEndPos already equals EndPos, so the assignment is a harmless
no-op:

    /*
     * Even though we reserved the rest of the segment for us, which is
     * reflected in EndPos, we return a pointer to just the end of the
     * xlog-switch record, which is consistent with other WAL types
     * returned by XLogBytePosToEndRecPtr().  When no switch record was
     * inserted, RecEndPos already equals EndPos, so this is a no-op.
     */
    EndPos = RecEndPos;

None of this changes behavior; it just localizes the invariant inside
ReserveXLogSwitch() and makes the out-parameters uniform. (2) and (3) are
take-it-or-leave-it -- the rename is the only one I feel strongly about.
Feel free to fold in whichever you find worthwhile for the next version.

Regards,
Henson

  13. Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

    ZizhuanLiu X-MAN <44973863@qq.com> — 2026-06-27T10:33:04Z

    >Original
>From: Henson Choi <assam258@gmail.com>
>Date: 2026-06-27 08:23
>To: ZizhuanLiu X-MAN <44973863@qq.com>
>Cc: rring0727 <rring0727@gmail.com>, cca5507 <cca5507@qq.com>, pgsql-hackers <pgsql-hackers@lists.postgresql.org>
>Subject: Re: Return value of XLogInsertRecord() for XLOG_SWITCH record
>
>Hi ZizhuanLiu,
>
>> Rebase v4 .
>
>Thanks for the rebase. I read through the v4 refactor of
>ReserveXLogSwitch() -- computing the standard EndPos once and reusing it
>reads much cleaner than the previous duplicated arithmetic. A few small,
>behavior-neutral suggestions on the new out-parameter, in case you'd like
>to fold them into the next version:
>
>1) Naming. Of the three, this is the one I'd really like to see in the
>next version. The parameter is spelled actual_EndPos, which is neither
>snake_case nor CamelCase -- an underscore-separated lowercase prefix glued
>to a CamelCase tail -- unlike its sibling out-parameters in the same
>function (StartPos, EndPos, PrevPtr) that are plain CamelCase; and
>"actual" doesn't say what it is actual relative to.
>Since it carries the end of the record itself -- as opposed to EndPos,
>which is padded out to the segment boundary -- I'd suggest RecEndPos,
>which matches the surrounding names:
>
>    static bool ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
>                                  XLogRecPtr *PrevPtr, XLogRecPtr *RecEndPos);
>
>2) Drop the NULL guard on the out-parameter. There is a single caller and
>it always passes &..., exactly like the other three out-parameters, which
>are dereferenced unconditionally. Guarding only this one is inconsistent:
>
>    /* Store the end position of just the record. */
>    *RecEndPos = *EndPos;
>
>To keep that unconditional store safe, the early "already at a segment
>boundary" return should set it too:
>
>    if (XLogSegmentOffset(ptr, wal_segment_size) == 0)
>    {
>        SpinLockRelease(&Insert->insertpos_lck);
>        *EndPos = *StartPos = ptr;
>        *RecEndPos = ptr;
>        return false;
>    }
>
>3) With RecEndPos always populated, the `if (inserted)` guard around the
>EndPos fixup in XLogInsertRecord() is no longer needed -- when nothing was
>inserted RecEndPos already equals EndPos, so the assignment is a harmless
>no-op:
>
>    /*
>     * Even though we reserved the rest of the segment for us, which is
>     * reflected in EndPos, we return a pointer to just the end of the
>     * xlog-switch record, which is consistent with other WAL types
>     * returned by XLogBytePosToEndRecPtr().  When no switch record was
>     * inserted, RecEndPos already equals EndPos, so this is a no-op.
>     */
>    EndPos = RecEndPos;
>
>None of this changes behavior; it just localizes the invariant inside
>ReserveXLogSwitch() and makes the out-parameters uniform. (2) and (3) are
>take-it-or-leave-it -- the rename is the only one I feel strongly about.
>Feel free to fold in whichever you find worthwhile for the next version.
>
>Regards,
>Henson



Hi, Henson

Thank you for your review and detailed suggestions.

Point 1) is a great catch; this was an oversight on my part.
I really appreciate the elegant code design proposed in points 2) and 3). 
Thank you very much for your detailed guidance.

I have unified the assignment logic inside `ReserveXLogSwitch()` with 
`*RecEndPos = *EndPos;` to keep the code consistent.

I have compiled and tested v5, and the results are as expected.

I would like to add some detailed clarification regarding the test case 
"4. Cross page boundaries within a single WAL segment file". After 
setting `Insert->CurrBytePos` via GDB, the correct derivation for the 
expected value of `EndPos` is as follows (v4 used 4 bytes, while v5 uses 8 bytes):
```
EndPos ==    (Current XLOG_BLCK)   StartPos + 8 (first 8 bytes of MAXALIGN(SizeOfXLogRecord))
                   + (Next      XLOG_BLCK)   24 (SizeOfXLogShortPHD)  + 16 (the remaining bytes of MAXALIGN(SizeOfXLogRecord))
```



regards,
--
ZizhuanLiu (X-MAN)
44973863@qq.com