Обсуждение: CREATE TABLE/ProcessUtility hook behavior change
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.
Regards,
-David
On Tue, Apr 30, 2024 at 10:26 AM David Steele <david@pgmasters.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);
On 4/30/24 12:57, jian he wrote: > On Tue, Apr 30, 2024 at 10:26 AM David Steele <david@pgmasters.net> wrote: >> >> 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 Hmm, I don't think so since the problem already existed in bb766cde, which was committed on Apr 8 vs Apr 19 for the above patch. Probably I'll need to figure out which exact part of bb766cde fixes the event trigger issue so I can backpatch just that part and continue bisecting. Regards, -David
On Tue, Apr 30, 2024 at 4:35 PM David Steele <david@pgmasters.net> wrote: > > On 4/30/24 12:57, jian he wrote: > > On Tue, Apr 30, 2024 at 10:26 AM David Steele <david@pgmasters.net> wrote: > >> > >> 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. > > I just tested these two commits. https://git.postgresql.org/cgit/postgresql.git/commit/?id=3da13a6257bc08b1d402c83feb2a35450f988365 https://git.postgresql.org/cgit/postgresql.git/commit/?id=b0e96f311985bceba79825214f8e43f65afa653a i think it's related to the catalog not null commit. it will alter table and add not null constraint internally (equivalent to `alter table test alter id set not null`).
On 4/30/24 21:15, jian he wrote: > On Tue, Apr 30, 2024 at 4:35 PM David Steele <david@pgmasters.net> wrote: >> >> On 4/30/24 12:57, jian he wrote: >>> On Tue, Apr 30, 2024 at 10:26 AM David Steele <david@pgmasters.net> wrote: >>>> >>>> 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. >>> > > I just tested these two commits. > https://git.postgresql.org/cgit/postgresql.git/commit/?id=3da13a6257bc08b1d402c83feb2a35450f988365 > https://git.postgresql.org/cgit/postgresql.git/commit/?id=b0e96f311985bceba79825214f8e43f65afa653a > > i think it's related to the catalog not null commit. > it will alter table and add not null constraint internally (equivalent > to `alter table test alter id set not null`). Yeah, looks like b0e96f31 was the culprit. Now that it has been reverted in 6f8bb7c1 the pgAudit output is back to expected. To be clear, I'm not saying this (now reverted) behavior was a bug, but it was certainly a change in behavior so I thought it was worth pointing out. Anyway, I guess we'll see how this goes in the next dev cycle. Thanks for helping me track that down! Regards, -David