Fix dropped object handling in pg_event_trigger_ddl_commands

Started by Sven Klemmover 4 years ago11 messages
#1Sven Klemm
sven@timescale.com
1 attachment(s)

Hello,

when creating an event trigger for ddl_command_end that calls
pg_event_trigger_ddl_commands certain statements will cause the
trigger to fail with a cache lookup error. The error happens on
master, 13 and 12 I didnt test any previous versions.

trg=# ALTER TABLE t ALTER COLUMN f1 SET DATA TYPE bigint, ALTER COLUMN
f1 DROP IDENTITY;
ERROR: XX000: cache lookup failed for relation 13476892
CONTEXT: PL/pgSQL function ddl_end() line 5 at FOR over SELECT rows
LOCATION: getRelationTypeDescription, objectaddress.c:4178

For the ALTER DATA TYPE we create a command to adjust the sequence
which gets recorded in the event trigger commandlist, which leads to
the described failure when the sequence is dropped as part of another
ALTER TABLE subcommand and information about the sequence can no
longer be looked up.

To reproduce:
CREATE OR REPLACE FUNCTION ddl_end()
RETURNS event_trigger AS $$
DECLARE
r RECORD;
BEGIN
FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
LOOP
RAISE NOTICE 'ddl_end: % %', r.command_tag, r.object_type;
END LOOP;
END;
$$ LANGUAGE plpgsql;

CREATE EVENT TRIGGER ddl_end ON ddl_command_end EXECUTE PROCEDURE ddl_end();

CREATE TABLE t(f1 int NOT NULL GENERATED ALWAYS AS IDENTITY);
ALTER TABLE t ALTER COLUMN f1 DROP IDENTITY, ALTER COLUMN f1 SET DATA
TYPE bigint;

I tried really hard to look for a different way to detect this error
earlier but since the subcommands are processed independently i
couldnt come up with a non-invasive version. Someone more familiar
with this code might have an idea for a better solution.

Any thoughts?

/messages/by-id/CAMCrgp39V7JQA_Gc+JaEZV3ALOU1ZG=Pwyk3oDpTq7F6Z0JSmg@mail.gmail.com
--
Regards, Sven Klemm

Attachments:

v1-0001-Fix-pg_event_trigger_ddl_commands.patchapplication/x-patch; name=v1-0001-Fix-pg_event_trigger_ddl_commands.patchDownload
From 4faf761528c735bd808e7ea4d9f356710e3a8ed0 Mon Sep 17 00:00:00 2001
From: Sven Klemm <sven@timescale.com>
Date: Sat, 17 Apr 2021 21:54:03 +0200
Subject: [PATCH v1] Fix pg_event_trigger_ddl_commands

If an object is modified that is dropped in the same
command we might end up in a position where we
generated a message but can no longer look up the
object information because the object got dropped.
E.g. ALTER TABLE ALTER COLUMN, DROP IDENTITY
---
 src/backend/commands/event_trigger.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 5bde507c75..f7b2c30f82 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1936,8 +1936,17 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
 					else if (cmd->type == SCT_AlterTSConfig)
 						addr = cmd->d.atscfg.address;
 
