Skip to content

fix(pretty_print): keep long identifiers as written - #901

Merged
psteinroe merged 11 commits into
mainfrom
fix/pretty-print-long-identifiers
Oct 26, 2026
Merged

psteinroe merged 11 commits into
mainfrom
fix/pretty-print-long-identifiers

Conversation

@psteinroe

Copy link
Copy Markdown
Collaborator

Stacked on #900.

Postgres keeps the first 63 bytes of an identifier (NAMEDATALEN - 1), and so does the AST the formatter works from. A longer name was therefore rewritten to its truncated form:

grant select on t to a_role_name_that_is_definitely_way_too_long_for_anything_postgres_keeps;
-- before
grant select on table t to a_role_name_that_is_definitely_way_too_long_for_anything_postgr;
-- after
grant select on table t to a_role_name_that_is_definitely_way_too_long_for_anything_postgres_keeps;

A quoted name could even end up cut in the middle, with a space before the closing quote. Postgres truncates both spellings the same way, so the meaning didn't change, but docs/features/formatting.md says the formatter doesn't change what is written.

What changed

format_statement now scans the statement for identifier tokens longer than 63 bytes, and maps the name the parser cut each one down to back to the name as written: dequoted for a quoted identifier, lower-cased for an unquoted one. The truncation clips to whole characters like truncate_identifier, so a multibyte character at the cut is handled. The identifier emitters (emit_identifier, emit_identifier_maybe_quoted, emit_type_identifier_maybe_quoted) look names up there and write the full name, quoted where needed as before. The AST check still passes, because the output parses back to the same truncated names.

A name is only restored when that is unambiguous. If two spellings are cut to the same 63 bytes, or a name is written both in full and already truncated, the AST name is kept as is.

Tests

  • Unit tests in identifiers.rs: unquoted and quoted names, a multibyte character at the cut, and the two ambiguous cases.
  • identifiers_longer_than_postgres_keeps_are_written_in_full in source_fidelity.rs covers a role, a quoted column and a table.

No snapshot changes. The snapshot harness drives the emitter directly without the source, and no fixture has an identifier this long.

Not included: deeply nested statements (finding P22)

About 40 nested CASEs, 20 nested subqueries or 45 levels of AND/OR still fail with "recursion limit reached". This limit is prost's decode limit, a constant 100 in prost 0.13. The only way to change it is the no-recursion-limit feature, which removes the limit for every prost user in the build. Without a limit, hostile input would hit stack overflows, which abort the process, in the decoder and in the recursive AST walkers (formatter, iter, normalisation) instead of returning an error. Doing that safely needs a bounded decoder or parsing on a thread with a larger stack. That isn't a small change, so I left it out of this PR.

@psteinroe
psteinroe changed the base branch from fix/pretty-print-grant-admin-option to main October 26, 2026 08:05
@psteinroe
psteinroe merged commit 0c42cd7 into main Oct 26, 2026
11 checks passed
@psteinroe
psteinroe deleted the fix/pretty-print-long-identifiers branch October 26, 2026 08:54
psteinroe added a commit that referenced this pull request Oct 26, 2026
Stacked on #901.

The snapshot tests render each fixture only with the options in its header, and the regression corpus test only runs the defaults. Bugs that need two options together got through both. Leading commas in fit layout (`select a , b`, fixed in #890) are one example. The release review found them by formatting everything under 288 option combinations. This PR lands a trimmed version of that harness, so the option matrix runs whenever the formatter changes.

**What the test does**

`tests/option_matrix.rs` formats two corpora under 32 configurations: every combination of `layout`, `clauseBodyStyle`, `logicalOperatorPlacement`, `commaStyle` and `valuesRowLayout`. Across those 32, indent (2 spaces, 4 spaces, tabs) and line width (40, 80, 120) cycle through all nine of their pairs. Upper-case keywords and constants come with width 120, `castStyle: operator` with tabs, and `isolateSemicolon` with width 40. The two corpora:

- every statement of the snapshot fixtures, without their headers;
- every statement Postgres 15–17 accepted in its regression suite.

Each output must:

- round-trip, i.e. `format_statement` doesn't refuse it as changing the AST;
- format to itself again;
- pass the whitespace check from #890: no space after `(` or before `)` and `,`, no double space, no trailing whitespace, no blank line;
- not mix indentation: no tab when indenting with spaces, no space before a tab when indenting with tabs. Lines inside function bodies keep their source indentation and are skipped.

Statements refused because a comment can't be placed (a comment after the terminator) are skipped; that refusal is by design.

Line width isn't checked. At width 40 there are known statements the formatter can't fit, mostly node types listed as not covered in #898. The snapshot tests already check width at 80 and 100.

**Where it runs**

Both corpora take about 35 seconds each in release on 8 cores. In debug, the fixtures alone take about three minutes. Adding that to `cargo test --workspace` would slow the Test job on all three platforms. The two tests are therefore `#[ignore]`d. A new `Formatter Option Matrix` job in `pull_request.yml` runs them once, in release on Ubuntu, when `pgls_pretty_print`, its codegen, `pgls_query`, the regression fixtures, `Cargo.lock` or the workflow change. I picked a path-filtered PR job over a nightly run so a regression fails the PR that introduces it. PRs that don't touch the formatter don't pay for it. A third test, not ignored, checks that the matrix covers all 32 combinations and all nine indent/width pairs.

On this stack, all 2.1 million formats pass. On `main`, the same test fails on the bugs fixed in #890–#901.

**New fixtures**

The review's test-gap list (P21) asked for these. VALUES with ORDER BY/LIMIT and short leading-comma lists came in #893 and #890:

- `aggregate_filter.sql`: `count(*) filter`, two filtered aggregates in one expression, `within group` + `filter`, and `filter` + `over`.
- `expanded_case_alignment.sql`: expanded CASE with `indentSize: 4` after `select` and after `on`, a broken WHEN condition, and a CASE nested in THEN.

**Not covered**

- The full 288-config matrix and the width check stay out. They are useful for a release review but too noisy and slow for CI.
- The snapshot width check still excuses lines whose longest token plus 20 columns fits. Tightening it would need a pass over every fixture, so I left it for later.

**Follow-ups:** The snapshot width check is tightened in #950.
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.

1 participant