Thread: Implicitly created operator family not listed by pg_event_trigger_ddl_commands

Hi,

I have an event trigger firing at command end on "CREATE OPERATOR 
CLASS", "CREATE OPERATOR FAMILY" and every other creation command of 
schema dependent objects.

As stated in the documentation [1], creating an operator class without 
specifying the FAMILY option will create an operator family with the 
same name as the new class if it doesn't already exist. In this case, my 
event trigger is activated through the "CREATE OPERATOR CLASS" tag as 
usual, but pg_event_trigger_ddl_commands() still returns only one row - 
for the operator class creation. I would have expected it to list the 
creation of the operator family as well.

Since event triggers don't catch implicit commands [2], the "CREATE 
OPERATOR FAMILY" tag is useless here and the operator family creation is 
never detected.

I observed this with PostgreSQL 14.2 (Windows 10, Visual C++ build 1914, 
64-bit) and PostgreSQL 10.12 (Windows 10, Visual C++ build 1800, 
64-bit), so it's likely not a version specific issue.

The documentation doesn't say much about pg_event_trigger_ddl_commands() 
[3], but I would assume it's meant to be as exhaustive as possible, 
hence this bug report. By comparison, with an event trigger firing at 
command end on "CREATE TABLE" and a command such as "CREATE TABLE 
my_table (id serial PRIMARY KEY)", pg_event_trigger_ddl_commands() would 
return four rows, including the sequence creation and the index 
creation.

Thanks in advance and sorry if I'm just missing something here.

Regards,

[1] https://www.postgresql.org/docs/14/sql-createopclass.html
[2] I don't think it's documented anywhere, but this seems to be a 
consistant behaviour and it makes sense as long as 
pg_event_trigger_ddl_commands() does the work.
[3] https://www.postgresql.org/docs/14/functions-event-triggers.html

-- 
Leslie Lemaire
Secrétariat général des ministères de la transition écologique, de la
cohésion des territoires et de la mer
Service du numérique

On 2022-May-02, LEMAIRE Leslie (Chargée de mission) - SG/SNUM/UNI/DRC wrote:

> As stated in the documentation [1], creating an operator class without
> specifying the FAMILY option will create an operator family with the same
> name as the new class if it doesn't already exist. In this case, my event
> trigger is activated through the "CREATE OPERATOR CLASS" tag as usual, but
> pg_event_trigger_ddl_commands() still returns only one row - for the
> operator class creation. I would have expected it to list the creation of
> the operator family as well.

I agree, this looks like an omission somewhere -- the extra object
should have been reported.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Wed, May 4, 2022 at 7:17 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-May-02, LEMAIRE Leslie (Chargée de mission) - SG/SNUM/UNI/DRC wrote:
>
> > As stated in the documentation [1], creating an operator class without
> > specifying the FAMILY option will create an operator family with the same
> > name as the new class if it doesn't already exist. In this case, my event
> > trigger is activated through the "CREATE OPERATOR CLASS" tag as usual, but
> > pg_event_trigger_ddl_commands() still returns only one row - for the
> > operator class creation. I would have expected it to list the creation of
> > the operator family as well.
>
> I agree, this looks like an omission somewhere -- the extra object
> should have been reported.

Agreed.

DefineOpClass() calls CreateOpFamily() to create the operator family
but in CreateOpFamily() we don't report the new object to event
triggers. The event by CREATE OPERATOR FAMILY is normally reported by
ProcessUtilitySlow(). I've confirmed this happens on all supported
branches. I've attached a patch to fix it.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Fri, May 06, 2022 at 02:26:42PM +0900, Masahiko Sawada wrote:
> DefineOpClass() calls CreateOpFamily() to create the operator family
> but in CreateOpFamily() we don't report the new object to event
> triggers. The event by CREATE OPERATOR FAMILY is normally reported by
> ProcessUtilitySlow(). I've confirmed this happens on all supported
> branches. I've attached a patch to fix it.

Hmm.  It looks like it makes sense to back-patch that.  That's indeed
a bit surprising to not have an event trigger inform about both.

+-- CRAETE OPERATOR CLASS without FAMILY clause should report
+-- both CRAETE OPERATOR FAMILY and CRAETE OPERATOR CLASS
Got the same typo here, repeated three times.

