Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state

Hi,

Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote (in August 2024):
> My question is;
> in order to track just catalog-change transactions, whether it's
> sufficient to check if XLOG_XACT_COMMIT[_PREPARED] has the
> XACT_XINFO_HAS_INVALS flag. If yes, we probably should change only
> xact_decode() to check the commit records even in BUILDING_SNAPSHOT.
> Otherwise, we would need to change mostly all paths where we mark the
> transaction as catalog-change as the patch does.

I tried to answer that with data, and it looks like the answer is no.

I added two LOG lines to master (the attached instrumentation diff, not
meant to be applied): SnapBuildCommitTxn() logs, for the xid and each
subxid, whether the reorder buffer has it as catalog-changing and
whether the commit has XACT_XINFO_HAS_INVALS; SnapBuildProcessNewCid()
logs which relation the NEW_CID is for. Then I ran installcheck-parallel
with a logical slot created beforehand, and decoded all of it. The
attached catchk_run.sh does all of this on a build with that diff,
including the user catalog table check below; three runs gave the same
counts.

catalog changes invals in commit top-level xacts
no no 5087
yes no 37
yes yes 15540

The 37 are two kinds. 33 changed only pg_largeobject and
pg_largeobject_metadata. In the other 4 the catalog changes were made
only in subtransactions that were rolled back (TRUNCATE and DROP TABLE
in stats.sql, and one pg_statistic write), so the top transaction was
marked through its child but had nothing left to invalidate. In that
run, every committed change to a catalog with a syscache or relcache
had invalidations.

But the regression tests don't write to user catalog tables, and those
behave like large objects:

CREATE TABLE uc (k int PRIMARY KEY, v text)
WITH (user_catalog_table = true);

INSERT INTO uc ... catalog changes: yes invals: no
UPDATE uc ... catalog changes: yes invals: no
DELETE FROM uc ... catalog changes: yes invals: no
ALTER TABLE plain ... catalog changes: yes invals: yes

Output plugins may read user catalog tables with the historic snapshot,
so a transaction that writes one during BUILDING_SNAPSHOT has to be
tracked, and XACT_XINFO_HAS_INVALS would miss it. So I think the
patch's approach, marking the transaction from the NEW_CID records as
well, is the one needed.

The same data says something about master that I can't turn into a
failure: SnapBuildXidHasCatalogChanges() relies on "The transactions
that have changed catalogs must have invalidation info" to skip the
catchange array lookup, which doesn't hold for user catalog tables or
large objects. Is that a problem when a snapshot is restored, for a
plugin that reads user catalog tables?

On the patch itself, v6 applied on master e8a3ee5b197, against that
master, with --enable-cassert:

- snapshot_build fails on master with "could not map filenumber
"base/16384/16772" to relation OID" and passes with v6.
- The same test with the catalog change in a subtransaction, and with
it committed by COMMIT PREPARED, fails the same way on master and
passes with v6.
- With ALTER TABLE ... ADD COLUMN on an existing table instead of
CREATE TABLE, master raises no error: the insert that follows is
decoded with the old tuple descriptor, and the new columns are
silently dropped:

master: table public.tbl3: INSERT: val1[integer]:1
v6: table public.tbl3: INSERT: val1[integer]:1 val2[text]:'two' val3[bigint]:3

That seems worse than the error, since nothing tells the user.

The attached diff, on top of v6-0002, adds these three as permutations
of snapshot_build. They fail on master and pass with v6.

About Ajin's question in March 2025 on the DecodeTXNNeedSkip() change
(the "SnapBuildCurrentState(...) < SNAPBUILD_CONSISTENT" test), which
ChangAo said might be redundant: on v6 I logged every call made before
CONSISTENT, and whether SnapBuildXactNeedsSkip() already skipped it.
Over test_decoding, src/test/subscription and the recovery tests below
plus 040, it was called 41 times before CONSISTENT (28 in
BUILDING_SNAPSHOT, 13 in FULL_SNAPSHOT), and SnapBuildXactNeedsSkip()
had already skipped every one of them. So it never changes the result
there. Maybe an Assert would say the same thing and catch a case where
it does.

With v6: test_decoding (20 and 15, plus the new permutations),
src/test/subscription (605 tests), the logical decoding tests in
src/test/recovery (006, 035 and 038, 119 tests) and make check (239)
pass.

v6-0001 applies cleanly to master and REL_18_STABLE; REL_19_STABLE
conflicts in DecodeTXNNeedSkip(), and REL_17_STABLE and older in the
XLOG_XACT_INVALIDATIONS case of xact_decode(). As Haiyang reported in
bug #19109, it reproduces back to 11.

I haven't looked at Ajin's alternative beyond your question about it.

Regards,
Manu