Horton

Thread

Commits

  1. Fix pg_identify_object_as_address() with event triggers

  1. pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Joel Jacobson <joel@compiler.org> — 2021-04-22T17:11:25Z

    Hi,

Is $SUBJECT intentional, or would it be desirable to add support it?

Example:

SELECT * FROM pg_catalog.pg_event_trigger;
    oid    |    evtname    |    evtevent     | evtowner |  evtfoid  | evtenabled | evttags
-----------+---------------+-----------------+----------+-----------+------------+---------
289361636 | ddl_postgrest | ddl_command_end |    16696 | 289361635 | O          |
(1 row)

SELECT * FROM pg_identify_object_as_address('pg_event_trigger'::regclass,289361636,0);
ERROR:  requested object address for unsupported object class 32: text result "ddl_postgrest"

/Joel

  2. Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-22T17:32:48Z

    On 2021-Apr-22, Joel Jacobson wrote:

> Is $SUBJECT intentional, or would it be desirable to add support it?
> 
> Example:
> 
> SELECT * FROM pg_catalog.pg_event_trigger;
>     oid    |    evtname    |    evtevent     | evtowner |  evtfoid  | evtenabled | evttags
> -----------+---------------+-----------------+----------+-----------+------------+---------
> 289361636 | ddl_postgrest | ddl_command_end |    16696 | 289361635 | O          |
> (1 row)
> 
> SELECT * FROM pg_identify_object_as_address('pg_event_trigger'::regclass,289361636,0);
> ERROR:  requested object address for unsupported object class 32: text result "ddl_postgrest"

Hmm, I think this is an accidental omission and it should be supported.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"In Europe they call me Niklaus Wirth; in the US they call me Nickel's worth.
 That's because in Europe they call me by name, and in the US by value!"

  3. Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Joel Jacobson <joel@compiler.org> — 2021-04-23T06:54:45Z

    On Thu, Apr 22, 2021, at 19:32, Alvaro Herrera wrote:
> On 2021-Apr-22, Joel Jacobson wrote:
> > SELECT * FROM pg_identify_object_as_address('pg_event_trigger'::regclass,289361636,0);
> > ERROR:  requested object address for unsupported object class 32: text result "ddl_postgrest"
> 
> Hmm, I think this is an accidental omission and it should be supported.

Oh, I realise now the error came from a server running v13,
but there seems to be a problem in HEAD as well;
the "object_names" text[] output is empty for event triggers,
so the output will be the same for all event triggers,
which doesn't seem right since the output should be unique.

The output from the other functions pg_describe_object() and pg_identify_object()
contain the name in the output though.

Example:

SELECT
  *,
  pg_describe_object('pg_event_trigger'::regclass,oid,0),
  pg_identify_object('pg_event_trigger'::regclass,oid,0),
  pg_identify_object_as_address('pg_event_trigger'::regclass,oid,0)
FROM pg_event_trigger;
-[ RECORD 1 ]-----------------+-----------------------------------------------
oid                           | 396715
evtname                       | ddl_postgrest
evtevent                      | ddl_command_end
evtowner                      | 10
evtfoid                       | 396714
evtenabled                    | O
evttags                       |
pg_describe_object            | event trigger ddl_postgrest
pg_identify_object            | ("event trigger",,ddl_postgrest,ddl_postgrest)
pg_identify_object_as_address | ("event trigger",{},{})

I therefore think the evtname should be added to object_names.

