Skip to content

fix(catalog): resolve the relations of the session's lock state with the catalog - #1042

Open
psteinroe wants to merge 3 commits into
refactor/lint-rule-catalog-lookupsfrom
fix/session-created-objects-resolve-names
Open

psteinroe wants to merge 3 commits into
refactor/lint-rule-catalog-lookupsfrom
fix/session-created-objects-resolve-names

Conversation

@psteinroe

@psteinroe psteinroe commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #1000.

The session's lock state (Locks) put every unqualified name into the first schema of the search path, both when it recorded a created table, an index or a NOT VALID constraint, and when a rule asked about one. Postgres does neither: it creates an unqualified table in the first schema of the search path that exists, and an unqualified name refers to the first relation with that name on the search path (after pg_temp and pg_catalog). lockTimeoutWarning didn't even use the search path; it looked unqualified tables up in public. With set search_path = app, public, it warned on an ALTER TABLE of a table created a statement before, and named it public.events although the table is app.events.

What changed

  • Catalog::relation_name_of(range_var, search_path): the schema and name of the relation a RangeVar resolves to with relation_of, or, if the catalog doesn't know one, where CREATE TABLE would create it (pg_temp for temporary tables, else the first existing schema of the search path).
  • The session resolves relation names with the catalog. Session::apply(stmt, &catalog) takes the catalog after the statement, which knows where the statement created its objects; the analyser and the workspace already apply the statement to the catalog first. has_created_object(schema, name) becomes has_created_relation(&RangeVar, &Catalog), and has_not_valid_constraint, is_dangerous_lock_stmt and access_exclusive_table_for_alter take the catalog too. normalized_schema is gone.
  • lockTimeoutWarning, requireStatementTimeout, requireIdleInTransactionTimeout, requireSeparateConstraintValidation and avoidWideLockWindow pass ctx.catalog(). lockTimeoutWarning names the table it resolved.

Without a database, the catalog only knows the relations of the file, so names of other tables resolve like before, to the first schema of the search path.

#970 adds Session::has_altered_table and object_schema with the same first-schema rule, and moves more rules to has_created_object. Whichever merges second needs to switch those to the catalog as well.

Tests

  • New spec lockTimeoutWarning/new_table_search_path.sql: tables created in app and in public with search_path = app, public, then altered and indexed unqualified (no finding), and app.users, which wasn't created (one finding). The old code reports three findings.
  • created_relations_are_resolved_with_the_catalog: a session with a database snapshot where app doesn't exist. CREATE TABLE made is recorded in public, an unqualified ALTER TABLE of a table created in public isn't a dangerous lock, and the access-exclusive table of ALTER TABLE users is public.users. The other session tests use a catalog without a database and are unchanged otherwise.
  • cargo test -p pgls_catalog --all-features, cargo test -p pgls_analyser (other snapshots unchanged), cargo test -p pgls_workspace, cargo clippy --workspace --all-targets -- -D warnings.

Update: The session compares the names of created relations and NOT VALID constraints exactly, like #970 does: the parser already folds unquoted names, so "Users" and users are different tables. Test: names_are_compared_exactly. When this PR and #970 meet, keep this PR's catalog-based resolution with the exact comparison, and switch #970's requireConcurrentIndexCreation and disallowUniqueConstraint to has_created_relation(relation, ctx.catalog()) and its has_altered_table to names resolved with relation_name_of.

The parser already folds unquoted names to lower case, so the session's
created relations and NOT VALID constraints must be compared exactly.
With a case-insensitive comparison, CREATE TABLE "Users" made the
existing table users look new, and the same with quoted schemas like
"App". #970 fixes the same comparison on its branch.
NodeEnum::CreateStmt(s) => {
if let Some(r) = &s.relation {
self.add_created_object(&r.schemaname, &r.relname, search_path);
self.created_objects.push(name_of(r));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a snapshot containing public.users(id bigint) and no app schema, this sequence reproduces the issue:

SET search_path = app, public;
CREATE TABLE IF NOT EXISTS users (id bigint);
ALTER TABLE public.users ADD COLUMN payload text;
CREATE INDEX users_id_idx ON public.users(id);

PostgreSQL skips the creation, and the catalog keeps the existing table. However, this push records public.users as created in the transaction. Both subsequent lockTimeoutWarning findings disappear, as do the statement, and idle-timeout findings. The base reports both statements.

The same issue affects a skipped CREATE INDEX IF NOT EXISTS when its name belongs to an existing table.

We should only register objects that the statement actually created. Maybe the catalog could expose the creation outcome so the session can distinguish a successful creation from an IF NOT EXISTS no-op.

Comment on lines +290 to +294
match self.relation_of(range_var, search_path).found() {
Some(relation) => (relation.schema, relation.name),
None if range_var.relpersistence == "t" => key("pg_temp", &range_var.relname),
None => self.creation_key(&names::range_var_name(range_var), search_path),
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a transaction, this sequence reproduces the issue:

CREATE TEMP TABLE events (id bigint);
CREATE TABLE events (id bigint);
ALTER TABLE public.events ADD COLUMN payload text;

PostgreSQL creates the second table in public, and the catalog contains both tables. However, relation_name_of resolves the second CREATE's RangeVar to pg_temp.events, so public.events is never recorded as newly created.

The following ALTER receives lock-, statement-, and idle-timeout findings at this head, whereas the base reports none. This reproduces without a database snapshot. CREATE TABLE AS has the same problem.

Maybe we could use the catalog's creation target when recording CREATE/CTAS effects, and reserve reference lookup for statements that operate on an existing relation.

match self.relation_of(range_var, search_path).found() {
Some(relation) => (relation.schema, relation.name),
None if range_var.relpersistence == "t" => key("pg_temp", &range_var.relname),
None => self.creation_key(&names::range_var_name(range_var), search_path),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With both schemas present and public.users(id bigint) in the snapshot:

SET search_path = app, public;
CREATE INDEX users_id_idx ON users(id);
ALTER INDEX users_id_idx SET (fillfactor = 70);

The session correctly records the new index as public.users_id_idx, and PostgreSQL confirms that schema. However, relation_of does not search the catalog's separate index map, so relation_name_of falls back to app.users_id_idx.

The ALTER is consequently classified as operating on an existing object by the statement- and idle-timeout checks: their finding count increases from one on the base to two here. The existing lockTimeoutWarning false positive also names app.users_id_idx, which does not exist.

Maybe we could resolve index names through the catalog's index lookup before falling back to a creation key, so recording and querying the new index use the same identity.

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants