Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state - Mailing list pgsql-hackers

Hi,

Masahiko Sawada <sawada.mshk@gmail.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