[psteinroe]

Stacked on #916.

A handful of lint rules matched the AST too loosely or resolved names on their own, which gave false positives and false negatives on ordinary migrations.

Column types

• banCharField only checked ADD COLUMN, so alter table users alter column code type char(10); went unreported. It now checks ALTER COLUMN ... TYPE like preferJsonb, preferTextField and preferTimestamptz, including the varchar fix.
• preferBigInt compared every part of the type name, lowercased, so myschema.int4, int4.counter and the quoted "integer" were flagged. It now uses the builtin_type_name helper that the other column type rules already use: the last name, unqualified or in pg_catalog, compared exactly (the parser already lowercases unquoted names and turns integer/smallint into int4/int2).
• preferIdentity flagged myschema.serial and serial.counter. Like Postgres' transformColumnDefinition, only an unqualified name (and not %TYPE) is a serial type now.
• changingColumnType treated any type whose last name was text as a safe change, e.g. type myschema.text, and also type text[]. Both are flagged now.

Tables created in the migration

requireConcurrentIndexCreation skipped an index when a CREATE TABLE with the same name was anywhere in the file, also after the index and in another schema:

create index users_email_idx on users (email); -- wasn't reported
create table users (id bigint, email text);

disallowUniqueConstraint only looked at earlier statements, but also ignored the schema. Both now ask the session (Session::has_created_object), like lockTimeoutWarning does, so only tables created earlier in the transaction count, in the schema the search path puts them in. CREATE TABLE AS counts too now.

Search path

lockTimeoutWarning and multipleAlterTable treated an unqualified table as public.<name>. With set search_path = app, create table users (...); alter table users ... reported a missing lock timeout for a table the migration had just created, and alter table authors ...; alter table app.authors ... wasn't reported as two ALTER TABLEs on the same table. The session now resolves the schema (Session::object_schema), and it records the tables that ALTER TABLE changed with the search path of that statement (Session::has_altered_table), so alter table authors ...; set search_path = app; alter table authors ... is no longer reported. Messages name the resolved schema, e.g. app.orders.

Dropped

requireConcurrentReindex already uses is_reindex_concurrent for the check on this base, so (concurrently false) is reported. The remaining check of the option in the fix is on purpose: an explicit CONCURRENTLY option in parentheses is left alone.

Tests

New spec files for each case above (banCharField/alter_column_type, preferBigInt/type_forms, preferIdentity/qualified_type_valid, changingColumnType/user_type, requireConcurrentIndexCreation/{table_created_later,table_in_other_schema,new_table_on_search_path}, a new disallowUniqueConstraint spec directory, lockTimeoutWarning/search_path, multipleAlterTable/{search_path,search_path_changed_valid}) and a session unit test. No existing snapshot changed.

Risks

requireConcurrentIndexCreation and disallowUniqueConstraint now report a table created before a COMMIT in the same file, since the session forgets created tables at the end of a transaction. This matches lockTimeoutWarning and Squawk. Unqualified names resolve to the first schema of the search path, where Postgres creates them; an ALTER TABLE of a table that exists in a later schema of the path is still attributed to the first one.

Update: #916 is merged, so main is merged in (merge commit, no rebase) and the diff now only contains this PR's own changes. Nothing here duplicated the catalog lookups that #1000 moves into pgls_catalog: the checks for tables created or altered earlier are transaction state, so they stay in the session. #1000 leaves requireConcurrentIndexCreation, disallowUniqueConstraint, lockTimeoutWarning and multipleAlterTable to this PR. Both PRs touch requireConcurrentIndexCreation, in different hunks (this one changes the created-table check, #1000 changes the partitioned-table check in the fix).

replace_keeping_comments (used by the type fixes of preferTextField, preferTimestamptz and banCharField) now keeps the line break after a -- comment from the replaced type. Before, name varchar -- length + newline + (10) NOT NULL became name text -- length NOT NULL, which parses but drops NOT NULL. New specs line_comment_in_type.sql for the three rules.

Update: Session::has_created_object and has_not_valid_constraint compare names exactly. The parser already folds unquoted names, so with the case-insensitive comparison CREATE TABLE "Users" made CREATE INDEX ... ON users and ALTER TABLE users ADD ... UNIQUE look like they target a new table, and the same with quoted schemas ("App" vs app). New specs quoted_table_name.sql for requireConcurrentIndexCreation and disallowUniqueConstraint, and the session test names_are_compared_exactly. #1042 gets the same fix. The --stdin-file-path exit-code issues from the review are in code from #916 on main and are fixed in #1082.

[edjubert]

replace_keeping_comments copies -- comments without preserving their terminating newline. This can silently comment out constraints following the replaced type.

For example, --fix --unsafe --only=preferTextField transforms:

CREATE TABLE t (name varchar -- length
(10) NOT NULL
);

into:

CREATE TABLE t (name text -- length NOT NULL
);

The result still parses, so keeps_statement_valid accepts it, but the column becomes nullable. The same issue affects preferTimestamptz.

[edjubert]

Switching these rules to Session::has_created_object introduces case-insensitive comparisons through eq_ignore_ascii_case. PostgreSQL already folds unquoted identifiers during parsing, so the resulting names must be compared exactly to preserve quoted identifiers.

Assuming public.users already exists:

CREATE TABLE "Users" (id bigint);
CREATE INDEX users_id_idx ON users (id);
ALTER TABLE users ADD CONSTRAINT users_id_key UNIQUE (id);

requireConcurrentIndexCreation and disallowUniqueConstraint now emit no diagnostics, although the operations target the existing table users, not the newly created "Users"

[edjubert]

I rechecked the latest changes, and this issue is still reproducible. Both rules still suppress the expected diagnostics when "Users" is created before an operation targeting the distinct table users. The same problem occurs with quoted schema names such as "App" versus app.

[edjubert]

The new stdin check bypasses check_database_connection, which the file-based path calls before running checks.

With an explicitly configured but unreachable database, checking this input through --stdin-file-path exits with code 0 and no diagnostic:

SELECT nonexistent_column FROM users;

Checking the same SQL from a file reports database/connection and exits with code 1.
The stdin path silently skips database-dependent checks, making an incomplete check appear successful.

[edjubert]

The new stdin path determines failure solely from diagnostics with severity Error and does not apply cli_options.error_on_warnings.

For example:

printf 'CREATE TABLE t (data json);\n' |
  postgres-language-server check \
    --disable-db \
    --only=preferJsonb \
    --error-on-warnings \
    --stdin-file-path=virtual.sql

This prints a preferJsonb warning but exits with code 0. Checking the same SQL from a file with the same options exits with code 1.