feat(pg/review): add DISALLOW_DROP_CONSTRAINT, REQUIRE_PRIMARY_KEY, and PRIOR_BACKUP

pg/review/event_trigger_oracle_test.go

@@ -0,0 +1,186 @@
+package review
+
+import (
+	"context"
+	"slices"
+	"testing"
+)
+
+// TestEventTriggerEventsAgainstPostgres runs statements on PostgreSQL
+// under event triggers that log every event they see, and checks that
+// the scan expects each one: a statement that raised ddl_command_end is
+// DDL, one that raised sql_drop may drop, and one that raised
+// table_rewrite may rewrite. Claiming an event that does not come only
+// costs findings; missing one would let a trigger run unseen.
+func TestEventTriggerEventsAgainstPostgres(t *testing.T) {
+	db := startPostgres(t)
+	ctx := context.Background()
+	conn, err := db.Conn(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer conn.Close()
+	for _, setup := range []string{
+		`CREATE TABLE ev_log (event text, tag text)`,
+		`CREATE FUNCTION ev_note() RETURNS event_trigger LANGUAGE plpgsql AS $$ BEGIN INSERT INTO ev_log VALUES (lower(tg_event), tg_tag); END $$`,
+		`CREATE FOREIGN DATA WRAPPER ev_fdw`,
+		`CREATE SERVER ev_srv FOREIGN DATA WRAPPER ev_fdw`,
+		`CREATE EVENT TRIGGER ev_end ON ddl_command_end EXECUTE FUNCTION ev_note()`,
+		`CREATE EVENT TRIGGER ev_drop ON sql_drop EXECUTE FUNCTION ev_note()`,
+		`CREATE EVENT TRIGGER ev_rewrite ON table_rewrite EXECUTE FUNCTION ev_note()`,
+		`CREATE EVENT TRIGGER ev_login ON login EXECUTE FUNCTION ev_note()`,
+	} {
+		if _, err := conn.ExecContext(ctx, setup); err != nil {
+			t.Fatalf("%s: %v", setup, err)
+		}
+	}
+	// Each statement runs after the ones before it, and must succeed.
+	for _, sql := range []string{
+		"CREATE TABLE ev_p (id int PRIMARY KEY, code text, n int DEFAULT 1)",
+		"CREATE UNIQUE INDEX ev_p_code ON ev_p (code)",
+		"CREATE TABLE ev_t (id int PRIMARY KEY, p_id int, x int)",
+		"CREATE VIEW ev_v AS SELECT 1 AS a",
+		"ALTER TABLE ev_t ADD COLUMN a int",
+		"ALTER TABLE ev_t ADD COLUMN b int DEFAULT random()::int",
+		"ALTER TABLE ev_t ADD COLUMN c serial",
+		"ALTER TABLE ev_t ADD CONSTRAINT ev_fk FOREIGN KEY (p_id) REFERENCES ev_p",
+		"ALTER TABLE ev_t ADD CONSTRAINT ev_chk CHECK (x > 0)",
+		"CREATE UNIQUE INDEX ev_x ON ev_t (x)",
+		"ALTER TABLE ev_t ADD CONSTRAINT ev_x_key UNIQUE USING INDEX ev_x",
+		"ALTER TABLE ev_t ALTER COLUMN x SET NOT NULL",
+		"ALTER TABLE ev_t ALTER COLUMN x DROP NOT NULL",
+		"ALTER TABLE ev_t ALTER COLUMN x SET DEFAULT 2",
+		"ALTER TABLE ev_t ALTER COLUMN x SET DEFAULT 3, ADD COLUMN d int",
+		"ALTER TABLE ev_t ALTER COLUMN x DROP DEFAULT",
+		"ALTER TABLE ev_t ALTER COLUMN a TYPE bigint",
+		"ALTER TABLE ev_t ALTER COLUMN x SET STATISTICS 100",
+		"ALTER TABLE ev_t ADD COLUMN e int DEFAULT 5",
+		"CREATE DOMAIN ev_pos AS int CHECK (VALUE > 0)",
+		"ALTER TABLE ev_t ADD COLUMN f ev_pos",
+		"ALTER TABLE ev_t ADD COLUMN g int GENERATED ALWAYS AS (x * 2) STORED",
+		"ALTER TABLE ev_t ADD COLUMN h int GENERATED ALWAYS AS IDENTITY",
+		"ALTER TABLE ev_t ADD COLUMN i text NOT NULL, ADD COLUMN j varchar(10) COLLATE \"C\", ADD COLUMN k int[], ADD COLUMN l pg_catalog.int8",
+		"ALTER TABLE ev_t DISABLE TRIGGER ALL, ENABLE TRIGGER USER",
+		"ALTER TABLE ev_t SET UNLOGGED",
+		"ALTER TABLE ev_t SET LOGGED",
+		"ALTER TABLE ev_t OWNER TO CURRENT_USER",
+		"ALTER TABLE ev_t VALIDATE CONSTRAINT ev_chk",
+		"ALTER TABLE ev_t ENABLE ROW LEVEL SECURITY",
+		"ALTER TABLE ev_t DISABLE ROW LEVEL SECURITY",
+		"ALTER TABLE ev_t RENAME COLUMN a TO aa",
+		"ALTER TABLE ev_t RENAME TO ev_t2",
+		"CREATE SCHEMA ev_s",
+		"ALTER TABLE ev_t2 SET SCHEMA ev_s",
+		"ALTER SEQUENCE ev_s.ev_t_c_seq OWNED BY NONE",
+		"CREATE OR REPLACE VIEW ev_v AS SELECT 1 AS a, 2 AS b",
+		"COMMENT ON TABLE ev_p IS 'x'",
+		"GRANT SELECT ON ev_p TO PUBLIC",
+		"REVOKE SELECT ON ev_p FROM PUBLIC",
+		"CREATE TYPE ev_mood AS ENUM ('a')",
+		"ALTER TYPE ev_mood ADD VALUE 'b'",
+		"CREATE DOMAIN ev_d AS int",
+		"CREATE SEQUENCE ev_q",
+		"CREATE FUNCTION ev_f() RETURNS int LANGUAGE sql AS 'SELECT 1'",
+		"CREATE OR REPLACE FUNCTION ev_f() RETURNS int LANGUAGE sql AS 'SELECT 2'",
+		"CREATE PROCEDURE ev_pr() LANGUAGE sql AS 'SELECT 1'",
+		"DROP PROCEDURE ev_pr()",
+		"CREATE MATERIALIZED VIEW ev_mv AS SELECT 1 AS a",
+		"ALTER VIEW ev_v RENAME TO ev_v2",
+		"ALTER VIEW ev_v2 RENAME TO ev_v",
+		"ALTER INDEX ev_p_code RENAME TO ev_p_code2",
+		"CREATE FOREIGN TABLE ev_ft (a int) SERVER ev_srv",
+		"CREATE TABLE ev_c AS SELECT 1 AS a",
+		"TRUNCATE ev_c",
+		"SELECT 1 AS a INTO ev_c2",
+		"CREATE INDEX ON ev_p (n)",
+		"CREATE SCHEMA ev_s2 CREATE TABLE x (id int UNIQUE) CREATE VIEW y AS SELECT 1 AS a",
+		"INSERT INTO ev_p VALUES (1, 'a', 1)",
+		"UPDATE ev_p SET n = 2",
+		"SELECT * FROM ev_p",
+		"EXPLAIN SELECT * FROM ev_p",
+		"SET search_path = public",
+		"ALTER TABLE ev_s.ev_t2 DROP CONSTRAINT ev_chk",
+		"ALTER TABLE ev_s.ev_t2 DROP COLUMN aa",
+		"DROP DOMAIN ev_d",
+		"DROP VIEW ev_v",
+		"DROP TABLE ev_c, ev_c2",
+		"DELETE FROM ev_p",
+	} {
+		if _, err := conn.ExecContext(ctx, sql); err != nil {
+			t.Fatalf("%s: %v", sql, err)
+		}
+		rows, err := conn.QueryContext(ctx, `DELETE FROM ev_log RETURNING event, tag`)
+		if err != nil {
+			t.Fatal(err)
+		}
+		raised := make(map[string]bool)
+		var tags []string
+		for rows.Next() {
+			var event, tag string
+			if err := rows.Scan(&event, &tag); err != nil {
+				t.Fatal(err)
+			}
+			raised[event] = true
+			if event == "ddl_command_end" {
+				tags = append(tags, tag)
+			}
+		}
+		rows.Close()
+		stmts, finding := parse(sql, statementRanges(sql))
+		if finding != nil || len(stmts) != 1 {
+			t.Fatalf("%s: parse: %v", sql, finding)
+		}
+		n := stmts[0].node
+		// A tag the scan names is the server's.
+		if tag := commandTag(n); tag != "" && len(tags) > 0 && !slices.Contains(tags, tag) {
+			t.Errorf("%s: the scan names tag %q, the server %q", sql, tag, tags)
+		}
+		if raised["ddl_command_end"] && !isDDL(n) {
+			t.Errorf("%s raised ddl_command_end, which the scan does not expect", sql)
+		}
+		if raised["sql_drop"] && !mayDrop(n) {
+			t.Errorf("%s raised sql_drop, which the scan does not expect", sql)
+		}
+		// enabledTriggers sets login triggers aside.
+		if raised["login"] {
+			t.Errorf("%s raised login", sql)
+		}
+		if raised["table_rewrite"] && !mayRewrite(n) {
+			t.Errorf("%s raised table_rewrite, which the scan does not expect", sql)
+		}
+	}
+}
+
+// TestEventTriggerClassification pins the statements the scan expects
+// not to raise sql_drop or table_rewrite, which
+// TestEventTriggerEventsAgainstPostgres runs on the server.
+func TestEventTriggerClassification(t *testing.T) {
+	for _, c := range []struct {
+		sql             string
+		drops, rewrites bool
+	}{
+		{"ALTER TABLE ev_t ADD COLUMN a int", false, false},
+		{`ALTER TABLE ev_t ADD COLUMN i text NOT NULL, ADD COLUMN j varchar(10) COLLATE "C", ADD COLUMN k int[], ADD COLUMN l pg_catalog.int8`, false, false},
+		{"ALTER TABLE ev_t ADD CONSTRAINT ev_fk FOREIGN KEY (p_id) REFERENCES ev_p", false, false},
+		{"ALTER TABLE ev_t ALTER COLUMN x SET DEFAULT 3, ADD COLUMN d int", false, false},
+		{"ALTER TABLE ev_t ALTER COLUMN x DROP DEFAULT", true, false},
+		{"ALTER TABLE ev_s.ev_t2 DROP COLUMN aa", true, false},
+		{"ALTER TABLE ev_t ADD COLUMN e int DEFAULT 5", false, true},
+		{"ALTER TABLE ev_t ADD COLUMN f ev_pos", false, true},
+		{"ALTER TABLE ev_t ALTER COLUMN a TYPE bigint", true, true},
+		{"ALTER TABLE ev_t SET LOGGED", true, true},
+		{"ALTER TABLE ev_t RENAME TO ev_t2", false, false},
+		{"ALTER TABLE ev_t DISABLE TRIGGER ALL, ENABLE TRIGGER USER", false, false},
+	} {
+		stmts, finding := parse(c.sql, statementRanges(c.sql))
+		if finding != nil || len(stmts) != 1 {
+			t.Fatalf("%s: parse: %v", c.sql, finding)
+		}
+		if got := mayDrop(stmts[0].node); got != c.drops {
+			t.Errorf("mayDrop(%s) = %v, want %v", c.sql, got, c.drops)
+		}
+		if got := mayRewrite(stmts[0].node); got != c.rewrites {
+			t.Errorf("mayRewrite(%s) = %v, want %v", c.sql, got, c.rewrites)
+		}
+	}
+}

pg/review/review.go

@@ -2,10 +2,15 @@
 // engine side of the contract in github.com/bytebase/omni/review.
 //
 // Review parses the SQL once, evaluates the rules that depend on the SQL
-// alone once, and reports every finding against every target. A syntax
-// error ends the review: the result then holds that one finding and
-// nothing else, because no other rule can be trusted on text the engine
-// could not parse.
+// alone once, and reports every finding against every target. The rules
+// that read a target check the target's synced schema directly, without
+// rebuilding it, and a finding equal across targets is reported once,
+// listing them all. A syntax error ends the review: the result then holds
+// that one finding and nothing else, because no other rule can be trusted
+// on text the engine could not parse.
+//
+// A rule reports only what it can establish. Where the SQL and the
+// target's schema leave the answer open, the rule reports nothing.
 package review
 
 import (
@@ -26,6 +31,9 @@ var Rules = []review.Rule{
 	review.DisallowDropObject,
 	review.DisallowTruncate,
 	review.DisallowRename,
+	review.PriorBackup,
+	review.DisallowDropConstraint,
+	review.RequirePrimaryKey,
 }
 
 // Review evaluates opts.Rules over sql for every target. See the contract
@@ -55,6 +63,9 @@ func Review(ctx context.Context, sql string, opts review.Options, targets []revi
 	if on[review.OnlineMigration] {
 		checkOnlineMigration(opts.Change, r)
 	}
+	if on[review.PriorBackup] && opts.Change.PriorBackup {
+		checkPriorBackupSize(sql, opts.Change, r)
+	}
 	for i := range stmts {
 		if err := ctx.Err(); err != nil {
 			return nil, err
@@ -76,6 +87,14 @@ func Review(ctx context.Context, sql string, opts review.Options, targets []revi
 			checkDisallowRename(s, r)
 		}
 	}
+	if backup, scans := targetWork(stmts, on, opts.Change); backup || scans {
+		findings, failures, err := reviewTargets(ctx, stmts, on, backup, scans, targets)
+		if err != nil {
+			return nil, err
+		}
+		r.findings = append(r.findings, findings...)
+		result.Failures = failures
+	}
 	result.Findings = r.sorted()
 	return result, nil
 }

pg/review/review_test.go

@@ -155,6 +155,7 @@ func TestReview(t *testing.T) {
 		{
 			name:    "require is null looks at predicates only",
 			sql:     "SELECT a = NULL AS c, CASE WHEN b = NULL THEN 1 END, CASE d WHEN NULL THEN 1 END, count(*) FILTER (WHERE e = NULL), JSON_ARRAYAGG(x) FILTER (WHERE ee = NULL), (SELECT f = NULL FROM u WHERE g = NULL) FROM t JOIN u ON h = NULL WHERE i IN (SELECT j = NULL FROM v WHERE k = NULL) GROUP BY 1 HAVING l = NULL;\nCREATE TABLE n (x int CHECK (x <> NULL)); CREATE INDEX i ON t (a) WHERE m = NULL; INSERT INTO t VALUES (1) ON CONFLICT (a) WHERE o = NULL DO UPDATE SET a = 1 WHERE p = NULL; UPDATE t SET q = NULL WHERE r = NULL;\nCREATE TABLE d (a boolean DEFAULT (NULL = NULL), b boolean GENERATED ALWAYS AS (NULL = NULL) STORED); CREATE TRIGGER tg BEFORE UPDATE ON t FOR EACH ROW WHEN (NEW.s = NULL) EXECUTE FUNCTION f(); CREATE PUBLICATION pub FOR TABLE t WHERE (u = NULL);\nSELECT 1 WHERE (SELECT v = NULL FROM t LIMIT 1) AND NOT (SELECT w = NULL FROM t) OR (SELECT x = NULL FROM t) IS TRUE OR y = (SELECT z = NULL FROM t) OR EXISTS (SELECT aa = NULL FROM t) OR (SELECT bb = NULL FROM t WHERE cc = NULL) OR COALESCE((SELECT dd = NULL FROM t), false) OR (SELECT ee = NULL FROM t)::boolean;",
+			opts:    review.Options{Rules: []review.Rule{review.RequireIsNull}},
 			targets: 1,
 			want: func(t *testing.T, sql string) []finding {
 				var out []finding