-					type = getObjectTypeDescription(&addr, false);
-					identity = getObjectIdentity(&addr, false);
+					/*
+					 * If an object is modified that is dropped in the same
+					 * command we might end up in a position where we
+					 * generated a message but can no longer look up the
+					 * object information because the object got dropped.
+					 * E.g. ALTER TABLE ALTER COLUMN, DROP IDENTITY
+					 */
+					type = getObjectTypeDescription(&addr, true);
+					identity = getObjectIdentity(&addr, true);
+					if (!identity)
+						continue;
 
 					/*
 					 * Obtain schema name, if any ("pg_temp" if a temp
-- 
2.30.0

#2Sven Klemm
sven@timescale.com
In reply to: Sven Klemm (#1)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

On Sun, Apr 18, 2021 at 2:12 PM Sven Klemm <sven@timescale.com> wrote:

when creating an event trigger for ddl_command_end that calls
pg_event_trigger_ddl_commands certain statements will cause the
trigger to fail with a cache lookup error. The error happens on
master, 13 and 12 I didnt test any previous versions.

trg=# ALTER TABLE t ALTER COLUMN f1 SET DATA TYPE bigint, ALTER COLUMN
f1 DROP IDENTITY;
ERROR: XX000: cache lookup failed for relation 13476892
CONTEXT: PL/pgSQL function ddl_end() line 5 at FOR over SELECT rows
LOCATION: getRelationTypeDescription, objectaddress.c:4178

Any opinions on the patch? Is this not worth the effort to fix or is
there a better way to fix this?

/messages/by-id/CAMCrgp2R1cEXU53iYKtW6yVEp2_yKUz+z=3-CTrYpPP+xryRtg@mail.gmail.com

--
Regards, Sven Klemm

#3Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Sven Klemm (#2)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

On 2021-Apr-25, Sven Klemm wrote:

On Sun, Apr 18, 2021 at 2:12 PM Sven Klemm <sven@timescale.com> wrote:

when creating an event trigger for ddl_command_end that calls
pg_event_trigger_ddl_commands certain statements will cause the
trigger to fail with a cache lookup error. The error happens on
master, 13 and 12 I didnt test any previous versions.

trg=# ALTER TABLE t ALTER COLUMN f1 SET DATA TYPE bigint, ALTER COLUMN
f1 DROP IDENTITY;
ERROR: XX000: cache lookup failed for relation 13476892
CONTEXT: PL/pgSQL function ddl_end() line 5 at FOR over SELECT rows
LOCATION: getRelationTypeDescription, objectaddress.c:4178

Any opinions on the patch? Is this not worth the effort to fix or is
there a better way to fix this?

Hello, I haven't looked at this but judging from the general shape of
function and error message, it seems clearly a bug that needs to be
fixed somehow. I'll try to make time to look at it sometime soon, but I
have other bugs to investigate and fix, so it may be some time.

I fear your proposal of ignoring the object may be the best we can do,
but I don't like it much.

--
�lvaro Herrera 39�49'30"S 73�17'W
"La verdad no siempre es bonita, pero el hambre de ella s�"

#4Aleksander Alekseev
aleksander@timescale.com
In reply to: Alvaro Herrera (#3)
1 attachment(s)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

Hi hackers,

Any opinions on the patch? Is this not worth the effort to fix or is
there a better way to fix this?

I confirm that the bug still exists in master (be57f216). The patch
fixes it and looks good to me. I changed the wording a little and
added a regression test. The updated patch is in the attachment. I
added this thread to the CF and updated the status to "Ready for
Committer".

--
Best regards,
Aleksander Alekseev

Attachments:

v2-0001-Fix-pg_event_trigger_ddl_commands.patchapplication/octet-stream; name=v2-0001-Fix-pg_event_trigger_ddl_commands.patchDownload
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 5bde507c75..aea38be8db 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1936,8 +1936,18 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
 					else if (cmd->type == SCT_AlterTSConfig)
 						addr = cmd->d.atscfg.address;
 
-					type = getObjectTypeDescription(&addr, false);
-					identity = getObjectIdentity(&addr, false);
+					/*
+					 * If an object was dropped in the same command we may end
+					 * up in a situation where we generated a message but can
+					 * no longer look up the object information.
+					 * Example:
+					 * ALTER TABLE t ALTER COLUMN f1 DROP IDENTITY,
+					 *               ALTER COLUMN f1 SET DATA TYPE bigint;
+					 */
+					type = getObjectTypeDescription(&addr, true);
+					identity = getObjectIdentity(&addr, true);
+					if (!identity)
+						continue;
 
 					/*
 					 * Obtain schema name, if any ("pg_temp" if a temp
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 369f3d7d84..5c832c330d 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -553,3 +553,24 @@ SELECT
 DROP EVENT TRIGGER start_rls_command;
 DROP EVENT TRIGGER end_rls_command;
 DROP EVENT TRIGGER sql_drop_command;
+-- Regression test for the ALTER COLUMN DROP IDENTITY, ALTER COLUMN SET DATA TYPE case
+CREATE OR REPLACE FUNCTION ddl_end()
+RETURNS event_trigger AS $$
+DECLARE
+r RECORD;
+BEGIN
+  FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
+  LOOP
+    RAISE NOTICE 'ddl_end: % %', r.command_tag, r.object_type;
+  END LOOP;
+END;
+$$ LANGUAGE plpgsql;
+CREATE EVENT TRIGGER ddl_end ON ddl_command_end EXECUTE PROCEDURE ddl_end();
+CREATE TABLE t(f1 int NOT NULL GENERATED ALWAYS AS IDENTITY);
+NOTICE:  ddl_end: CREATE SEQUENCE sequence
+NOTICE:  ddl_end: CREATE TABLE table
+NOTICE:  ddl_end: ALTER SEQUENCE sequence
+ALTER TABLE t ALTER COLUMN f1 DROP IDENTITY, ALTER COLUMN f1 SET DATA TYPE bigint;
+NOTICE:  ddl_end: ALTER TABLE table
+DROP TABLE t;
+DROP EVENT TRIGGER ddl_end;
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index e79c5f0b5d..81c201fd96 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -440,3 +440,23 @@ SELECT
 DROP EVENT TRIGGER start_rls_command;
 DROP EVENT TRIGGER end_rls_command;
 DROP EVENT TRIGGER sql_drop_command;
+
+-- Regression test for the ALTER COLUMN DROP IDENTITY, ALTER COLUMN SET DATA TYPE case
+CREATE OR REPLACE FUNCTION ddl_end()
+RETURNS event_trigger AS $$
+DECLARE
+r RECORD;
+BEGIN
+  FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
+  LOOP
+    RAISE NOTICE 'ddl_end: % %', r.command_tag, r.object_type;
+  END LOOP;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE EVENT TRIGGER ddl_end ON ddl_command_end EXECUTE PROCEDURE ddl_end();
+CREATE TABLE t(f1 int NOT NULL GENERATED ALWAYS AS IDENTITY);
+ALTER TABLE t ALTER COLUMN f1 DROP IDENTITY, ALTER COLUMN f1 SET DATA TYPE bigint;
+
+DROP TABLE t;
+DROP EVENT TRIGGER ddl_end;
#5Michael Paquier
michael@paquier.xyz
In reply to: Aleksander Alekseev (#4)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

On Mon, Jun 07, 2021 at 12:44:42PM +0300, Aleksander Alekseev wrote:

I confirm that the bug still exists in master (be57f216). The patch
fixes it and looks good to me. I changed the wording a little and
added a regression test. The updated patch is in the attachment. I
added this thread to the CF and updated the status to "Ready for
Committer".

FWIW, that looks rather natural to me to me to just ignore the object
if it has already been dropped here. The same kind of rules apply to
tables dropped with DROP TABLE which would not show up as of
pg_event_trigger_ddl_commands(), but one can get a list as of
pg_event_trigger_dropped_objects().

Alvaro, were you planning to look at that? I have not looked at the
patch in details. missing_ok is available in getObjectIdentity() only
since v14, so this cannot be backpatched :/
--
Michael

#6Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Michael Paquier (#5)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

On 2021-Jun-09, Michael Paquier wrote:

On Mon, Jun 07, 2021 at 12:44:42PM +0300, Aleksander Alekseev wrote:

I confirm that the bug still exists in master (be57f216). The patch
fixes it and looks good to me. I changed the wording a little and
added a regression test. The updated patch is in the attachment. I
added this thread to the CF and updated the status to "Ready for
Committer".

FWIW, that looks rather natural to me to me to just ignore the object
if it has already been dropped here. The same kind of rules apply to
tables dropped with DROP TABLE which would not show up as of
pg_event_trigger_ddl_commands(), but one can get a list as of
pg_event_trigger_dropped_objects().

Oh, that parallel didn't occur to me. I agree it seems a useful
precedent.

Alvaro, were you planning to look at that? I have not looked at the
patch in details.

I have it on my list of things to look at, but it isn't priority. If
you to mess with it, please be my guest.

missing_ok is available in getObjectIdentity() only
since v14, so this cannot be backpatched :/

Ooh, yeah, I forgot about that. And that one was pretty invasive ...

I'm not sure if we can reasonably implement a fix for older releases.
I mean, it's a relatively easy test: do a syscache search for the object
or a catalog indexscan (easy to do with get_object_property_data-based
API), and if the object is gone, skip getObjectTypeDescription and
getObjectIdentity. But maybe this is too much code to add to stable
releases ...

--
�lvaro Herrera Valdivia, Chile

#7Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#6)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

On Wed, Jun 09, 2021 at 09:55:08AM -0400, Alvaro Herrera wrote:

I'm not sure if we can reasonably implement a fix for older releases.
I mean, it's a relatively easy test: do a syscache search for the object
or a catalog indexscan (easy to do with get_object_property_data-based
API), and if the object is gone, skip getObjectTypeDescription and
getObjectIdentity. But maybe this is too much code to add to stable
releases ...

Except that these syscache lookups need to be done on an object-type
basis, which is basically what getObjectDescription() & friends now do
where the logic makes sure that we have a correct objectId <-> cacheId
mapping for the syscache lookups. So that would be roughly copying
into event_trigger.c what objectaddress.c does now, but for the back
branches. It would be better to just backport the changes to support
missing_ok in objectaddress.c if we go down this road, but the
invasiveness makes that much more complicated.

Another thing is that ATExecDropIdentity() does performDeletion() by
using PERFORM_DELETION_INTERNAL, which does not feed the dropped
sequence to pg_event_trigger_dropped_objects(). That feels
inconsistent with CREATE TABLE GENERATED that would feed to event
triggers the CREATE SEQUENCE and ALTER SEQUENCE commands, as well as
ALTER TABLE SET DATA TYPE on a generated column that feeds an internal
ALTER SEQUENCE.

An extra idea we could consider, as the drop events are logged before
the other ALTER TABLE subcommands, is to look at the event triggers
registered when the generated sequence is dropped and to erase from
existence any previous events associated to it, but that would make
the logic weak as hell. In all this stuff, simply ignoring the
objects still sounds like a fair and simple course of action to take
if they are gone at the time an event trigger checks after them within
pg_event_trigger_ddl_commands(). Now, this approach makes my spider
sense tingle.
--
Michael

#8Michael Paquier
michael@paquier.xyz
In reply to: Michael Paquier (#7)
1 attachment(s)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

On Thu, Jun 10, 2021 at 05:07:28PM +0900, Michael Paquier wrote:

Except that these syscache lookups need to be done on an object-type
basis, which is basically what getObjectDescription() & friends now do
where the logic makes sure that we have a correct objectId <-> cacheId
mapping for the syscache lookups. So that would be roughly copying
into event_trigger.c what objectaddress.c does now, but for the back
branches. It would be better to just backport the changes to support
missing_ok in objectaddress.c if we go down this road, but the
invasiveness makes that much more complicated.

I have been looking at that more this morning, and I have convinced
myself that skipping objects should work fine. The test added at the
bottom of event_trigger.sql was making the file a bit messier though,
and there are already tests for relations when it comes to dropped
objects. So let's do a bit of consolidation while on it with an extra
event trigger on ddl_command_end and relations on the schema evttrig.

This one already included some cases for serial columns, so that's
natural to me to extend the area for identity columns. I have also
added a case for a serial column dropped, while on it. The last thing
is the addition of r.object_identity from
pg_event_trigger_ddl_commands() in the data generated for the output
messages, so as the output is as complete as possible.

Regarding the back-branches, I am tempted to do nothing. The APIs are
just not here to do the job. On top of being an invasive change, it
took 4 years for somebody to complain on this matter, as this exists
since 10. That's not worth the risk/cost.
--
Michael

Attachments:

altertab-eventtrigger-v3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 5bde507c75..d9d1b28401 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1936,8 +1936,19 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
 					else if (cmd->type == SCT_AlterTSConfig)
 						addr = cmd->d.atscfg.address;
 
-					type = getObjectTypeDescription(&addr, false);
-					identity = getObjectIdentity(&addr, false);
+					/*
+					 * If an object was dropped in the same command we may end
+					 * up in a situation where we generated a message but can
+					 * no longer look for the object information, so skip it
+					 * rather than failing.  This can happen for example with
+					 * some subcommand combinations of ALTER TABLE.
+					 */
+					identity = getObjectIdentity(&addr, true);
+					if (identity == NULL)
+						continue;
+
+					/* The type can never be NULL */
+					type = getObjectTypeDescription(&addr, true);
 
 					/*
 					 * Obtain schema name, if any ("pg_temp" if a temp
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 369f3d7d84..44d545de25 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -366,6 +366,7 @@ SELECT * FROM dropped_objects WHERE type = 'schema';
 DROP ROLE regress_evt_user;
 DROP EVENT TRIGGER regress_event_trigger_drop_objects;
 DROP EVENT TRIGGER undroppable;
+-- Event triggers on relations.
 CREATE OR REPLACE FUNCTION event_trigger_report_dropped()
  RETURNS event_trigger
  LANGUAGE plpgsql
@@ -384,41 +385,90 @@ BEGIN
 END; $$;
 CREATE EVENT TRIGGER regress_event_trigger_report_dropped ON sql_drop
     EXECUTE PROCEDURE event_trigger_report_dropped();
+CREATE OR REPLACE FUNCTION event_trigger_report_end()
+ RETURNS event_trigger
+ LANGUAGE plpgsql
+AS $$
+DECLARE r RECORD;
+BEGIN
+    FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
+    LOOP
+        RAISE NOTICE 'END: command_tag=% type=% identity=%',
+            r.command_tag, r.object_type, r.object_identity;
+    END LOOP;
+END; $$;
+CREATE EVENT TRIGGER regress_event_trigger_report_end ON ddl_command_end
+  EXECUTE PROCEDURE event_trigger_report_end();
 CREATE SCHEMA evttrig
-	CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two')
+	CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two', col_c SERIAL)
 	CREATE INDEX one_idx ON one (col_b)
-	CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42);
+	CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42)
+	CREATE TABLE id (col_d int NOT NULL GENERATED ALWAYS AS IDENTITY);
+NOTICE:  END: command_tag=CREATE SCHEMA type=schema identity=evttrig
+NOTICE:  END: command_tag=CREATE SEQUENCE type=sequence identity=evttrig.one_col_a_seq
+NOTICE:  END: command_tag=CREATE SEQUENCE type=sequence identity=evttrig.one_col_c_seq
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.one
+NOTICE:  END: command_tag=CREATE INDEX type=index identity=evttrig.one_pkey
+NOTICE:  END: command_tag=ALTER SEQUENCE type=sequence identity=evttrig.one_col_a_seq
+NOTICE:  END: command_tag=ALTER SEQUENCE type=sequence identity=evttrig.one_col_c_seq
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.two
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.two
+NOTICE:  END: command_tag=CREATE SEQUENCE type=sequence identity=evttrig.id_col_d_seq
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.id
+NOTICE:  END: command_tag=ALTER SEQUENCE type=sequence identity=evttrig.id_col_d_seq
+NOTICE:  END: command_tag=CREATE INDEX type=index identity=evttrig.one_idx
 -- Partitioned tables with a partitioned index
 CREATE TABLE evttrig.parted (
     id int PRIMARY KEY)
     PARTITION BY RANGE (id);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.parted
+NOTICE:  END: command_tag=CREATE INDEX type=index identity=evttrig.parted_pkey
 CREATE TABLE evttrig.part_1_10 PARTITION OF evttrig.parted (id)
   FOR VALUES FROM (1) TO (10);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.part_1_10
 CREATE TABLE evttrig.part_10_20 PARTITION OF evttrig.parted (id)
   FOR VALUES FROM (10) TO (20) PARTITION BY RANGE (id);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.part_10_20
 CREATE TABLE evttrig.part_10_15 PARTITION OF evttrig.part_10_20 (id)
   FOR VALUES FROM (10) TO (15);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.part_10_15
 CREATE TABLE evttrig.part_15_20 PARTITION OF evttrig.part_10_20 (id)
   FOR VALUES FROM (15) TO (20);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.part_15_20
 ALTER TABLE evttrig.two DROP COLUMN col_c;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=table column identity=evttrig.two.col_c name={evttrig,two,col_c} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table constraint identity=two_col_c_check on evttrig.two name={evttrig,two,two_col_c_check} args={}
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.two
 ALTER TABLE evttrig.one ALTER COLUMN col_b DROP DEFAULT;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=default value identity=for evttrig.one.col_b name={evttrig,one,col_b} args={}
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.one
 ALTER TABLE evttrig.one DROP CONSTRAINT one_pkey;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=table constraint identity=one_pkey on evttrig.one name={evttrig,one,one_pkey} args={}
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.one
+ALTER TABLE evttrig.one DROP COLUMN col_c;
+NOTICE:  NORMAL: orig=t normal=f istemp=f type=table column identity=evttrig.one.col_c name={evttrig,one,col_c} args={}
+NOTICE:  NORMAL: orig=f normal=t istemp=f type=default value identity=for evttrig.one.col_c name={evttrig,one,col_c} args={}
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.one
+ALTER TABLE evttrig.id ALTER COLUMN col_d SET DATA TYPE bigint;
+NOTICE:  END: command_tag=ALTER SEQUENCE type=sequence identity=evttrig.id_col_d_seq
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.id
+ALTER TABLE evttrig.id ALTER COLUMN col_d DROP IDENTITY,
+  ALTER COLUMN col_d SET DATA TYPE int;
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.id
 DROP INDEX evttrig.one_idx;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=index identity=evttrig.one_idx name={evttrig,one_idx} args={}
 DROP SCHEMA evttrig CASCADE;
-NOTICE:  drop cascades to 3 other objects
+NOTICE:  drop cascades to 4 other objects
 DETAIL:  drop cascades to table evttrig.one
 drop cascades to table evttrig.two
+drop cascades to table evttrig.id
 drop cascades to table evttrig.parted
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=schema identity=evttrig name={evttrig} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.one name={evttrig,one} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=sequence identity=evttrig.one_col_a_seq name={evttrig,one_col_a_seq} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=default value identity=for evttrig.one.col_a name={evttrig,one,col_a} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.two name={evttrig,two} args={}
+NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.id name={evttrig,id} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.parted name={evttrig,parted} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_1_10 name={evttrig,part_1_10} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_10_20 name={evttrig,part_10_20} args={}
@@ -427,6 +477,7 @@ NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_15_20
 DROP TABLE a_temp_tbl;
 NOTICE:  NORMAL: orig=t normal=f istemp=t type=table identity=pg_temp.a_temp_tbl name={pg_temp,a_temp_tbl} args={}
 DROP EVENT TRIGGER regress_event_trigger_report_dropped;
+DROP EVENT TRIGGER regress_event_trigger_report_end;
 -- only allowed from within an event trigger function, should fail
 select pg_event_trigger_table_rewrite_oid();
 ERROR:  pg_event_trigger_table_rewrite_oid() can only be called in a table_rewrite event trigger function
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index e79c5f0b5d..1446cf8cc8 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -273,6 +273,7 @@ DROP ROLE regress_evt_user;
 DROP EVENT TRIGGER regress_event_trigger_drop_objects;
 DROP EVENT TRIGGER undroppable;
 
+-- Event triggers on relations.
 CREATE OR REPLACE FUNCTION event_trigger_report_dropped()
  RETURNS event_trigger
  LANGUAGE plpgsql
@@ -291,10 +292,26 @@ BEGIN
 END; $$;
 CREATE EVENT TRIGGER regress_event_trigger_report_dropped ON sql_drop
     EXECUTE PROCEDURE event_trigger_report_dropped();
+CREATE OR REPLACE FUNCTION event_trigger_report_end()
+ RETURNS event_trigger
+ LANGUAGE plpgsql
+AS $$
+DECLARE r RECORD;
+BEGIN
+    FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
+    LOOP
+        RAISE NOTICE 'END: command_tag=% type=% identity=%',
+            r.command_tag, r.object_type, r.object_identity;
+    END LOOP;
+END; $$;
+CREATE EVENT TRIGGER regress_event_trigger_report_end ON ddl_command_end
+  EXECUTE PROCEDURE event_trigger_report_end();
+
 CREATE SCHEMA evttrig
-	CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two')
+	CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two', col_c SERIAL)
 	CREATE INDEX one_idx ON one (col_b)
-	CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42);
+	CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42)
+	CREATE TABLE id (col_d int NOT NULL GENERATED ALWAYS AS IDENTITY);
 
 -- Partitioned tables with a partitioned index
 CREATE TABLE evttrig.parted (
@@ -312,11 +329,16 @@ CREATE TABLE evttrig.part_15_20 PARTITION OF evttrig.part_10_20 (id)
 ALTER TABLE evttrig.two DROP COLUMN col_c;
 ALTER TABLE evttrig.one ALTER COLUMN col_b DROP DEFAULT;
 ALTER TABLE evttrig.one DROP CONSTRAINT one_pkey;
+ALTER TABLE evttrig.one DROP COLUMN col_c;
+ALTER TABLE evttrig.id ALTER COLUMN col_d SET DATA TYPE bigint;
+ALTER TABLE evttrig.id ALTER COLUMN col_d DROP IDENTITY,
+  ALTER COLUMN col_d SET DATA TYPE int;
 DROP INDEX evttrig.one_idx;
 DROP SCHEMA evttrig CASCADE;
 DROP TABLE a_temp_tbl;
 
 DROP EVENT TRIGGER regress_event_trigger_report_dropped;
+DROP EVENT TRIGGER regress_event_trigger_report_end;
 
 -- only allowed from within an event trigger function, should fail
 select pg_event_trigger_table_rewrite_oid();
#9Aleksander Alekseev
aleksander@timescale.com
In reply to: Michael Paquier (#8)
1 attachment(s)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

Hi Michael,

/* The type can never be NULL */
type = getObjectTypeDescription(&addr, true);

