Re: CREATE TABLE/ProcessUtility hook behavior change

On Tue, Apr 30, 2024 at 10:26 AM David Steele <david(at)pgmasters(dot)net> wrote:
>
> Hackers,
>
> While testing pgAudit against PG17 I noticed the following behavioral
> change:
>
> CREATE TABLE public.test
> (
> id INT,
> name TEXT,
> description TEXT,
> CONSTRAINT test_pkey PRIMARY KEY (id)
> );
> NOTICE: AUDIT: SESSION,23,1,DDL,CREATE TABLE,TABLE,public.test,"CREATE
> TABLE public.test
> (
> id INT,
> name TEXT,
> description TEXT,
> CONSTRAINT test_pkey PRIMARY KEY (id)
> );",<none>
> NOTICE: AUDIT: SESSION,23,1,DDL,ALTER TABLE,TABLE,public.test,"CREATE
> TABLE public.test
> (
> id INT,
> name TEXT,
> description TEXT,
> CONSTRAINT test_pkey PRIMARY KEY (id)
> );",<none>
> NOTICE: AUDIT: SESSION,23,1,DDL,CREATE
> INDEX,INDEX,public.test_pkey,"CREATE TABLE public.test
> (
> id INT,
> name TEXT,
> description TEXT,
> CONSTRAINT test_pkey PRIMARY KEY (id)
> );",<none>
>
> Prior to PG17, ProcessUtility was not being called for ALTER TABLE in
> this context. The change probably makes some sense, since the table is
> being created then altered to add the primary key, but since it is a
> behavioral change I wanted to bring it up.
>
> I attempted a bisect to identify the commit that caused this behavioral
> change but pgAudit has been broken since at least November on master and
> didn't get fixed again until bb766cde (April 8). Looking at that commit
> I'm a bit baffled by how it fixed the issue I was seeing, which was that
> an event trigger was firing in the wrong context in the pgAudit tests:
>
> CREATE TABLE tmp (id int, data text);
> +ERROR: not fired by event trigger manager
>
> That error comes out of pgAudit itself:
>
> if (!CALLED_AS_EVENT_TRIGGER(fcinfo))
> elog(ERROR, "not fired by event trigger manager");
>
> Since bb766cde cannot be readily applied to older commits in master I'm
> unable to continue bisecting to find the ALTER TABLE behavioral change.
>
> This seems to leave me with manual code inspection and there have been a
> lot of changes in this area, so I'm hoping somebody will know why this
> ALTER TABLE change happened before I start digging into it.

probably this commit:
https://git.postgresql.org/cgit/postgresql.git/commit/?id=0cd711271d42b0888d36f8eda50e1092c2fed4b3

especially:

- * The parser will create AT_AttSetNotNull subcommands for
- * columns of PRIMARY KEY indexes/constraints, but we need
- * not do anything with them here, because the columns'
- * NOT NULL marks will already have been propagated into
- * the new table definition.
+ * We see this subtype when a primary key is created
+ * internally, for example when it is replaced with a new
+ * constraint (say because one of the columns changes
+ * type); in this case we need to reinstate attnotnull,
+ * because it was removed because of the drop of the old
+ * PK. Schedule this subcommand to an upcoming AT pass.
*/
+ cmd->subtype = AT_SetAttNotNull;
+ tab->subcmds[AT_PASS_OLD_COL_ATTRS] =
+ lappend(tab->subcmds[AT_PASS_OLD_COL_ATTRS], cmd);