Date: Fri, 11 Jun 2021 12:46:56 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Aleksander Alekseev <aleksander@timescale.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Sven Klemm <sven@timescale.com>
Subject: Re: Fix dropped object handling in pg_event_trigger_ddl_commands
Message-ID: <YMLcsJrjWAYs/7SL@paquier.xyz>
References: <YMBRvAIZxJyQE5fv@paquier.xyz>
 <202106091355.sphciftmvqds@alvherre.pgsql>
 <YMHIQAdUA7LkrsFZ@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="CL61+9R94HGqHaAv"
Content-Disposition: inline
In-Reply-To: <YMHIQAdUA7LkrsFZ@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/YMLcsJrjWAYs/7SL%40paquier.xyz>
Precedence: bulk


--CL61+9R94HGqHaAv
Content-Type: multipart/mixed; boundary="bejncxCX384gzbbn"
Content-Disposition: inline


--bejncxCX384gzbbn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--bejncxCX384gzbbn
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="altertab-eventtrigger-v3.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/ev=
ent_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 =3D=3D SCT_AlterTSConfig)
 						addr =3D cmd->d.atscfg.address;
=20
-					type =3D getObjectTypeDescription(&addr, false);
-					identity =3D 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 =3D getObjectIdentity(&addr, true);
+					if (identity =3D=3D NULL)
+						continue;
+
+					/* The type can never be NULL */
+					type =3D getObjectTypeDescription(&addr, true);
=20
 					/*
 					 * 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 =3D '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=3D% type=3D% identity=3D%',
+            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=3DCREATE SCHEMA type=3Dschema identity=3Devttrig
+NOTICE:  END: command_tag=3DCREATE SEQUENCE type=3Dsequence identity=3Devt=
trig.one_col_a_seq
+NOTICE:  END: command_tag=3DCREATE SEQUENCE type=3Dsequence identity=3Devt=
trig.one_col_c_seq
+NOTICE:  END: command_tag=3DCREATE TABLE type=3Dtable identity=3Devttrig.o=
ne
+NOTICE:  END: command_tag=3DCREATE INDEX type=3Dindex identity=3Devttrig.o=
ne_pkey
+NOTICE:  END: command_tag=3DALTER SEQUENCE type=3Dsequence identity=3Devtt=
rig.one_col_a_seq
+NOTICE:  END: command_tag=3DALTER SEQUENCE type=3Dsequence identity=3Devtt=
rig.one_col_c_seq
+NOTICE:  END: command_tag=3DCREATE TABLE type=3Dtable identity=3Devttrig.t=
wo
+NOTICE:  END: command_tag=3DALTER TABLE type=3Dtable identity=3Devttrig.two
+NOTICE:  END: command_tag=3DCREATE SEQUENCE type=3Dsequence identity=3Devt=
trig.id_col_d_seq
+NOTICE:  END: command_tag=3DCREATE TABLE type=3Dtable identity=3Devttrig.id
+NOTICE:  END: command_tag=3DALTER SEQUENCE type=3Dsequence identity=3Devtt=
rig.id_col_d_seq
+NOTICE:  END: command_tag=3DCREATE INDEX type=3Dindex identity=3Devttrig.o=
ne_idx
 -- Partitioned tables with a partitioned index
 CREATE TABLE evttrig.parted (
     id int PRIMARY KEY)
     PARTITION BY RANGE (id);
+NOTICE:  END: command_tag=3DCREATE TABLE type=3Dtable identity=3Devttrig.p=
arted
+NOTICE:  END: command_tag=3DCREATE INDEX type=3Dindex identity=3Devttrig.p=
arted_pkey
 CREATE TABLE evttrig.part_1_10 PARTITION OF evttrig.parted (id)
   FOR VALUES FROM (1) TO (10);
+NOTICE:  END: command_tag=3DCREATE TABLE type=3Dtable identity=3Devttrig.p=
art_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=3DCREATE TABLE type=3Dtable identity=3Devttrig.p=
art_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=3DCREATE TABLE type=3Dtable identity=3Devttrig.p=
art_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=3DCREATE TABLE type=3Dtable identity=3Devttrig.p=
art_15_20
 ALTER TABLE evttrig.two DROP COLUMN col_c;
 NOTICE:  NORMAL: orig=3Dt normal=3Df istemp=3Df type=3Dtable column identi=
ty=3Devttrig.two.col_c name=3D{evttrig,two,col_c} args=3D{}
 NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Dtable constraint id=
entity=3Dtwo_col_c_check on evttrig.two name=3D{evttrig,two,two_col_c_check=
} args=3D{}
+NOTICE:  END: command_tag=3DALTER TABLE type=3Dtable identity=3Devttrig.two
 ALTER TABLE evttrig.one ALTER COLUMN col_b DROP DEFAULT;
 NOTICE:  NORMAL: orig=3Dt normal=3Df istemp=3Df type=3Ddefault value ident=
ity=3Dfor evttrig.one.col_b name=3D{evttrig,one,col_b} args=3D{}
+NOTICE:  END: command_tag=3DALTER TABLE type=3Dtable identity=3Devttrig.one
 ALTER TABLE evttrig.one DROP CONSTRAINT one_pkey;
 NOTICE:  NORMAL: orig=3Dt normal=3Df istemp=3Df type=3Dtable constraint id=
entity=3Done_pkey on evttrig.one name=3D{evttrig,one,one_pkey} args=3D{}
+NOTICE:  END: command_tag=3DALTER TABLE type=3Dtable identity=3Devttrig.one
+ALTER TABLE evttrig.one DROP COLUMN col_c;
+NOTICE:  NORMAL: orig=3Dt normal=3Df istemp=3Df type=3Dtable column identi=
ty=3Devttrig.one.col_c name=3D{evttrig,one,col_c} args=3D{}
+NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Ddefault value ident=
ity=3Dfor evttrig.one.col_c name=3D{evttrig,one,col_c} args=3D{}
+NOTICE:  END: command_tag=3DALTER TABLE type=3Dtable identity=3Devttrig.one
+ALTER TABLE evttrig.id ALTER COLUMN col_d SET DATA TYPE bigint;
+NOTICE:  END: command_tag=3DALTER SEQUENCE type=3Dsequence identity=3Devtt=
rig.id_col_d_seq
+NOTICE:  END: command_tag=3DALTER TABLE type=3Dtable identity=3Devttrig.id
+ALTER TABLE evttrig.id ALTER COLUMN col_d DROP IDENTITY,
+  ALTER COLUMN col_d SET DATA TYPE int;
+NOTICE:  END: command_tag=3DALTER TABLE type=3Dtable identity=3Devttrig.id
 DROP INDEX evttrig.one_idx;
 NOTICE:  NORMAL: orig=3Dt normal=3Df istemp=3Df type=3Dindex identity=3Dev=
ttrig.one_idx name=3D{evttrig,one_idx} args=3D{}
 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=3Dt normal=3Df istemp=3Df type=3Dschema identity=3De=
vttrig name=3D{evttrig} args=3D{}
 NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Dtable identity=3Dev=
ttrig.one name=3D{evttrig,one} args=3D{}
 NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Dsequence identity=
=3Devttrig.one_col_a_seq name=3D{evttrig,one_col_a_seq} args=3D{}
 NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Ddefault value ident=
ity=3Dfor evttrig.one.col_a name=3D{evttrig,one,col_a} args=3D{}
 NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Dtable identity=3Dev=
ttrig.two name=3D{evttrig,two} args=3D{}
+NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Dtable identity=3Dev=
ttrig.id name=3D{evttrig,id} args=3D{}
 NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Dtable identity=3Dev=
ttrig.parted name=3D{evttrig,parted} args=3D{}
 NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Dtable identity=3Dev=
ttrig.part_1_10 name=3D{evttrig,part_1_10} args=3D{}
 NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3Dtable identity=3Dev=
ttrig.part_10_20 name=3D{evttrig,part_10_20} args=3D{}
@@ -427,6 +477,7 @@ NOTICE:  NORMAL: orig=3Df normal=3Dt istemp=3Df type=3D=
table identity=3Devttrig.part_15_20
 DROP TABLE a_temp_tbl;
 NOTICE:  NORMAL: orig=3Dt normal=3Df istemp=3Dt type=3Dtable identity=3Dpg=
_temp.a_temp_tbl name=3D{pg_temp,a_temp_tbl} args=3D{}
 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;
=20
+-- 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=3D% type=3D% identity=3D%',
+            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);
=20
 -- 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;
=20
 DROP EVENT TRIGGER regress_event_trigger_report_dropped;
+DROP EVENT TRIGGER regress_event_trigger_report_end;
=20
 -- only allowed from within an event trigger function, should fail
 select pg_event_trigger_table_rewrite_oid();

--bejncxCX384gzbbn--

--CL61+9R94HGqHaAv
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmDC3LAACgkQnvQgOdby
QH28lQ//c5mrngVdFHlR5NCwF1tdf5US51QV47nS8lflgwV/b0Q4pBKz+SYpXyCH
Mbs5tE1uzJVAPo4bcYa8bQuNB5OLtNBAT+37FP0WAItsLgBgMXFVorwpgVYsKBmU
3KLjsGmByNjwEI3aJ4IeRjvA2Mf4S4IwPwHaTlI238lv6WWzLLJTUlNCaMPY4I0V
0cShMoHctcljWn/BKPtR2gr/hWB4YHNnxm7wfZHTX3AXWaMsvmgbsivtEBE8fdP0
1YssD5AOIbMariVn6mHp73wdoqzrzsU2yHUw1tsBgnR668IW1UVb0QPFukRGscPZ
XVKUUqqMvisDQGC3ktDpiz4tSs50f/fhoInCth1CMhrK4Cccgzgytk/G1LHZZA5w
szcKWyv/zEFAeVBhnvrOJT8aCzRD8ICXELXldJ1VsmeXW9+s4sAfYvN4DraOtANV
gK4osommGSEKlddEFezX2Qmeurj9j67izh9NJRPMnPueisCLlb0V+xWXbHe36T5D
tM8hcV4byVeeoXSUAwQOa29/kloc5GQy7qJSlLrPqhH84h6FCK7O3JhZU9xAsU4c
09xQDCp1v53EMiu5EqOalMIHROBtSU7zNNvhOCijRNyqBntRE/WzGucsi6H3uRCa
tB2lcNelh8vfNjJERbYh7R5CCW1TFzYAGo2ev5UYu+yQheBTlcI=
=c0zF
-----END PGP SIGNATURE-----

--CL61+9R94HGqHaAv--