Skip to content

fix: support schema privilege diffs#287

Draft
dilame wants to merge 3 commits into
stripe:mainfrom
dilame:fix/schema-privileges
Draft

fix: support schema privilege diffs#287
dilame wants to merge 3 commits into
stripe:mainfrom
dilame:fix/schema-privileges

Conversation

@dilame

@dilame dilame commented May 25, 2026

Copy link
Copy Markdown

What

Plan and apply schema-level authorization drift:

  • schema privileges (GRANT / REVOKE on schemas), including WITH GRANT OPTION changes;
  • schema ownership (ALTER SCHEMA ... OWNER TO ...) when target owner differs;
  • CREATE SCHEMA ... AUTHORIZATION ... for newly materialized schemas so validation can faithfully rebuild non-default owners.

This adds schema ACLs and schema owners to the introspected NamedSchema model. Owner-granted USAGE / CREATE ACL entries are treated as implicit owner privileges rather than ordinary grant drift.

Why

pg-schema-diff already tracks table privileges, but schema authorization state was incomplete. Before this PR, a migration plan could converge to zero statements while the source and target still differed on schema grants.

After the first schema-privilege fix, one production-shaped no-op remained: schemas owned by a role can expose ACL rows like service=UC/service. PostgreSQL owners already have implicit schema privileges, so those owner grants should not create repeated no-op GRANT USAGE ON SCHEMA ... TO service noise.

A second production-shaped drift then surfaced: the tool read schema owner names only to suppress implicit owner grants, but did not generate ALTER OWNER. This PR now treats schema ownership as real schema state, so ownership drift is caught by the plan instead of requiring manual repair SQL.

How

  • internal/queries/queries.sql: add GetSchemaPrivileges from pg_namespace.nspacl; make GetSchemas return schema owner names.
  • internal/schema/schema.go: add SchemaPrivilege, attach sorted privileges to NamedSchema, and keep schema owner for ownership diffs plus implicit-owner filtering.
  • pkg/diff/sql_generator.go: diff schema privileges, recreate when WITH GRANT OPTION changes, and filter old privileges by old owner / target privileges by target owner.
  • pkg/diff/schema_privilege_sql_generator.go: emit GRANT ... ON SCHEMA / REVOKE ... ON SCHEMA with AUTHZ_UPDATE hazards.
  • pkg/diff/named_schema_sql_generator.go: emit owner-aware CREATE SCHEMA ... AUTHORIZATION ... and ALTER SCHEMA ... OWNER TO ... with AUTHZ_UPDATE hazard.
  • pkg/diff/plan_generator.go: clear schema privileges during validation for skipped privilege statements, matching table privilege handling.
  • internal/migration_acceptance_tests/named_schema_cases_test.go: cover grant, revoke, grant-option drift, non-owner CREATE revocation, owner-grant no-op, and explicit owner drift.

Test

PATH="/Applications/Postgres.app/Contents/Versions/16/bin:$PATH" go test ./internal/migration_acceptance_tests -run TestNamedSchemaTestCases -count=1
PATH="/Applications/Postgres.app/Contents/Versions/16/bin:$PATH" go test ./pkg/diff -count=1
PATH="/Applications/Postgres.app/Contents/Versions/16/bin:$PATH" go test ./internal/schema -count=1
PATH="/Applications/Postgres.app/Contents/Versions/16/bin:$PATH" go test ./internal/migration_acceptance_tests -run TestMaterializedViewTestCases/'Add materialized view with dependent column' -count=1

Also attempted broader runs:

PATH="/Applications/Postgres.app/Contents/Versions/16/bin:$PATH" go test ./...
PATH="/Applications/Postgres.app/Contents/Versions/16/bin:$PATH" go test $(go list ./... | grep -v '^github.com/stripe/pg-schema-diff$') -count=1
PATH="/Applications/Postgres.app/Contents/Versions/16/bin:$PATH" go test ./internal/migration_acceptance_tests -count=1

The root go test ./... is blocked in this local worktree by an untracked scratch test_diff.go at repository root. The broader acceptance package hit local temp-database statement-timeout noise in unrelated tests; the named-schema suite and the isolated materialized-view case passed when rerun directly.

dilame added 3 commits May 25, 2026 20:24
Schema-level grants were invisible to the schema model, so migrations could converge while a role was missing USAGE on a schema.

Constraint: pg-schema-diff skips privilege DDL during plan validation because temp roles are absent.

Rejected: Treat public schema default USAGE as implicit | would keep schema ACL introspection incomplete.

Confidence: high

Scope-risk: moderate

Directive: Keep schema privilege validation clearing in sync with every SkipValidation privilege generator.

Tested: PATH="/Applications/Postgres.app/Contents/Versions/16/bin:$PATH" go test ./...

Not-tested: Cross-version PostgreSQL matrix beyond local PostgreSQL 16.12.
Schema owners receive implicit USAGE and CREATE privileges, so ACL rows granted to the current owner should not be diffed as ordinary schema grants. Track schema owner during fetch and filter owner USAGE/CREATE while comparing schema privileges.

Constraint: PostgreSQL schema owners have implicit schema privileges, and pg-schema-diff does not generate schema ownership migrations.

Rejected: Treat owner-granted USAGE/CREATE as regular ACL drift | this repeats no-op GRANT/REVOKE statements when the desired schema grants the current owner.

Confidence: high

Scope-risk: narrow

Directive: Keep owner-aware privilege filtering local to schema privilege diffing until schema ownership migration is supported.

Tested: targeted named-schema acceptance; failed acceptance groups with -parallel 1; internal/schema; pkg/diff; pkg/tempdb; all non-acceptance cmd/internal/pkg packages.

Not-tested: Production migrate-plan against Cloud SQL; full go test ./... is blocked by untracked scratch test_diff.go in this worktree and parallel temp database statement-timeout noise in acceptance tests.
Schema owner is now treated as part of named-schema state rather than only as a helper for suppressing implicit owner grants. New schemas are rebuilt with AUTHORIZATION so plan validation can faithfully reconstruct non-default owners, and existing schemas emit ALTER OWNER when target ownership differs.

Constraint: declarative schema diffs must converge repeated migrate-plan runs without separate manual owner repair SQL.

Rejected: documenting ownership drift as a manual repair step | leaves pg-schema-diff blind to a catalog fact it already reads.

Confidence: high

Scope-risk: narrow

Directive: when changing owner-aware privilege filtering, filter old privileges against old owner and target privileges against target owner.

Tested: go test ./internal/migration_acceptance_tests -run TestNamedSchemaTestCases -count=1; go test ./pkg/diff -count=1; go test ./internal/schema -count=1; go test ./internal/migration_acceptance_tests -run TestMaterializedViewTestCases/'Add materialized view with dependent column' -count=1

Not-tested: full acceptance package passed far enough to validate many packages but hit unrelated temp database statement timeouts in this local Postgres environment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant