| From: | ZizhuanLiu X-MAN <44973863(at)qq(dot)com> |
|---|---|
| To: | cca5507 <cca5507(at)qq(dot)com>, 반지현 <rring0727(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Return value of XLogInsertRecord() for XLOG_SWITCH record |
| Date: | 2026-06-10 13:28:13 |
| Message-ID: | tencent_6ABF8DC7F0116D2AB44C13EEDF0E96E6D408@qq.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
>Original
>From: cca5507 <cca5507(at)qq(dot)com>
>Date: 2026-04-21 19:35
>To:반지현<rring0727(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)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(at)qq(dot)com
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Eisentraut | 2026-06-10 13:34:45 | Re: [PATCH] Fix compiler warnings by using designated initializers |
| Previous Message | Peter Eisentraut | 2026-06-10 13:25:30 | Re: Move system identifier generation to a common helper |