Subject: [PATCH v2 RFC] Register table rewrite targets once per ALTER queue
diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml
index c10627554bd..cdfeee7bd53 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
+ physically rewritten 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..12a862becb3 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/event_trigger.h"
#include "pgstat.h"
#include "storage/lmgr.h"
#include "storage/lock.h"
@@ -73,6 +74,9 @@ relation_open(Oid relationId, LOCKMODE lockmode)
if (RelationUsesLocalBuffers(r))
MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPNAMESPACE;
+ if (unlikely(in_table_rewrite_event))
+ EventTriggerCheckRelationAccess(r);
+
pgstat_init_relation(r);
return r;
@@ -123,6 +127,9 @@ try_relation_open(Oid relationId, LOCKMODE lockmode)
if (RelationUsesLocalBuffers(r))
MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPNAMESPACE;
+ if (unlikely(in_table_rewrite_event))
+ EventTriggerCheckRelationAccess(r);
+
pgstat_init_relation(r);
return r;
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index d868c7f42c3..326060602c3 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -53,6 +53,7 @@
#include "utils/evtcache.h"
#include "utils/fmgroids.h"
#include "utils/fmgrprotos.h"
+#include "utils/hsearch.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -60,6 +61,25 @@
#include "utils/syscache.h"
#include "utils/tuplestore.h"
+/*
+ * Registrations live until the owning complete query ends. Nested commands
+ * can register the same relation when the outer command is not firing a
+ * table_rewrite event; restore the previous registration when they finish.
+ */
+typedef struct TableRewriteRelation
+{
+ Oid relid;
+ struct EventTriggerQueryState *owner;
+ struct TableRewriteRelation *previous;
+ struct TableRewriteRelation *next;
+} TableRewriteRelation;
+
+typedef struct TableRewriteRelationEntry
+{
+ Oid relid;
+ TableRewriteRelation *registration;
+} TableRewriteRelationEntry;
+
typedef struct EventTriggerQueryState
{
/* memory context for this state's objects */
@@ -73,6 +93,7 @@ typedef struct EventTriggerQueryState
Oid table_rewrite_oid; /* InvalidOid, or set for table_rewrite
* event */
int table_rewrite_reason; /* AT_REWRITE reason */
+ TableRewriteRelation *rewrite_relations;
/* Support for command collection */
bool commandCollectionInhibited;
@@ -83,6 +104,10 @@ typedef struct EventTriggerQueryState
} EventTriggerQueryState;
static EventTriggerQueryState *currentEventTriggerState = NULL;
+static HTAB *table_rewrite_relations = NULL;
+
+/* Fast path for relation opens outside table_rewrite events. */
+bool in_table_rewrite_event = false;
/* GUC parameter */
bool event_triggers = true;
@@ -1007,12 +1032,15 @@ EventTriggerOnLogin(void)
/*
- * Fire table_rewrite triggers.
+ * Fire table_rewrite triggers. The caller initializes *registered to false
+ * for each ALTER work queue; subsequent events reuse its registrations.
*/
void
-EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason)
+EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason,
+ List *rewriteOids, bool *registered)
{
List *runlist;
+ bool was_in_table_rewrite_event = in_table_rewrite_event;
EventTriggerData trigdata;
/*
@@ -1039,6 +1067,48 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason)
if (runlist == NIL)
return;
+ /*
+ * Register each work queue once, only if a trigger will
+ * actually run. Keep the registrations across successive rewrite events;
+ * EventTriggerEndCompleteQuery() removes them on success or error.
+ */
+ if (!*registered)
+ {
+ ListCell *lc;
+
+ if (table_rewrite_relations == NULL)
+ {
+ HASHCTL ctl;
+
+ ctl.keysize = sizeof(Oid);
+ ctl.entrysize = sizeof(TableRewriteRelationEntry);
+ ctl.hcxt = TopMemoryContext;
+ table_rewrite_relations = hash_create("table rewrite relations", 32,
+ &ctl,
+ HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+ }
+
+ foreach(lc, rewriteOids)
+ {
+ TableRewriteRelation *registration;
+ TableRewriteRelationEntry *entry;
+ bool found;
+
+ registration = MemoryContextAlloc(currentEventTriggerState->cxt,
+ sizeof(TableRewriteRelation));
+ registration->relid = lfirst_oid(lc);
+ registration->owner = currentEventTriggerState;
+ entry = hash_search(table_rewrite_relations, ®istration->relid,
+ HASH_ENTER, &found);
+ /* No error can occur before linking the registration for cleanup. */
+ registration->previous = found ? entry->registration : NULL;
+ registration->next = currentEventTriggerState->rewrite_relations;
+ entry->registration = registration;
+ currentEventTriggerState->rewrite_relations = registration;
+ }
+ *registered = true;
+ }
+
/*
* Make sure pg_event_trigger_table_rewrite_oid only works when running
* these triggers. Use PG_TRY to ensure table_rewrite_oid is reset even
@@ -1048,6 +1118,7 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason)
*/
currentEventTriggerState->table_rewrite_oid = tableOid;
currentEventTriggerState->table_rewrite_reason = reason;
+ in_table_rewrite_event = true;
/* Run the triggers. */
PG_TRY();
@@ -1058,6 +1129,7 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason)
{
currentEventTriggerState->table_rewrite_oid = InvalidOid;
currentEventTriggerState->table_rewrite_reason = 0;
+ in_table_rewrite_event = was_in_table_rewrite_event;
}
PG_END_TRY();
@@ -1071,6 +1143,30 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason)
CommandCounterIncrement();
}
+/*
+ * A registered relation is unsafe to open only while its owning command is
+ * firing table_rewrite triggers. ALTER's own accesses between events remain
+ * allowed. An overlapping nested registration can only hide an inactive
+ * owner: accessing an active owner's target would already have been rejected
+ * before the nested ALTER could reach its rewrite events.
+ */
+void
+EventTriggerCheckRelationAccess(Relation rel)
+{
+ TableRewriteRelationEntry *entry;
+
+ Assert(in_table_rewrite_event);
+ entry = hash_search(table_rewrite_relations, &RelationGetRelid(rel),
+ HASH_FIND, NULL);
+ if (entry != NULL &&
+ OidIsValid(entry->registration->owner->table_rewrite_oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("cannot access relation \"%s\" during a table_rewrite event trigger",
+ RelationGetRelationName(rel)),
+ errdetail("The relation is affected by the command that fired the event trigger.")));
+}
+
/*
* Invoke each event trigger in a list of event triggers.
*/
@@ -1211,6 +1307,7 @@ EventTriggerBeginCompleteQuery(void)
slist_init(&(state->SQLDropList));
state->in_sql_drop = false;
state->table_rewrite_oid = InvalidOid;
+ state->rewrite_relations = NULL;
state->commandCollectionInhibited = currentEventTriggerState ?
currentEventTriggerState->commandCollectionInhibited : false;
@@ -1240,6 +1337,24 @@ EventTriggerEndCompleteQuery(void)
prevstate = currentEventTriggerState->previous;
+ /* Remove registrations before freeing the state they point to. */
+ while (currentEventTriggerState->rewrite_relations != NULL)
+ {
+ TableRewriteRelation *registration =
+ currentEventTriggerState->rewrite_relations;
+ TableRewriteRelationEntry *entry;
+
+ entry = hash_search(table_rewrite_relations, ®istration->relid,
+ HASH_FIND, NULL);
+ Assert(entry != NULL && entry->registration == registration);
+ if (registration->previous != NULL)
+ entry->registration = registration->previous;
+ else
+ hash_search(table_rewrite_relations, ®istration->relid,
+ HASH_REMOVE, NULL);
+ currentEventTriggerState->rewrite_relations = registration->next;
+ }
+
/* this avoids the need for retail pfree of SQLDropList items: */
MemoryContextDelete(currentEventTriggerState->cxt);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8dc70bfa0f1..0d1dc83f972 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5898,6 +5898,8 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
ListCell *ltab;
+ List *rewriteOids = NIL;
+ bool rewriteRegistered = false;
/* Go through each table that needs to be checked or rewritten */
foreach(ltab, *wqueue)
@@ -6018,9 +6020,32 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
* And fire it only once.
*/
if (parsetree)
+ {
+ /*
+ * Relations physically rewritten by this queue have new catalog
+ * definitions while their stored tuples still have the old
+ * layout.
+ */
+ if (rewriteOids == NIL)
+ {
+ ListCell *lc;
+
+ foreach(lc, *wqueue)
+ {
+ AlteredTableInfo *other = lfirst(lc);
+
+ if (RELKIND_HAS_STORAGE(other->relkind) &&
+ other->rewrite > 0 &&
+ other->relkind != RELKIND_SEQUENCE)
+ rewriteOids = lappend_oid(rewriteOids,
+ other->relid);
+ }
+ }
EventTriggerTableRewrite((Node *) parsetree,
tab->relid,
- tab->rewrite);
+ tab->rewrite, rewriteOids,
+ &rewriteRegistered);
+ }
/*
* Create transient table that will receive the modified data.
diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
index 27340655061..b5805c0ec42 100644
--- a/src/include/commands/event_trigger.h
+++ b/src/include/commands/event_trigger.h
@@ -20,6 +20,7 @@
#include "tcop/cmdtag.h"
#include "tcop/deparse_utility.h"
#include "utils/aclchk_internal.h"
+#include "utils/relcache.h"
typedef struct EventTriggerData
{
@@ -30,6 +31,9 @@ typedef struct EventTriggerData
} EventTriggerData;
extern PGDLLIMPORT bool event_triggers;
+extern PGDLLIMPORT bool in_table_rewrite_event;
+
+extern void EventTriggerCheckRelationAccess(Relation rel);
/*
* Reasons for relation rewrites.
@@ -61,7 +65,8 @@ extern bool EventTriggerSupportsObject(const ObjectAddress *object);
extern void EventTriggerDDLCommandStart(Node *parsetree);
extern void EventTriggerDDLCommandEnd(Node *parsetree);
extern void EventTriggerSQLDrop(Node *parsetree);
-extern void EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason);
+extern void EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason,
+ List *rewriteOids, bool *registered);
extern void EventTriggerOnLogin(void);
extern bool EventTriggerBeginCompleteQuery(void);
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 065f586310f..3438fc37e82 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -639,6 +639,84 @@ ERROR: cannot alter type "rewritetype" because column "rewritemetoo3.a" uses it
drop table rewriteme;
drop event trigger no_rewrite_allowed;
drop function test_evtrig_no_rewrite();
+-- table_rewrite triggers must not access tables with a partially updated layout.
+CREATE TABLE rewrite_target (a int, b text);
+INSERT INTO rewrite_target VALUES (1, 'original');
+CREATE FUNCTION rewrite_access() RETURNS event_trigger
+LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO rewrite_target VALUES (999, 'rw');
+END;
+$$;
+CREATE EVENT TRIGGER rewrite_access ON table_rewrite
+ EXECUTE FUNCTION rewrite_access();
+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 affected 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 SQL statement
+-- Both the data and the type must survive the rejected ALTER unchanged.
+SELECT a, b, pg_typeof(a) FROM rewrite_target;
+ a | b | pg_typeof
+---+----------+-----------
+ 1 | original | integer
+(1 row)
+
+-- Protect the whole work queue, not just the table firing the event. Check
+-- all three levels at each event, catching errors to let the rewrite finish.
+CREATE TABLE rewrite_child () INHERITS (rewrite_target);
+CREATE TABLE rewrite_grandchild () INHERITS (rewrite_child);
+INSERT INTO rewrite_grandchild VALUES (2, 'grandchild');
+CREATE TABLE rewrite_log (relid oid, reason int);
+CREATE OR REPLACE FUNCTION rewrite_access() RETURNS event_trigger
+LANGUAGE plpgsql AS $$
+DECLARE
+ relname text;
+BEGIN
+ FOREACH relname IN ARRAY ARRAY['rewrite_target', 'rewrite_child',
+ 'rewrite_grandchild'] LOOP
+ BEGIN
+ EXECUTE format('INSERT INTO %I VALUES (999, %L)', relname, 'rw');
+ EXCEPTION WHEN object_in_use THEN
+ RAISE NOTICE 'access to % rejected', relname;
+ END;
+ END LOOP;
+ -- Unrelated tables and catalog queries remain usable.
+ INSERT INTO rewrite_log
+ SELECT oid, pg_event_trigger_table_rewrite_reason()
+ FROM pg_class WHERE oid = pg_event_trigger_table_rewrite_oid();
+END;
+$$;
+ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint;
+NOTICE: access to rewrite_target rejected
+NOTICE: access to rewrite_child rejected
+NOTICE: access to rewrite_grandchild rejected
+NOTICE: access to rewrite_target rejected
+NOTICE: access to rewrite_child rejected
+NOTICE: access to rewrite_grandchild rejected
+NOTICE: access to rewrite_target rejected
+NOTICE: access to rewrite_child rejected
+NOTICE: access to rewrite_grandchild rejected
+SELECT a, b, pg_typeof(a) FROM rewrite_target ORDER BY a;
+ a | b | pg_typeof
+---+------------+-----------
+ 1 | original | bigint
+ 2 | grandchild | bigint
+(2 rows)
+
+SELECT relid::regclass, reason FROM rewrite_log ORDER BY relid::regclass::text;
+ relid | reason
+--------------------+--------
+ rewrite_child | 4
+ rewrite_grandchild | 4
+ rewrite_target | 4
+(3 rows)
+
+DROP EVENT TRIGGER rewrite_access;
+DROP FUNCTION rewrite_access();
+DROP TABLE rewrite_grandchild, rewrite_child, rewrite_target, rewrite_log;
-- Tests for REINDEX
CREATE OR REPLACE FUNCTION reindex_start_command()
RETURNS event_trigger AS $$
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index 32e9bb58c5e..104c353b2d2 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -476,6 +476,53 @@ drop table rewriteme;
drop event trigger no_rewrite_allowed;
drop function test_evtrig_no_rewrite();
+-- table_rewrite triggers must not access tables with a partially updated layout.
+CREATE TABLE rewrite_target (a int, b text);
+INSERT INTO rewrite_target VALUES (1, 'original');
+CREATE FUNCTION rewrite_access() RETURNS event_trigger
+LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO rewrite_target VALUES (999, 'rw');
+END;
+$$;
+CREATE EVENT TRIGGER rewrite_access ON table_rewrite
+ EXECUTE FUNCTION rewrite_access();
+ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint;
+-- Both the data and the type must survive the rejected ALTER unchanged.
+SELECT a, b, pg_typeof(a) FROM rewrite_target;
+
+-- Protect the whole work queue, not just the table firing the event. Check
+-- all three levels at each event, catching errors to let the rewrite finish.
+CREATE TABLE rewrite_child () INHERITS (rewrite_target);
+CREATE TABLE rewrite_grandchild () INHERITS (rewrite_child);
+INSERT INTO rewrite_grandchild VALUES (2, 'grandchild');
+CREATE TABLE rewrite_log (relid oid, reason int);
+CREATE OR REPLACE FUNCTION rewrite_access() RETURNS event_trigger
+LANGUAGE plpgsql AS $$
+DECLARE
+ relname text;
+BEGIN
+ FOREACH relname IN ARRAY ARRAY['rewrite_target', 'rewrite_child',
+ 'rewrite_grandchild'] LOOP
+ BEGIN
+ EXECUTE format('INSERT INTO %I VALUES (999, %L)', relname, 'rw');
+ EXCEPTION WHEN object_in_use THEN
+ RAISE NOTICE 'access to % rejected', relname;
+ END;
+ END LOOP;
+ -- Unrelated tables and catalog queries remain usable.
+ INSERT INTO rewrite_log
+ SELECT oid, pg_event_trigger_table_rewrite_reason()
+ FROM pg_class WHERE oid = pg_event_trigger_table_rewrite_oid();
+END;
+$$;
+ALTER TABLE rewrite_target ALTER COLUMN a TYPE bigint;
+SELECT a, b, pg_typeof(a) FROM rewrite_target ORDER BY a;
+SELECT relid::regclass, reason FROM rewrite_log ORDER BY relid::regclass::text;
+DROP EVENT TRIGGER rewrite_access;
+DROP FUNCTION rewrite_access();
+DROP TABLE rewrite_grandchild, rewrite_child, rewrite_target, rewrite_log;
+
-- Tests for REINDEX
CREATE OR REPLACE FUNCTION reindex_start_command()
RETURNS event_trigger AS $$