/Joel

  4. [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Joel Jacobson <joel@compiler.org> — 2021-04-23T07:30:47Z

    On Fri, Apr 23, 2021, at 08:54, Joel Jacobson wrote:
> pg_describe_object            | event trigger ddl_postgrest
> pg_identify_object            | ("event trigger",,ddl_postgrest,ddl_postgrest)
> pg_identify_object_as_address | ("event trigger",{},{})
> 
> I therefore think the evtname should be added to object_names.

Could it possibly be as simple to fix as the attached patch?
Not sure if the the string constructed by appendStringInfo() also needs to be adjusted.

With the patch, the example above returns:

pg_identify_object_as_address | ("event trigger",{ddl_postgrest},{})

/Joel

  5. Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Joel Jacobson <joel@compiler.org> — 2021-04-23T07:33:36Z

    On Fri, Apr 23, 2021, at 09:30, Joel Jacobson wrote:
> fix-pg_identify_object_as_address-for-event-triggers.patch

Also, since this is a problem also in v13 maybe this should also be back-ported?
I think it's a bug since both pg_identify_object_as_address() and event triggers exists in v13,
so the function should work there as well, otherwise users would need to do work-arounds for event triggers.

/Joel

  6. Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Joel Jacobson <joel@compiler.org> — 2021-04-24T06:08:04Z

    On Thu, Apr 22, 2021, at 19:32, Alvaro Herrera wrote:
> On 2021-Apr-22, Joel Jacobson wrote:
> 
> > Is $SUBJECT intentional, or would it be desirable to add support it?
> > 
> > Example:
> > 
> > SELECT * FROM pg_catalog.pg_event_trigger;
> >     oid    |    evtname    |    evtevent     | evtowner |  evtfoid  | evtenabled | evttags
> > -----------+---------------+-----------------+----------+-----------+------------+---------
> > 289361636 | ddl_postgrest | ddl_command_end |    16696 | 289361635 | O          |
> > (1 row)
> > 
> > SELECT * FROM pg_identify_object_as_address('pg_event_trigger'::regclass,289361636,0);
> > ERROR:  requested object address for unsupported object class 32: text result "ddl_postgrest"
> 
> Hmm, I think this is an accidental omission and it should be supported.

I've added the patch to the commitfest and added you as a reviewer, hope that works.

/Joel

  7. Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Michael Paquier <michael@paquier.xyz> — 2021-04-26T08:30:25Z

    On Fri, Apr 23, 2021 at 09:33:36AM +0200, Joel Jacobson wrote:
> Also, since this is a problem also in v13 maybe this should also be
> back-ported?  I think it's a bug since both
> pg_identify_object_as_address() and event triggers exists in v13, so
> the function should work there as well, otherwise users would need
> to do work-arounds for event triggers.  

No objections from here to do something in back-branches.  We cannot
have a test for event triggers in object_address.sql and it would be
better to keep it in a parallel set (see 676858b for example).  Could
you however add a small test for that in event_trigger.sql?  It would
be good to check after all three functions pg_identify_object(),
pg_identify_object_as_address() and pg_get_object_address().
--
Michael

  8. Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Joel Jacobson <joel@compiler.org> — 2021-04-27T05:16:25Z

    On Mon, Apr 26, 2021, at 10:30, Michael Paquier wrote:
> On Fri, Apr 23, 2021 at 09:33:36AM +0200, Joel Jacobson wrote:
> > Also, since this is a problem also in v13 maybe this should also be
> > back-ported?  I think it's a bug since both
> > pg_identify_object_as_address() and event triggers exists in v13, so
> > the function should work there as well, otherwise users would need
> > to do work-arounds for event triggers.  
> 
> No objections from here to do something in back-branches.  We cannot
> have a test for event triggers in object_address.sql and it would be
> better to keep it in a parallel set (see 676858b for example).  Could
> you however add a small test for that in event_trigger.sql?  It would
> be good to check after all three functions pg_identify_object(),
> pg_identify_object_as_address() and pg_get_object_address().
> --
> Michael

Thanks for the guidance in how to test.

I've added a test at the end of event_trigger.sql,
reusing the three event triggers already in existence,
just before they are dropped.

New patch attached.

/Joel

  9. Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Japin Li <japinli@hotmail.com> — 2021-04-27T05:46:41Z

    On Tue, 27 Apr 2021 at 13:16, Joel Jacobson <joel@compiler.org> wrote:
> On Mon, Apr 26, 2021, at 10:30, Michael Paquier wrote:
>> On Fri, Apr 23, 2021 at 09:33:36AM +0200, Joel Jacobson wrote:
>> > Also, since this is a problem also in v13 maybe this should also be
>> > back-ported?  I think it's a bug since both
>> > pg_identify_object_as_address() and event triggers exists in v13, so
>> > the function should work there as well, otherwise users would need
>> > to do work-arounds for event triggers.  
>> 
>> No objections from here to do something in back-branches.  We cannot
>> have a test for event triggers in object_address.sql and it would be
>> better to keep it in a parallel set (see 676858b for example).  Could
>> you however add a small test for that in event_trigger.sql?  It would
>> be good to check after all three functions pg_identify_object(),
>> pg_identify_object_as_address() and pg_get_object_address().
>> --
>> Michael
>
> Thanks for the guidance in how to test.
>
> I've added a test at the end of event_trigger.sql,
> reusing the three event triggers already in existence,
> just before they are dropped.
>
> New patch attached.

IMO we should add a space between the parameters to keep the code
style consistently.

+SELECT
+  evtname,
+  pg_describe_object('pg_event_trigger'::regclass,oid,0),
+  pg_identify_object('pg_event_trigger'::regclass,oid,0),
+  pg_identify_object_as_address('pg_event_trigger'::regclass,oid,0)
+FROM pg_event_trigger
+WHERE evtname IN ('start_rls_command','end_rls_command','sql_drop_command')
+ORDER BY evtname;

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

  10. Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Michael Paquier <michael@paquier.xyz> — 2021-04-27T07:48:07Z

    On Tue, Apr 27, 2021 at 07:16:25AM +0200, Joel Jacobson wrote:
> I've added a test at the end of event_trigger.sql,
> reusing the three event triggers already in existence,
> just before they are dropped.

Cool, thanks.  I have been looking at it and I'd still like to
cross-check the output data of pg_get_object_address() to see if
pg_identify_object() remains consistent through it.  See for example
the attached that uses a trick based on LATERAL, a bit different than
what's done in object_address.sql but that gives the same amount of
coverage (I could also use two ROW()s and an equality, but well..).
--
Michael

  11. Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Joel Jacobson <joel@compiler.org> — 2021-04-27T12:33:36Z

    On Tue, Apr 27, 2021, at 09:48, Michael Paquier wrote:
> On Tue, Apr 27, 2021 at 07:16:25AM +0200, Joel Jacobson wrote:
> > I've added a test at the end of event_trigger.sql,
> > reusing the three event triggers already in existence,
> > just before they are dropped.
> 
> Cool, thanks.  I have been looking at it and I'd still like to
> cross-check the output data of pg_get_object_address() to see if
> pg_identify_object() remains consistent through it.  See for example
> the attached that uses a trick based on LATERAL, a bit different than
> what's done in object_address.sql but that gives the same amount of
> coverage (I could also use two ROW()s and an equality, but well..).

Neat trick, looks good to me.

I've successfully tested fix_event_trigger_pg_identify_object_as_address3.patch.

/Joel

  12. Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids

    Michael Paquier <michael@paquier.xyz> — 2021-04-28T03:08:45Z

    On Tue, Apr 27, 2021 at 02:33:36PM +0200, Joel Jacobson wrote:
> I've successfully tested fix_event_trigger_pg_identify_object_as_address3.patch.

Thanks.  Applied down to 9.6 then.
--
Michael