Repository navigation
Conversation
…eated-objects-resolve-names
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)); |
There was a problem hiding this comment.
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.
| 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), | ||
| } |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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.
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 aNOT VALIDconstraint, 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 (afterpg_tempandpg_catalog).lockTimeoutWarningdidn't even use the search path; it looked unqualified tables up inpublic. Withset search_path = app, public, it warned on anALTER TABLEof a table created a statement before, and named itpublic.eventsalthough the table isapp.events.What changed
Catalog::relation_name_of(range_var, search_path): the schema and name of the relation aRangeVarresolves to withrelation_of, or, if the catalog doesn't know one, whereCREATE TABLEwould create it (pg_tempfor temporary tables, else the first existing schema of the search path).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)becomeshas_created_relation(&RangeVar, &Catalog), andhas_not_valid_constraint,is_dangerous_lock_stmtandaccess_exclusive_table_for_altertake the catalog too.normalized_schemais gone.lockTimeoutWarning,requireStatementTimeout,requireIdleInTransactionTimeout,requireSeparateConstraintValidationandavoidWideLockWindowpassctx.catalog().lockTimeoutWarningnames 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_tableandobject_schemawith the same first-schema rule, and moves more rules tohas_created_object. Whichever merges second needs to switch those to the catalog as well.Tests
lockTimeoutWarning/new_table_search_path.sql: tables created inappand inpublicwithsearch_path = app, public, then altered and indexed unqualified (no finding), andapp.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 whereappdoesn't exist.CREATE TABLE madeis recorded inpublic, an unqualifiedALTER TABLEof a table created inpublicisn't a dangerous lock, and the access-exclusive table ofALTER TABLE usersispublic.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 VALIDconstraints exactly, like #970 does: the parser already folds unquoted names, so"Users"andusersare 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'srequireConcurrentIndexCreationanddisallowUniqueConstrainttohas_created_relation(relation, ctx.catalog())and itshas_altered_tableto names resolved withrelation_name_of.