v2 attached. While running the v1 patches under CFBot (and
subsequently reproducing locally with CPPFLAGS=-DEXEC_BACKEND on
Linux), I discovered that v1 mis-handles processes that do not
inherit the postmaster's address space via fork(). v2 fixes this
and also addresses several unrelated items that I should have
caught in v1 self-review.
Changes in v2-0001:
* EXEC_BACKEND load-order fix. On platforms without fork()
(Windows, plus any build with CPPFLAGS=-DEXEC_BACKEND), each
child process re-runs process_shared_preload_libraries() in its
own fresh address space. In v1, extension _PG_init() could
reach register_sync_handler() before the child had called
InitSync(), so the first dynamic registration landed at ID 0,
colliding with SYNC_HANDLER_MD, and the idempotent guard in
InitSyncHandlers() ("if NSyncHandlers != 0 return") then
suppressed built-in registration entirely for the rest of that
process's lifetime. v2 calls InitSyncHandlers() at the top of
register_sync_handler() and replaces the counter-based guard
with an explicit builtin_sync_handlers_registered flag. The
v1 test that reported "got id=0" on Windows and FreeBSD CFBot
now reports id=5 under both fork and EXEC_BACKEND.
* Documentation. v1 shipped without SGML docs; v2 adds
doc/src/sgml/custom-sync-handler.sgml (modeled on
custom-rmgr.sgml) and registers it in filelist.sgml and
postgres.sgml. The doc build is clean.
* Error-message style. The four errmsg() strings that embedded
function names ("register_sync_handler: ...",
"test_sync_handler: ...") are reworded per the error-message
style guide. The two developer-bug paths that used
ERRCODE_NULL_VALUE_NOT_ALLOWED are changed to elog(FATAL, ...)
since they cannot be triggered from SQL.
* Stale comment. The v1 comment in
sync_handler_register_internal() claiming "fork() is full
POSIX barrier" was accurate only on fork-based platforms.
Rewritten to cover both fork and EXEC_BACKEND paths.
* SYNC_HANDLER_NONE enum value. Previously implicit 5 (after
MULTIXACT_MEMBER=4), changed in v1 to explicit -1 so that a
sentinel "no handler" value cannot be confused with a valid
index. I audited uses in master: the only references are
!= comparisons in slru.c at lines 1057, 1442, and 1558, which
are value-agnostic. Flagging explicitly here because it is
an ABI-visible enum value change.
Changes in v2-0002:
* Error-message style fixes in the test module to match the
core-side cleanups above.
* pgindent pass (required adding TshSharedState via
--list-of-typedefs since it is a test-module-local type).
Verification for v2:
* make check-world under autoconf on Linux, fork-based: all PASS
* make check-world under autoconf on Linux,
CPPFLAGS=-DEXEC_BACKEND: all PASS
* meson test under Linux with c_args='-DEXEC_BACKEND':
344 OK / 34 SKIP / 0 FAIL
* test_sync_handler/001_basic: 5/5 under all four combinations
* src/test/recovery: 597/597 under -DEXEC_BACKEND
* test_slru: 18/18 under -DEXEC_BACKEND (SLRU is the main user
of SYNC_HANDLER_NONE; the enum value change is safe)
* pgindent, pgperltidy (20230309), pgperlcritic: clean
* headerscheck on src/include/storage/sync.h in regular and
--cplusplus modes: clean
* doc/src/sgml builds cleanly; new chapter renders as expected
I apologize for the v1 verification gap. v1's "make check-world
green" claim was accurate on fork-based Linux only and did not
exercise EXEC_BACKEND; that is the single most important reason
the Windows failure slipped through. My pre-submission checklist
now includes the -DEXEC_BACKEND path.
Thanks,
Greg
