Skip to content

fix(migrations): Skip pg_dumpall restrict/unrestrict tokens in drift check#112777

Merged
vgrozdanic merged 1 commit intomasterfrom
vgrozdanic/fix/migration-drift-restrict-tokens
Apr 13, 2026
Merged

fix(migrations): Skip pg_dumpall restrict/unrestrict tokens in drift check#112777
vgrozdanic merged 1 commit intomasterfrom
vgrozdanic/fix/migration-drift-restrict-tokens

Conversation

@vgrozdanic
Copy link
Copy Markdown
Member

PostgreSQL 16+ added \restrict / \unrestrict directives to pg_dumpall output. These carry random per-session tokens for password scrambling and are not part of the database schema.

The migration drift CI job runs pg_dumpall twice (once after sequential migrations, once after squashed migrations) and compares the output. Since the tokens are regenerated on every dump, the comparison always reports a false-positive diff — causing the migrations-drift workflow to fail on every PR that touches migrations, in both sentry and getsentry.

This filters out \restrict and \unrestrict lines in the norm() function, alongside the existing filters for comments and django_migrations tables. The quick_drift_compare.py script delegates to the same compare module, so both code paths are fixed.

Example of the spurious diff this eliminates:

-\restrict chSRsrfaJ6ZjA7ck8dSaajbhnPiwOqTaC6ldxf7bN2kbn4VJZk4ODoz8l0XL2pk
+\restrict dXbwY4tgrWq0glKas9UkhhnydrGduwSqaYnQMvQj53fxjnB36DA9XvXg3TMMG9U

…check

PostgreSQL 16+ emits \restrict and \unrestrict directives with random
per-session tokens in pg_dumpall output. These tokens differ between
any two dumps and are unrelated to the database schema, causing the
migration drift detection to always report false positives.

Filter out these lines in the norm() function alongside the existing
filters for comments and django_migrations tables.

Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 13, 2026
@vgrozdanic vgrozdanic marked this pull request as ready for review April 13, 2026 10:45
@vgrozdanic vgrozdanic requested a review from a team as a code owner April 13, 2026 10:45
@vgrozdanic vgrozdanic requested review from a team April 13, 2026 10:45
continue
if last == "\n" and line == "\n":
continue
else:
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.

Bug: The script will crash when parsing a \connect statement that includes a role, due to an incorrect assumption about the number of arguments.
Severity: HIGH

Suggested Fix

Modify the \connect parsing logic to handle a variable number of arguments. For example, use parts = line.split() and then check len(parts) to correctly extract the database name, which is always the second element, while ignoring any subsequent parts like the role.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: tools/migrations/compare.py#L48

Potential issue: The parsing logic for `\connect` statements in
`tools/migrations/compare.py` incorrectly assumes that `line.split()` will always yield
two values. However, `pg_dumpall` can produce `\connect` statements with a role, such as
`\connect mydatabase myrole`, which results in three values. This will cause the script
to raise a `ValueError: too many values to unpack` when it encounters such a line,
crashing the migration drift CI job. This is a common output format in environments
where databases have specific owners.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not a part of this PR nor this change

@shellmayr
Copy link
Copy Markdown
Member

Maybe just for posterity - the restrict command in Postgres seems to have been added to mitigate a security issue - so for the sake of checking migrations this is irrelevant. Just did a little bit of research because I wanted to make sure we're not missing info if we remove this.

@vgrozdanic vgrozdanic merged commit 6d3af9d into master Apr 13, 2026
56 checks passed
@vgrozdanic vgrozdanic deleted the vgrozdanic/fix/migration-drift-restrict-tokens branch April 13, 2026 11:43
@trevor-e
Copy link
Copy Markdown
Member

Can we revert this? I think my fix here is the proper fix. #112731

@vgrozdanic
Copy link
Copy Markdown
Member Author

Can we revert this? I think my fix here is the proper fix. #112731

Of course! That's way better fix than mine, I'll revert my change

@vgrozdanic vgrozdanic added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Apr 13, 2026
@getsentry-bot
Copy link
Copy Markdown
Contributor

PR reverted: f6ed917

getsentry-bot added a commit that referenced this pull request Apr 13, 2026
…n drift check (#112777)"

This reverts commit 6d3af9d.

Co-authored-by: vgrozdanic <26199428+vgrozdanic@users.noreply.github.com>
wedamija pushed a commit that referenced this pull request Apr 13, 2026
…check (#112777)

PostgreSQL 16+ added `\restrict` / `\unrestrict` directives to
`pg_dumpall` output. These carry random per-session tokens for password
scrambling and are not part of the database schema.

The migration drift CI job runs `pg_dumpall` twice (once after
sequential migrations, once after squashed migrations) and compares the
output. Since the tokens are regenerated on every dump, the comparison
always reports a false-positive diff — causing the `migrations-drift`
workflow to fail on every PR that touches migrations, in both sentry and
getsentry.

This filters out `\restrict` and `\unrestrict` lines in the `norm()`
function, alongside the existing filters for comments and
`django_migrations` tables. The `quick_drift_compare.py` script
delegates to the same compare module, so both code paths are fixed.

Example of the spurious diff this eliminates:
```
-\restrict chSRsrfaJ6ZjA7ck8dSaajbhnPiwOqTaC6ldxf7bN2kbn4VJZk4ODoz8l0XL2pk
+\restrict dXbwY4tgrWq0glKas9UkhhnydrGduwSqaYnQMvQj53fxjnB36DA9XvXg3TMMG9U
```

Co-authored-by: Claude Opus 4 <noreply@anthropic.com>
wedamija pushed a commit that referenced this pull request Apr 13, 2026
…n drift check (#112777)"

This reverts commit 6d3af9d.

Co-authored-by: vgrozdanic <26199428+vgrozdanic@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert Add to a merged PR to revert it (skips CI)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants