Subject: [PATCH RFC] Reject access to ALTER targets from table_rewrite triggers The catalog definitions of ALTER targets are already updated when table_rewrite event triggers run, but their tuples may still have the old layout. Inserting into such a table can create a new-layout tuple that the subsequent rewrite deforms with the old tuple descriptor, corrupting data. Reject relation opens for the command's work queue during these triggers. Check the entire queue to cover inherited alterations, and preserve the outer guard across nested rewrites and exceptions. Allow unrelated tables and direct system catalog queries. Document the restriction and add regression coverage. This is an initial conservative approach for discussion. It also blocks opens of already-rewritten targets and metadata helpers that open targets; the scope and placement of the guard merit review. Base-commit: 1a531f787f89446db51942a67ee37ba5924d521d --- diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml index c10627554bd..55ef1b8b122 100644 --- a/doc/src/sgml/event-trigger.sgml +++ b/doc/src/sgml/event-trigger.sgml @@ -159,6 +159,15 @@ pg_event_trigger_table_rewrite_reason() (see ). + + A table_rewrite trigger must not open relations being + altered by the command that fired it, including other relations affected + by an inherited alteration. Their catalog definitions have already been + updated, but their stored rows might still have the old layout. Accessing + these relations raises an error. The trigger can query the system catalogs + and access unrelated relations, for example to record the event in a log + table. + diff --git a/src/backend/access/common/relation.c b/src/backend/access/common/relation.c index 38b356b8239..e38042b173f 100644 --- a/src/backend/access/common/relation.c +++ b/src/backend/access/common/relation.c @@ -23,6 +23,7 @@ #include "access/relation.h" #include "access/xact.h" #include "catalog/namespace.h" +#include "commands/tablecmds.h" #include "pgstat.h" #include "storage/lmgr.h" #include "storage/lock.h" @@ -73,6 +74,8 @@ relation_open(Oid relationId, LOCKMODE lockmode) if (RelationUsesLocalBuffers(r)) MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPNAMESPACE; + CheckRelationNotInTableRewrite(r); + pgstat_init_relation(r); return r; @@ -123,6 +126,8 @@ try_relation_open(Oid relationId, LOCKMODE lockmode) if (RelationUsesLocalBuffers(r)) MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPNAMESPACE; + CheckRelationNotInTableRewrite(r); + pgstat_init_relation(r); return r; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index a1519297ec7..a4bcade97c2 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -212,6 +212,20 @@ typedef struct AlteredTableInfo List *changedStatisticsOwners; /* owners of same */ } AlteredTableInfo; +/* + * Relations that must not be opened by a table_rewrite event trigger. Catalog + * changes for the whole work queue are already visible, but the corresponding + * tuples have not necessarily been rewritten yet. Keep a stack so that a + * nested ALTER TABLE cannot hide the outer command's relations. + */ +typedef struct TableRewriteEventState +{ + List *wqueue; + struct TableRewriteEventState *previous; +} TableRewriteEventState; + +static TableRewriteEventState *table_rewrite_event_state = NULL; + /* Struct describing one new constraint to check in Phase 3 scan */ /* Note: new not-null constraints are handled elsewhere */ typedef struct NewConstraint @@ -5890,6 +5904,37 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, return newcmd; } +/* + * CheckRelationNotInTableRewrite + * + * Called when opening a relation, before its possibly inconsistent tuple + * descriptor can be used by a table_rewrite event trigger. Check the entire + * work queue, not just the relation for which the event is being fired: an + * inherited ALTER TABLE has already changed the catalogs for its children too. + */ +void +CheckRelationNotInTableRewrite(Relation rel) +{ + TableRewriteEventState *state; + + for (state = table_rewrite_event_state; state; state = state->previous) + { + ListCell *lc; + + foreach(lc, state->wqueue) + { + AlteredTableInfo *tab = lfirst(lc); + + if (tab->relid == RelationGetRelid(rel)) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_IN_USE), + errmsg("cannot access relation \"%s\" during a table_rewrite event trigger", + RelationGetRelationName(rel)), + errdetail("The relation is being altered by the command that fired the event trigger."))); + } + } +} + /* * ATRewriteTables: ALTER TABLE phase 3 */ @@ -6018,9 +6063,24 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, * And fire it only once. */ if (parsetree) - EventTriggerTableRewrite((Node *) parsetree, - tab->relid, - tab->rewrite); + { + TableRewriteEventState state; + + state.wqueue = *wqueue; + state.previous = table_rewrite_event_state; + table_rewrite_event_state = &state; + PG_TRY(); + { + EventTriggerTableRewrite((Node *) parsetree, + tab->relid, + tab->rewrite); + } + PG_FINALLY(); + { + table_rewrite_event_state = state.previous; + } + PG_END_TRY(); + } /* * Create transient table that will receive the modified data. diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index c3d8518cb62..53e8017173a 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -57,6 +57,7 @@ extern void AlterRelationNamespaceInternal(Relation classRel, Oid relOid, ObjectAddresses *objsMoved); extern void CheckTableNotInUse(Relation rel, const char *stmt); +extern void CheckRelationNotInTableRewrite(Relation rel); extern void ExecuteTruncate(TruncateStmt *stmt); extern void ExecuteTruncateGuts(List *explicit_rels, diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule index ba01fb8a450..91c03b5c607 100644 --- a/src/test/regress/parallel_schedule +++ b/src/test/regress/parallel_schedule @@ -134,6 +134,7 @@ test: compression compression_lz4 compression_pglz cluster # any test that runs DDL # oidjoins is read-only, though, and should run late for best coverage test: oidjoins event_trigger +test: event_trigger_rewrite # event_trigger_login cannot run concurrently with any other tests because diff --git a/src/test/regress/sql/event_trigger_rewrite.sql b/src/test/regress/sql/event_trigger_rewrite.sql new file mode 100644 index 00000000000..3744d41fb23 --- /dev/null +++ b/src/test/regress/sql/event_trigger_rewrite.sql @@ -0,0 +1,107 @@ +-- A table_rewrite trigger must not access a relation whose catalog definition +-- has changed but whose tuples have not yet been rewritten. +CREATE TABLE rewrite_target (a int, b text); +INSERT INTO rewrite_target VALUES (1, 'original'); +CREATE TABLE rewrite_log (relid oid, reason int); + +CREATE FUNCTION rewrite_access() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + EXECUTE current_setting('regress.rewrite_command'); +END; +$$; +CREATE EVENT TRIGGER rewrite_access ON table_rewrite + EXECUTE FUNCTION rewrite_access(); + +-- The original corruption case. The whole ALTER must roll back. +SET regress.rewrite_command = 'INSERT INTO rewrite_target VALUES (999, ''rw'')'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +SELECT a, b, pg_typeof(a) FROM rewrite_target; + +-- Reading the table is unsafe too. Exercise planning and cached plans. +SET regress.rewrite_command = 'SELECT b FROM rewrite_target'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +PREPARE rewrite_insert AS INSERT INTO rewrite_target VALUES (999, 'cached'); +SET regress.rewrite_command = 'EXECUTE rewrite_insert'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +DEALLOCATE rewrite_insert; + +SET regress.rewrite_command = 'UPDATE rewrite_target SET b = ''changed'''; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +SET regress.rewrite_command = 'DELETE FROM rewrite_target'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +-- Use server-side COPY so SPI's client-COPY restriction is not the guard. +SET regress.rewrite_command = 'COPY rewrite_target FROM ''nonexistent_rewrite_input'''; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +SET regress.rewrite_command = 'TRUNCATE rewrite_target'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +SELECT a, b, pg_typeof(a) FROM rewrite_target; + +-- Catalog inspection and writing an unrelated audit table remain allowed. +SET regress.rewrite_command = 'INSERT INTO rewrite_log SELECT oid, pg_event_trigger_table_rewrite_reason() FROM pg_class WHERE oid = pg_event_trigger_table_rewrite_oid()'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +SELECT a, b, pg_typeof(a) FROM rewrite_target; +SELECT relid::regclass, reason FROM rewrite_log; + +-- Protect other tables in the same work queue, including inheritance children. +CREATE TABLE rewrite_parent (a int, b text); +CREATE TABLE rewrite_child () INHERITS (rewrite_parent); +INSERT INTO rewrite_child VALUES (2, 'child'); +SET regress.rewrite_command = 'INSERT INTO rewrite_child VALUES (999, ''rw'')'; +ALTER TABLE rewrite_parent ALTER COLUMN a TYPE bigint; +SELECT a, b, pg_typeof(a) FROM rewrite_child; + +-- Nested rewrites must preserve the outer guard, including after an error +-- caught by a PL/pgSQL exception handler. The unrelated inner table is usable +-- again once its ALTER has finished. +CREATE TABLE rewrite_inner (a int); +CREATE OR REPLACE FUNCTION rewrite_access() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_parent'::regclass THEN + BEGIN + ALTER TABLE rewrite_inner ALTER COLUMN a TYPE bigint; + EXCEPTION WHEN object_in_use THEN + RAISE NOTICE 'nested access rejected'; + END; + -- The failed inner ALTER must have restored both its catalog and guard. + INSERT INTO rewrite_inner VALUES (7); + BEGIN + INSERT INTO rewrite_child VALUES (999, 'outer'); + EXCEPTION WHEN object_in_use THEN + RAISE NOTICE 'outer access still rejected'; + END; + ELSIF pg_event_trigger_table_rewrite_oid() = 'rewrite_inner'::regclass THEN + -- This is protected by the outer command, not the inner work queue. + INSERT INTO rewrite_child VALUES (999, 'nested'); + END IF; +END; +$$; +ALTER TABLE rewrite_parent ALTER COLUMN a TYPE bigint; +SELECT a, b, pg_typeof(a) FROM rewrite_child; +SELECT a, pg_typeof(a) FROM rewrite_inner; + +-- A successful nested rewrite also restores the previous guard. +CREATE OR REPLACE FUNCTION rewrite_access() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_parent'::regclass THEN + ALTER TABLE rewrite_inner ALTER COLUMN a TYPE bigint; + INSERT INTO rewrite_inner VALUES (8); + BEGIN + PERFORM b FROM rewrite_child; + EXCEPTION WHEN object_in_use THEN + RAISE NOTICE 'outer access still rejected after nested success'; + END; + END IF; +END; +$$; +ALTER TABLE rewrite_parent ALTER COLUMN a TYPE numeric; +SELECT a, b, pg_typeof(a) FROM rewrite_child; +SELECT a, pg_typeof(a) FROM rewrite_inner ORDER BY a; + +DROP EVENT TRIGGER rewrite_access; +DROP FUNCTION rewrite_access(); +DROP TABLE rewrite_child, rewrite_parent, rewrite_inner; +DROP TABLE rewrite_target, rewrite_log; +RESET regress.rewrite_command; diff --git a/src/test/regress/expected/event_trigger_rewrite.out b/src/test/regress/expected/event_trigger_rewrite.out new file mode 100644 index 00000000000..b5fb3caef68 --- /dev/null +++ b/src/test/regress/expected/event_trigger_rewrite.out @@ -0,0 +1,189 @@ +-- A table_rewrite trigger must not access a relation whose catalog definition +-- has changed but whose tuples have not yet been rewritten. +CREATE TABLE rewrite_target (a int, b text); +INSERT INTO rewrite_target VALUES (1, 'original'); +CREATE TABLE rewrite_log (relid oid, reason int); +CREATE FUNCTION rewrite_access() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + EXECUTE current_setting('regress.rewrite_command'); +END; +$$; +CREATE EVENT TRIGGER rewrite_access ON table_rewrite + EXECUTE FUNCTION rewrite_access(); +-- The original corruption case. The whole ALTER must roll back. +SET regress.rewrite_command = 'INSERT INTO rewrite_target VALUES (999, ''rw'')'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +ERROR: cannot access relation "rewrite_target" during a table_rewrite event trigger +LINE 1: INSERT INTO rewrite_target VALUES (999, 'rw') + ^ +DETAIL: The relation is being altered by the command that fired the event trigger. +QUERY: INSERT INTO rewrite_target VALUES (999, 'rw') +CONTEXT: PL/pgSQL function rewrite_access() line 3 at EXECUTE +SELECT a, b, pg_typeof(a) FROM rewrite_target; + a | b | pg_typeof +---+----------+----------- + 1 | original | integer +(1 row) + +-- Reading the table is unsafe too. Exercise planning and cached plans. +SET regress.rewrite_command = 'SELECT b FROM rewrite_target'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +ERROR: cannot access relation "rewrite_target" during a table_rewrite event trigger +LINE 1: SELECT b FROM rewrite_target + ^ +DETAIL: The relation is being altered by the command that fired the event trigger. +QUERY: SELECT b FROM rewrite_target +CONTEXT: PL/pgSQL function rewrite_access() line 3 at EXECUTE +PREPARE rewrite_insert AS INSERT INTO rewrite_target VALUES (999, 'cached'); +SET regress.rewrite_command = 'EXECUTE rewrite_insert'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +ERROR: cannot access relation "rewrite_target" during a table_rewrite event trigger +DETAIL: The relation is being altered by the command that fired the event trigger. +QUERY: EXECUTE rewrite_insert +CONTEXT: PL/pgSQL function rewrite_access() line 3 at EXECUTE +DEALLOCATE rewrite_insert; +SET regress.rewrite_command = 'UPDATE rewrite_target SET b = ''changed'''; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +ERROR: cannot access relation "rewrite_target" during a table_rewrite event trigger +LINE 1: UPDATE rewrite_target SET b = 'changed' + ^ +DETAIL: The relation is being altered by the command that fired the event trigger. +QUERY: UPDATE rewrite_target SET b = 'changed' +CONTEXT: PL/pgSQL function rewrite_access() line 3 at EXECUTE +SET regress.rewrite_command = 'DELETE FROM rewrite_target'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +ERROR: cannot access relation "rewrite_target" during a table_rewrite event trigger +LINE 1: DELETE FROM rewrite_target + ^ +DETAIL: The relation is being altered by the command that fired the event trigger. +QUERY: DELETE FROM rewrite_target +CONTEXT: PL/pgSQL function rewrite_access() line 3 at EXECUTE +-- Use server-side COPY so SPI's client-COPY restriction is not the guard. +SET regress.rewrite_command = 'COPY rewrite_target FROM ''nonexistent_rewrite_input'''; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +ERROR: cannot access relation "rewrite_target" during a table_rewrite event trigger +DETAIL: The relation is being altered by the command that fired the event trigger. +CONTEXT: SQL statement "COPY rewrite_target FROM 'nonexistent_rewrite_input'" +PL/pgSQL function rewrite_access() line 3 at EXECUTE +SET regress.rewrite_command = 'TRUNCATE rewrite_target'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +ERROR: cannot access relation "rewrite_target" during a table_rewrite event trigger +DETAIL: The relation is being altered by the command that fired the event trigger. +CONTEXT: SQL statement "TRUNCATE rewrite_target" +PL/pgSQL function rewrite_access() line 3 at EXECUTE +SELECT a, b, pg_typeof(a) FROM rewrite_target; + a | b | pg_typeof +---+----------+----------- + 1 | original | integer +(1 row) + +-- Catalog inspection and writing an unrelated audit table remain allowed. +SET regress.rewrite_command = 'INSERT INTO rewrite_log SELECT oid, pg_event_trigger_table_rewrite_reason() FROM pg_class WHERE oid = pg_event_trigger_table_rewrite_oid()'; +ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint; +SELECT a, b, pg_typeof(a) FROM rewrite_target; + a | b | pg_typeof +---+----------+----------- + 1 | original | bigint +(1 row) + +SELECT relid::regclass, reason FROM rewrite_log; + relid | reason +----------------+-------- + rewrite_target | 4 +(1 row) + +-- Protect other tables in the same work queue, including inheritance children. +CREATE TABLE rewrite_parent (a int, b text); +CREATE TABLE rewrite_child () INHERITS (rewrite_parent); +INSERT INTO rewrite_child VALUES (2, 'child'); +SET regress.rewrite_command = 'INSERT INTO rewrite_child VALUES (999, ''rw'')'; +ALTER TABLE rewrite_parent ALTER COLUMN a TYPE bigint; +ERROR: cannot access relation "rewrite_child" during a table_rewrite event trigger +LINE 1: INSERT INTO rewrite_child VALUES (999, 'rw') + ^ +DETAIL: The relation is being altered by the command that fired the event trigger. +QUERY: INSERT INTO rewrite_child VALUES (999, 'rw') +CONTEXT: PL/pgSQL function rewrite_access() line 3 at EXECUTE +SELECT a, b, pg_typeof(a) FROM rewrite_child; + a | b | pg_typeof +---+-------+----------- + 2 | child | integer +(1 row) + +-- Nested rewrites must preserve the outer guard, including after an error +-- caught by a PL/pgSQL exception handler. The unrelated inner table is usable +-- again once its ALTER has finished. +CREATE TABLE rewrite_inner (a int); +CREATE OR REPLACE FUNCTION rewrite_access() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_parent'::regclass THEN + BEGIN + ALTER TABLE rewrite_inner ALTER COLUMN a TYPE bigint; + EXCEPTION WHEN object_in_use THEN + RAISE NOTICE 'nested access rejected'; + END; + -- The failed inner ALTER must have restored both its catalog and guard. + INSERT INTO rewrite_inner VALUES (7); + BEGIN + INSERT INTO rewrite_child VALUES (999, 'outer'); + EXCEPTION WHEN object_in_use THEN + RAISE NOTICE 'outer access still rejected'; + END; + ELSIF pg_event_trigger_table_rewrite_oid() = 'rewrite_inner'::regclass THEN + -- This is protected by the outer command, not the inner work queue. + INSERT INTO rewrite_child VALUES (999, 'nested'); + END IF; +END; +$$; +ALTER TABLE rewrite_parent ALTER COLUMN a TYPE bigint; +NOTICE: nested access rejected +NOTICE: outer access still rejected +SELECT a, b, pg_typeof(a) FROM rewrite_child; + a | b | pg_typeof +---+-------+----------- + 2 | child | bigint +(1 row) + +SELECT a, pg_typeof(a) FROM rewrite_inner; + a | pg_typeof +---+----------- + 7 | integer +(1 row) + +-- A successful nested rewrite also restores the previous guard. +CREATE OR REPLACE FUNCTION rewrite_access() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_parent'::regclass THEN + ALTER TABLE rewrite_inner ALTER COLUMN a TYPE bigint; + INSERT INTO rewrite_inner VALUES (8); + BEGIN + PERFORM b FROM rewrite_child; + EXCEPTION WHEN object_in_use THEN + RAISE NOTICE 'outer access still rejected after nested success'; + END; + END IF; +END; +$$; +ALTER TABLE rewrite_parent ALTER COLUMN a TYPE numeric; +NOTICE: outer access still rejected after nested success +SELECT a, b, pg_typeof(a) FROM rewrite_child; + a | b | pg_typeof +---+-------+----------- + 2 | child | numeric +(1 row) + +SELECT a, pg_typeof(a) FROM rewrite_inner ORDER BY a; + a | pg_typeof +---+----------- + 7 | bigint + 8 | bigint +(2 rows) + +DROP EVENT TRIGGER rewrite_access; +DROP FUNCTION rewrite_access(); +DROP TABLE rewrite_child, rewrite_parent, rewrite_inner; +DROP TABLE rewrite_target, rewrite_log; +RESET regress.rewrite_command;