Skip to content

fix: detect Postgres table catalog staleness#514

Open
hello-world-bfree wants to merge 1 commit into
duckdb:mainfrom
hello-world-bfree:fix/postgres-catalog-cache-staleness
Open

fix: detect Postgres table catalog staleness#514
hello-world-bfree wants to merge 1 commit into
duckdb:mainfrom
hello-world-bfree:fix/postgres-catalog-cache-staleness

Conversation

@hello-world-bfree

Copy link
Copy Markdown
Contributor

Resolves #512

Problem

The Postgres catalog cache, PostgresCatalogSet, loads table/schema metadata once and holds it for the lifetime of the process (or until pg_clear_cache() is called). There's no automatic way to notice when the underlying Postgres tables actually changed. If another connection, or even the same connection on a later statement, does some DDL there's nothing to tell the cache its in-memory view is misaligned. The next query against that table uses the stale column/type info until the cache is eventually cleared from a separate action.

This isn't often an issue in everyday duckdb-postgres but it's the contract that DuckLake depends on. DuckLake opens its own internal connection to the same attached Postgres database, a second ClientContext, not the user's, and expects catalog changes made through one path to become visible to the other without any manual cache management in-between. Right now that's not guaranteed. A ClientContext that primed its cache before another connection's DDL landed keeps serving the old shape indefinitely.

The cache should detect drift on its own by checking whether Postgres's own bookkeeping (pg_class.xmin, row identity) has moved since the last load, and only then pay the cost of reloading. That'll be correct without needing every caller to know when to invalidate.

Fix

PostgresTableSet is the only nested set that can independently reload. PostgresIndexSet and PostgresTypeSet can not. They throw without a pre-supplied batch-load slice. So PostgresTableSet gets a staleness query: (oid, relname, xmin) per table in the schema, string-compared against the signature captured at last load. Any add, drop, rename changes that set. A plain row-count check doesn't.

string PostgresTableSet::GetStalenessQuery() const {
        return "SELECT pg_class.oid, relname, pg_class.xmin FROM pg_class "
               "JOIN pg_namespace ON relnamespace = pg_namespace.oid "
               "WHERE relkind IN ('r','v','m','f','p') AND pg_namespace.nspname = $1 "
               "ORDER BY pg_class.oid";
}

The check runs once per TryLoadEntries call, inside the existing load_lock. It does not run per-entry, nor per-query. When there's a mismatch it clears and reloads exactly the way an explicit pg_clear_cache() already does.

A bug surfaced during implementation:

A connection's own writes; i.e., CREATE TABLE, DROP, etc., were not updating the signature, so the next access on that same connection saw its own committed write as drift, wiped the entry it just created, and reloaded from Postgres. So it lost anything DuckDB's AddColumn path doesn't reconstruct, default values probably being the most notable. Fixed by refreshing the signature from CreateTable, DropEntry, andReloadEntry directly, not from the generic bulk-load path.

That fix in turn exposed a second, narrower bug:

A same-connection ALTER TABLE inside an explicit BEGIN ... ROLLBACK. Postgres reverts xmin on rollback (correct MVCC behavior) which can make a mid-transaction signature match the pre-transaction one once the rollback lands. The cache then keeps serving state that was never actually committed. Fixed by staging the signature instead of writing it directly whenever a transaction is open, and only promoting it to the real signature on Commit(). Rollback() just discards the pending value:

void PostgresCatalogSet::RefreshStalenessSignature(PostgresTransaction &transaction) {
        auto signature = ComputeStalenessSignature(*transaction.Query(GetStalenessQuery()));
        if (transaction.GetTransactionState() == PostgresTransactionState::TRANSACTION_STARTED) {
                transaction.StageStalenessSignature(*this, std::move(signature));
                return;
        }
        staleness_signature = std::move(signature);
}

The pending value lives on PostgresTransaction as pending_signatures, mirroring the existing referenced_entries pattern, not on the catalog set itself since the cache is shared across every connection and a single pending slot would let one connection's commit or rollback clobber another's still-open transaction. Commit() promotes everything staged; Rollback() just drops it. Nothing in this path calls ClearEntries(), so it doesn't touch the reload machinery at all.

Note

PostgresIndexSet and PostgresTypeSet don't get automatic staleness detection with this PR. They can't reload standalone at all today (confirmed: LoadEntries throws InternalException without a pre-supplied result slice; Same for both). Fixing that is a much bigger lift. Scope here is only PostgresTableSet

Verified against a live Postgres 16 instance, full test/sql/storage/* suite (108 tests, 3873 assertions) run twice, zero regressions. New coverage: external DDL visibility across two ClientContexts, an identity-swap case that rules out a pure cardinality signal, and a two-connection staged-signature case confirming one connection's pending (uncommitted) signature can't be promoted or clobbered by a concurrent, unrelated connection's commit.

@staticlibs

Copy link
Copy Markdown
Member

Hi, thanks for the PR! I wonder if we can place the staleness_query behind an option? I would think there is a good chance the current query may not work with Postgres-wire-compatible DBs that otherwise work with duckdb-postgres.

I think something like this may be better,

  • option pg_staleness_query
  • default value: empty string, no staleness checks
  • special value: PG_CLASS_DEFAULT (or something similar) - will use the current query hardcoded in sources
  • custom query may be provided by a user

Perhaps it may be better to have 2 options - a flag and an actual value.

I would like to get this into 1.5.5, and my concern is to not break existing scenarios with non-Postgres servers.

@staticlibs

Copy link
Copy Markdown
Member

As a follow-up to previous comment, perhaps having it "off" by default is a bad idea, maybe tying it with pg_use_information_schema_introspection will be better:

  • option pg_staleness_query
  • current query by default
  • if pg_use_information_schema_introspection is set - and the pg_staleness_query is set to empty string
  • if pg_use_information_schema_introspection is reset - pg_staleness_query is reset to default

@hello-world-bfree

Copy link
Copy Markdown
Contributor Author

As a follow-up to previous comment, perhaps having it "off" by default is a bad idea, maybe tying it with pg_use_information_schema_introspection will be better:

  • option pg_staleness_query
  • current query by default
  • if pg_use_information_schema_introspection is set - and the pg_staleness_query is set to empty string
  • if pg_use_information_schema_introspection is reset - pg_staleness_query is reset to default

Thanks @staticlibs! Makes a ton of sense. Probably should've looked to pg_use_information_schema_introspection from the start.

The plain boolean is compelling:

  • new option pg_staleness_query_enabled
  • default = !pg_use_information_schema_introspection
  • reuses the existing ClearCacheOnSetting callback that pg_use_information_schema_introspection already uses, so flipping it at runtime clears the cache the exact same way
  • GetStalenessQuery returns "" when disabled; same as the base-class default) when disabled

wdyt?

@staticlibs

Copy link
Copy Markdown
Member

M, can we still make the query itself configurable? Assuming someone will be running DuckLake on Postgres-compatible DB and will want to avail of the staleness checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Catalog cache doesn't detect external Postgres DDL changes

2 participants