On 2017/05/14 12:03, Mark Dilger wrote:
> Hackers,
>
> I discovered a reproducible crash using event triggers in the current
> development version, 29c7d5e4844443acaa74a0d06dd6c70b320bb315.
> I was getting a crash before this version, and cloned a fresh copy of
> the sources to be sure I was up to date, so I don't think the bug can be
> attributed to Andres' commit. (The prior version I was testing against
> was heavily modified by me, so I recreated the bug using the latest
> standard, unmodified sources.)
>
> I create both before and after event triggers early in the regression test
> schedule, which then fire here and there during the following tests, leading
> fairly reproducibly to the server crashing somewhere during the test suite.
> These crashes do not happen for me without the event triggers being added
> to the tests. Many tests show as 'FAILED' simply because the logging
> that happens in the event triggers creates unexpected output for the test.
> Those "failures" are expected. The server crashes are not.
>
> The server logs suggest the crashes might be related to partitioned tables.
>
> Please find attached the patch that includes my changes to the sources
> for recreating this bug. The logs and regression.diffs are a bit large; let
> me know if you need them.
>
> I built using the command
>
> ./configure --enable-cassert --enable-tap-tests && make -j4 && make check
Thanks for the report and providing steps to reproduce.
It seems that it is indeed a bug related to creating range-partitioned
tables. DefineRelation() calls AlterTableInternal() to add NOT NULL
constraints on the range partition key columns, but the code fails to
first initialize the event trigger context information. Attached patch
should fix that.
Thanks to the above test case, I also discovered that in the case of
creating a partition, manipulations performed by MergeAttributes() on the
input schema list may cause it to become invalid, that is, the List
metadata (length) will no longer match the reality, because while the
ListCells are deleted from the input list, the List pointer passed to
list_delete_cell does not point to the same list. This caused a crash
when the CreateStmt in question was subsequently passed to copyObject,
which tried to access CreateStmt.tableElts that has become invalid as just
described. The attached patch also takes care of that.
Thanks,
Amit
>From 43f29ac13abda3c6ad71143d462c2dcd2c42f521 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 15 May 2017 14:00:54 +0900
Subject: [PATCH] Fix to make partitioned tables work with event triggers
There were a couple of bugs here:
1. When creating range partitioned tables, AlterTableInternal is
called which invokes the event trigger code. We must perform
EventTriggerAlterTableStart() before calling AlterTableInternal
so that the event trigger context information is initialized at
all.
2. When creating a partition, CreateStmt.tableElts is likely to
become invalid upon return from MergeAttributes(). We must replace
the invalid value with the newly constructed correct list value
computed by MergeAttributes(). For example, in the caller
ProcessUtilitySlow(), EventTriggerCollectSimpleCommand() is called
right after returning from DefineRelation(), and it expects the
the content of CreateStmt to be valid to perform copyObject on it.
First bug was reported by Mark Dilger.
Report: https://postgr.es/m/4A9B8269-9771-4FB7-BFA3-84249800917D%40gmail.com
---
src/backend/commands/tablecmds.c | 17 ++++++++++++++
src/test/regress/expected/event_trigger.out | 32 ++++++++++++++++++++++++++
src/test/regress/sql/event_trigger.sql | 35 +++++++++++++++++++++++++++++
3 files changed, 84 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b94db89b25..c3f489308a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -620,6 +620,19 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
stmt->relation->relpersistence,
stmt->partbound != NULL,
&inheritOids, &old_constraints, &parentOidCount);
+ /*
+ * The original value of stmt->tableElts may have been obsoleted by
+ * MergeAttributes(), especially those for partitions, wherein the
+ * cells of stmt->tableElts are deleted. Note that in a partition's
+ * case, stmt->tableElts contains dummy ColumnDef's created to capture
+ * the partition's own column constraints which are merged into the actual
+ * column definitions derived from the parent, before deleting the
+ * aforementioned dummy ones. We don't need to refer to stmt->tableElts
+ * hereafter in this function, although the caller expects it to contain
+ * valid elements upon return. So replace the original list with the
+ * one that's known to be valid.
+ */
+ stmt->tableElts = schema;
/*
* Create a tuple descriptor from the relation schema. Note that this
@@ -853,7 +866,11 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* pass true for recurse; ATPrepSetNotNull() complains if we don't
*/
if (cmds != NIL)
+ {
+ EventTriggerAlterTableStart((Node *) stmt);
AlterTableInternal(RelationGetRelid(rel), cmds, true);
+ EventTriggerAlterTableEnd();
+ }
}
}
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 906dcb8b31..d5021b648e 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -454,3 +454,35 @@ NOTICE: DROP POLICY - ddl_command_end
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
+-- check events triggers work correctly in the case of creating partitioned
+-- tables, especially range-partitioned ones which invoke
+-- AlterTableInternal().
+CREATE OR REPLACE FUNCTION create_partitioned_table_before_proc() RETURNS
+event_trigger AS $$
+BEGIN
+ RAISE WARNING 'preparing to create table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+CREATE EVENT TRIGGER create_partitioned_table_before_trigger
+ ON ddl_command_start
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_before_proc();
+CREATE OR REPLACE FUNCTION create_partitioned_table_after_proc()
+RETURNS event_trigger AS $$
+BEGIN
+ RAISE WARNING 'finished creating table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+CREATE EVENT TRIGGER create_partitioned_table_after_trigger
+ ON ddl_command_end
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_after_proc();
+-- create a range-partitioned table
+CREATE TABLE range_partitioned (a int) PARTITION BY RANGE (a);
+WARNING: preparing to create table: ddl_command_start CREATE TABLE
+WARNING: finished creating table: ddl_command_end CREATE TABLE
+DROP TABLE range_partitioned;
+DROP EVENT TRIGGER create_partitioned_table_before_trigger;
+DROP EVENT TRIGGER create_partitioned_table_after_trigger;
+DROP FUNCTION create_partitioned_table_before_proc();
+DROP FUNCTION create_partitioned_table_after_proc();
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index b65bf3ec66..1491fd7f07 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -375,3 +375,38 @@ DROP POLICY p2 ON event_trigger_test;
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
+
+-- check events triggers work correctly in the case of creating partitioned
+-- tables, especially range-partitioned ones which invoke
+-- AlterTableInternal().
+CREATE OR REPLACE FUNCTION create_partitioned_table_before_proc() RETURNS
+event_trigger AS $$
+BEGIN
+ RAISE WARNING 'preparing to create table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE EVENT TRIGGER create_partitioned_table_before_trigger
+ ON ddl_command_start
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_before_proc();
+
+CREATE OR REPLACE FUNCTION create_partitioned_table_after_proc()
+RETURNS event_trigger AS $$
+BEGIN
+ RAISE WARNING 'finished creating table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE EVENT TRIGGER create_partitioned_table_after_trigger
+ ON ddl_command_end
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_after_proc();
+
+-- create a range-partitioned table
+CREATE TABLE range_partitioned (a int) PARTITION BY RANGE (a);
+DROP TABLE range_partitioned;
+DROP EVENT TRIGGER create_partitioned_table_before_trigger;
+DROP EVENT TRIGGER create_partitioned_table_after_trigger;
+DROP FUNCTION create_partitioned_table_before_proc();
+DROP FUNCTION create_partitioned_table_after_proc();
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