-           tmpAddr = CreateOpFamily(stmt->amname, opcname,
+           tmpAddr = CreateOpFamily(&opfstmt, stmt->amname, opcname,
                                     namespaceoid, amoid);
CreateOpFamily() does not need its second argument now that you pass
down a CreateOpFamilyStmt as first argument, no?
--
Michael

On Thu, May 19, 2022 at 2:26 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, May 06, 2022 at 02:26:42PM +0900, Masahiko Sawada wrote:
> > DefineOpClass() calls CreateOpFamily() to create the operator family
> > but in CreateOpFamily() we don't report the new object to event
> > triggers. The event by CREATE OPERATOR FAMILY is normally reported by
> > ProcessUtilitySlow(). I've confirmed this happens on all supported
> > branches. I've attached a patch to fix it.
>
> Hmm.  It looks like it makes sense to back-patch that.
>

Yeah, I also don't see a reason to not backpatch this.

>  That's indeed
> a bit surprising to not have an event trigger inform about both.
>
> +-- CRAETE OPERATOR CLASS without FAMILY clause should report
> +-- both CRAETE OPERATOR FAMILY and CRAETE OPERATOR CLASS
> Got the same typo here, repeated three times.
>
> -           tmpAddr = CreateOpFamily(stmt->amname, opcname,
> +           tmpAddr = CreateOpFamily(&opfstmt, stmt->amname, opcname,
>                                      namespaceoid, amoid);
> CreateOpFamily() does not need its second argument now that you pass
> down a CreateOpFamilyStmt as first argument, no?
>

Right, apart from the patch looks good to me.

-- 
With Regards,
Amit Kapila.

On 2022-May-20, Amit Kapila wrote:

> On Thu, May 19, 2022 at 2:26 PM Michael Paquier <michael@paquier.xyz> wrote:

> > CreateOpFamily() does not need its second argument now that you pass
> > down a CreateOpFamilyStmt as first argument, no?
> 
> Right, apart from the patch looks good to me.

+1, like this then.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Fri, May 20, 2022 at 12:34:32PM +0200, Alvaro Herrera wrote:
> +1, like this then.

That seems fine, at quick glance.  Thanks for the patch!
--
Michael

On 2022-May-20, Michael Paquier wrote:

> That seems fine, at quick glance.  Thanks for the patch!

Unfortunately, the tests don't pass in versions 10-13, so I'm
investigating what's up with that.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?"  (Mafalda)

On Fri, May 20, 2022 at 7:57 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-May-20, Michael Paquier wrote:
>
> > That seems fine, at quick glance.  Thanks for the patch!

Thank you for updating the patch!

>
> Unfortunately, the tests don't pass in versions 10-13, so I'm
> investigating what's up with that.

I got the failure when testing in 13:

@@ -429,8 +429,6 @@
 -- CREATE OPERATOR CLASS without FAMILY clause should report
 -- both CREATE OPERATOR FAMILY and CREATE OPERATOR CLASS
 CREATE OPERATOR CLASS evttrigopclass FOR TYPE int USING btree AS STORAGE int;
-NOTICE:  END: command_tag=CREATE OPERATOR FAMILY type=operator family
identity=public.evttrigopclass USING btree
-NOTICE:  END: command_tag=CREATE OPERATOR CLASS type=operator class
identity=public.evttrigopclass USING btree
 DROP EVENT TRIGGER regress_event_trigger_report_dropped;
 -- only allowed from within an event trigger function, should fail
 select pg_event_trigger_table_rewrite_oid();

I think that the event trigger that emits these NOTICE messages
doesn't exist in 13 or older branches. It was added by 2d689babe3c.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On 2022-May-20, Masahiko Sawada wrote:

> I got the failure when testing in 13:
> 
> @@ -429,8 +429,6 @@
>  -- CREATE OPERATOR CLASS without FAMILY clause should report
>  -- both CREATE OPERATOR FAMILY and CREATE OPERATOR CLASS
>  CREATE OPERATOR CLASS evttrigopclass FOR TYPE int USING btree AS STORAGE int;
> -NOTICE:  END: command_tag=CREATE OPERATOR FAMILY type=operator family
> identity=public.evttrigopclass USING btree
> -NOTICE:  END: command_tag=CREATE OPERATOR CLASS type=operator class
> identity=public.evttrigopclass USING btree
>  DROP EVENT TRIGGER regress_event_trigger_report_dropped;
>  -- only allowed from within an event trigger function, should fail
>  select pg_event_trigger_table_rewrite_oid();
> 
> I think that the event trigger that emits these NOTICE messages
> doesn't exist in 13 or older branches. It was added by 2d689babe3c.

Oh, hahah.  Hmm, I feel inclined to add the trigger rather than remove
the lines from the expected output.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)

On 2022-May-20, Alvaro Herrera wrote:

> On 2022-May-20, Masahiko Sawada wrote:
> 
> > I think that the event trigger that emits these NOTICE messages
> > doesn't exist in 13 or older branches. It was added by 2d689babe3c.
> 
> Oh, hahah.  Hmm, I feel inclined to add the trigger rather than remove
> the lines from the expected output.

So I'm inclined to add the attached patch before the bugfix, in branches
10-13.  It simply adds the new event trigger to the existing test case,
being careful not to trip on the error message that it would generate
otherwise, for a bug that was only fixed in 14.

The other option, of course, is to not backpatch any of this earlier
than 14.  But the OP says she was experimenting with version 10
initially.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Java is clearly an example of money oriented programming"  (A. Stepanov)

On Fri, May 20, 2022 at 05:23:19PM +0200, Alvaro Herrera wrote:
> The other option, of course, is to not backpatch any of this earlier
> than 14.  But the OP says she was experimenting with version 10
> initially.

Nah.  I think that you are right to have done that down to 10 with the
extra test coming from 14~.  Thanks for fixing the issue!
--
Michael

On Sat, May 21, 2022 at 12:23 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-May-20, Alvaro Herrera wrote:
>
> > On 2022-May-20, Masahiko Sawada wrote:
> >
> > > I think that the event trigger that emits these NOTICE messages
> > > doesn't exist in 13 or older branches. It was added by 2d689babe3c.
> >
> > Oh, hahah.  Hmm, I feel inclined to add the trigger rather than remove
> > the lines from the expected output.
>
> So I'm inclined to add the attached patch before the bugfix, in branches
> 10-13.  It simply adds the new event trigger to the existing test case,
> being careful not to trip on the error message that it would generate
> otherwise, for a bug that was only fixed in 14.

+1

The patch looks good to me.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/