The last argument should be `false` then.

--
Best regards,
Aleksander Alekseev

Attachments:

v4-0001-pg_event_trigger_ddl_commands.patchapplication/octet-stream; name=v4-0001-pg_event_trigger_ddl_commands.patchDownload
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 5bde507c75..5fdd063162 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1936,8 +1936,19 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
 					else if (cmd->type == SCT_AlterTSConfig)
 						addr = cmd->d.atscfg.address;
 
+					/*
+					 * If an object was dropped in the same command we may end
+					 * up in a situation where we generated a message but can
+					 * no longer look for the object information, so skip it
+					 * rather than failing.  This can happen for example with
+					 * some subcommand combinations of ALTER TABLE.
+					 */
+					identity = getObjectIdentity(&addr, true);
+					if (identity == NULL)
+						continue;
+
+					/* The type can never be NULL */
 					type = getObjectTypeDescription(&addr, false);
-					identity = getObjectIdentity(&addr, false);
 
 					/*
 					 * Obtain schema name, if any ("pg_temp" if a temp
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 369f3d7d84..44d545de25 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -366,6 +366,7 @@ SELECT * FROM dropped_objects WHERE type = 'schema';
 DROP ROLE regress_evt_user;
 DROP EVENT TRIGGER regress_event_trigger_drop_objects;
 DROP EVENT TRIGGER undroppable;
+-- Event triggers on relations.
 CREATE OR REPLACE FUNCTION event_trigger_report_dropped()
  RETURNS event_trigger
  LANGUAGE plpgsql
@@ -384,41 +385,90 @@ BEGIN
 END; $$;
 CREATE EVENT TRIGGER regress_event_trigger_report_dropped ON sql_drop
     EXECUTE PROCEDURE event_trigger_report_dropped();
+CREATE OR REPLACE FUNCTION event_trigger_report_end()
+ RETURNS event_trigger
+ LANGUAGE plpgsql
+AS $$
+DECLARE r RECORD;
+BEGIN
+    FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
+    LOOP
+        RAISE NOTICE 'END: command_tag=% type=% identity=%',
+            r.command_tag, r.object_type, r.object_identity;
+    END LOOP;
+END; $$;
+CREATE EVENT TRIGGER regress_event_trigger_report_end ON ddl_command_end
+  EXECUTE PROCEDURE event_trigger_report_end();
 CREATE SCHEMA evttrig
-	CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two')
+	CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two', col_c SERIAL)
 	CREATE INDEX one_idx ON one (col_b)
-	CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42);
+	CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42)
+	CREATE TABLE id (col_d int NOT NULL GENERATED ALWAYS AS IDENTITY);
+NOTICE:  END: command_tag=CREATE SCHEMA type=schema identity=evttrig
+NOTICE:  END: command_tag=CREATE SEQUENCE type=sequence identity=evttrig.one_col_a_seq
+NOTICE:  END: command_tag=CREATE SEQUENCE type=sequence identity=evttrig.one_col_c_seq
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.one
+NOTICE:  END: command_tag=CREATE INDEX type=index identity=evttrig.one_pkey
+NOTICE:  END: command_tag=ALTER SEQUENCE type=sequence identity=evttrig.one_col_a_seq
+NOTICE:  END: command_tag=ALTER SEQUENCE type=sequence identity=evttrig.one_col_c_seq
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.two
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.two
+NOTICE:  END: command_tag=CREATE SEQUENCE type=sequence identity=evttrig.id_col_d_seq
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.id
+NOTICE:  END: command_tag=ALTER SEQUENCE type=sequence identity=evttrig.id_col_d_seq
+NOTICE:  END: command_tag=CREATE INDEX type=index identity=evttrig.one_idx
 -- Partitioned tables with a partitioned index
 CREATE TABLE evttrig.parted (
     id int PRIMARY KEY)
     PARTITION BY RANGE (id);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.parted
+NOTICE:  END: command_tag=CREATE INDEX type=index identity=evttrig.parted_pkey
 CREATE TABLE evttrig.part_1_10 PARTITION OF evttrig.parted (id)
   FOR VALUES FROM (1) TO (10);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.part_1_10
 CREATE TABLE evttrig.part_10_20 PARTITION OF evttrig.parted (id)
   FOR VALUES FROM (10) TO (20) PARTITION BY RANGE (id);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.part_10_20
 CREATE TABLE evttrig.part_10_15 PARTITION OF evttrig.part_10_20 (id)
   FOR VALUES FROM (10) TO (15);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.part_10_15
 CREATE TABLE evttrig.part_15_20 PARTITION OF evttrig.part_10_20 (id)
   FOR VALUES FROM (15) TO (20);
+NOTICE:  END: command_tag=CREATE TABLE type=table identity=evttrig.part_15_20
 ALTER TABLE evttrig.two DROP COLUMN col_c;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=table column identity=evttrig.two.col_c name={evttrig,two,col_c} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table constraint identity=two_col_c_check on evttrig.two name={evttrig,two,two_col_c_check} args={}
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.two
 ALTER TABLE evttrig.one ALTER COLUMN col_b DROP DEFAULT;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=default value identity=for evttrig.one.col_b name={evttrig,one,col_b} args={}
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.one
 ALTER TABLE evttrig.one DROP CONSTRAINT one_pkey;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=table constraint identity=one_pkey on evttrig.one name={evttrig,one,one_pkey} args={}
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.one
+ALTER TABLE evttrig.one DROP COLUMN col_c;
+NOTICE:  NORMAL: orig=t normal=f istemp=f type=table column identity=evttrig.one.col_c name={evttrig,one,col_c} args={}
+NOTICE:  NORMAL: orig=f normal=t istemp=f type=default value identity=for evttrig.one.col_c name={evttrig,one,col_c} args={}
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.one
+ALTER TABLE evttrig.id ALTER COLUMN col_d SET DATA TYPE bigint;
+NOTICE:  END: command_tag=ALTER SEQUENCE type=sequence identity=evttrig.id_col_d_seq
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.id
+ALTER TABLE evttrig.id ALTER COLUMN col_d DROP IDENTITY,
+  ALTER COLUMN col_d SET DATA TYPE int;
+NOTICE:  END: command_tag=ALTER TABLE type=table identity=evttrig.id
 DROP INDEX evttrig.one_idx;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=index identity=evttrig.one_idx name={evttrig,one_idx} args={}
 DROP SCHEMA evttrig CASCADE;
-NOTICE:  drop cascades to 3 other objects
+NOTICE:  drop cascades to 4 other objects
 DETAIL:  drop cascades to table evttrig.one
 drop cascades to table evttrig.two
+drop cascades to table evttrig.id
 drop cascades to table evttrig.parted
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=schema identity=evttrig name={evttrig} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.one name={evttrig,one} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=sequence identity=evttrig.one_col_a_seq name={evttrig,one_col_a_seq} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=default value identity=for evttrig.one.col_a name={evttrig,one,col_a} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.two name={evttrig,two} args={}
+NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.id name={evttrig,id} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.parted name={evttrig,parted} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_1_10 name={evttrig,part_1_10} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_10_20 name={evttrig,part_10_20} args={}
@@ -427,6 +477,7 @@ NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_15_20
 DROP TABLE a_temp_tbl;
 NOTICE:  NORMAL: orig=t normal=f istemp=t type=table identity=pg_temp.a_temp_tbl name={pg_temp,a_temp_tbl} args={}
 DROP EVENT TRIGGER regress_event_trigger_report_dropped;
+DROP EVENT TRIGGER regress_event_trigger_report_end;
 -- only allowed from within an event trigger function, should fail
 select pg_event_trigger_table_rewrite_oid();
 ERROR:  pg_event_trigger_table_rewrite_oid() can only be called in a table_rewrite event trigger function
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index e79c5f0b5d..1446cf8cc8 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -273,6 +273,7 @@ DROP ROLE regress_evt_user;
 DROP EVENT TRIGGER regress_event_trigger_drop_objects;
 DROP EVENT TRIGGER undroppable;
 
+-- Event triggers on relations.
 CREATE OR REPLACE FUNCTION event_trigger_report_dropped()
  RETURNS event_trigger
  LANGUAGE plpgsql
@@ -291,10 +292,26 @@ BEGIN
 END; $$;
 CREATE EVENT TRIGGER regress_event_trigger_report_dropped ON sql_drop
     EXECUTE PROCEDURE event_trigger_report_dropped();
+CREATE OR REPLACE FUNCTION event_trigger_report_end()
+ RETURNS event_trigger
+ LANGUAGE plpgsql
+AS $$
+DECLARE r RECORD;
+BEGIN
+    FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
+    LOOP
+        RAISE NOTICE 'END: command_tag=% type=% identity=%',
+            r.command_tag, r.object_type, r.object_identity;
+    END LOOP;
+END; $$;
+CREATE EVENT TRIGGER regress_event_trigger_report_end ON ddl_command_end
+  EXECUTE PROCEDURE event_trigger_report_end();
+
 CREATE SCHEMA evttrig
-	CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two')
+	CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two', col_c SERIAL)
 	CREATE INDEX one_idx ON one (col_b)
-	CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42);
+	CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42)
+	CREATE TABLE id (col_d int NOT NULL GENERATED ALWAYS AS IDENTITY);
 
 -- Partitioned tables with a partitioned index
 CREATE TABLE evttrig.parted (
@@ -312,11 +329,16 @@ CREATE TABLE evttrig.part_15_20 PARTITION OF evttrig.part_10_20 (id)
 ALTER TABLE evttrig.two DROP COLUMN col_c;
 ALTER TABLE evttrig.one ALTER COLUMN col_b DROP DEFAULT;
 ALTER TABLE evttrig.one DROP CONSTRAINT one_pkey;
+ALTER TABLE evttrig.one DROP COLUMN col_c;
+ALTER TABLE evttrig.id ALTER COLUMN col_d SET DATA TYPE bigint;
+ALTER TABLE evttrig.id ALTER COLUMN col_d DROP IDENTITY,
+  ALTER COLUMN col_d SET DATA TYPE int;
 DROP INDEX evttrig.one_idx;
 DROP SCHEMA evttrig CASCADE;
 DROP TABLE a_temp_tbl;
 
 DROP EVENT TRIGGER regress_event_trigger_report_dropped;
+DROP EVENT TRIGGER regress_event_trigger_report_end;
 
 -- only allowed from within an event trigger function, should fail
 select pg_event_trigger_table_rewrite_oid();
#10Michael Paquier
michael@paquier.xyz
In reply to: Aleksander Alekseev (#9)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

On Fri, Jun 11, 2021 at 11:00:40AM +0300, Aleksander Alekseev wrote:

The last argument should be `false` then.

Hm, nope. I think that we had better pass true as argument here.

First, this is more consistent with the identity lookup (OK, it does
not matter as we would have discarded the object after the identity
lookup anyway, but any future shuffling of this code may not be that
wise). Second, now that I look at it, getObjectTypeDescription() can
never be NULL as we have fallback names for relations, routines and
constraints for all object types so the buffer will be filled with
some data. Let's replace the bottom of getObjectTypeDescription()
that returns now NULL by Assert(buffer.len > 0). This code is new as
of v14, so it is better to adjust that sooner than later.
--
Michael

#11Michael Paquier
michael@paquier.xyz
In reply to: Michael Paquier (#10)
Re: Fix dropped object handling in pg_event_trigger_ddl_commands

On Fri, Jun 11, 2021 at 09:36:57PM +0900, Michael Paquier wrote:

Hm, nope. I think that we had better pass true as argument here.

The main patch has been applied as of 2d689ba.

First, this is more consistent with the identity lookup (OK, it does
not matter as we would have discarded the object after the identity
lookup anyway, but any future shuffling of this code may not be that
wise). Second, now that I look at it, getObjectTypeDescription() can
never be NULL as we have fallback names for relations, routines and
constraints for all object types so the buffer will be filled with
some data. Let's replace the bottom of getObjectTypeDescription()
that returns now NULL by Assert(buffer.len > 0). This code is new as
of v14, so it is better to adjust that sooner than later.

And this has been simplified with b56b83a.
--
Michael