Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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 signature.asc Description: PGP signature
Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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
Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 6d88b690d8..ad9740098e 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -5607,10 +5607,7 @@ getObjectIdentityParts(const ObjectAddress *object,
{
HeapTuple tup;
Form_pg_event_trigger trigForm;
-
-/* no objname support here */
-if (objname)
- *objname = NIL;
+char *evtname;
tup = SearchSysCache1(EVENTTRIGGEROID,
ObjectIdGetDatum(object->objectId));
@@ -5622,8 +5619,10 @@ getObjectIdentityParts(const ObjectAddress *object,
break;
}
trigForm = (Form_pg_event_trigger) GETSTRUCT(tup);
-appendStringInfoString(,
- quote_identifier(NameStr(trigForm->evtname)));
+evtname = NameStr(trigForm->evtname);
+appendStringInfoString(, quote_identifier(evtname));
+if (objname)
+ *objname = list_make1(evtname);
ReleaseSysCache(tup);
break;
}
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index bdd0ffcdaf..1aca794d6d 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -533,6 +533,23 @@ DROP POLICY p2 ON event_trigger_test;
NOTICE: DROP POLICY - ddl_command_start
NOTICE: DROP POLICY - sql_drop
NOTICE: DROP POLICY - ddl_command_end
+-- Check the object address of those event triggers
+SELECT
+e.evtname,
+pg_describe_object('pg_event_trigger'::regclass, e.oid, 0) as descr,
+b.type, b.object_names, b.object_args,
+pg_identify_object(a.classid, a.objid, a.objsubid) as ident
+ FROM pg_event_trigger as e,
+LATERAL pg_identify_object_as_address('pg_event_trigger'::regclass, e.oid, 0) as b,
+LATERAL pg_get_object_address(b.type, b.object_names, b.object_args) as a
+ ORDER BY e.evtname;
+ evtname | descr | type |object_names | object_args | ident
+---+-+---+-+-+
+ end_rls_command | event trigger end_rls_command | event trigger | {end_rls_command} | {} | ("event trigger",,end_rls_command,end_rls_command)
+ sql_drop_command | event trigger sql_drop_command | event trigger | {sql_drop_command} | {} | ("event trigger",,sql_drop_command,sql_drop_command)
+ start_rls_command | event trigger start_rls_command | event trigger | {start_rls_command} | {} | ("event trigger",,start_rls_command,start_rls_command)
+(3 rows)
+
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index 18b2a267cb..f7347ffbb1 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -426,6 +426,17 @@ ALTER POLICY p1 ON event_trigger_test USING (TRUE);
ALTER POLICY p1 ON event_trigger_test RENAME TO p2;
DROP POLICY p2 ON event_trigger_test;
+-- Check the object address of those event triggers
+SELECT
+e.evtname,
+pg_describe_object('pg_event_trigger'::regclass, e.oid, 0) as descr,
+b.type, b.object_names, b.object_args,
+pg_identify_object(a.classid, a.objid, a.objsubid) as ident
+ FROM pg_event_trigger as e,
+LATERAL pg_identify_object_as_address('pg_event_trigger'::regclass, e.oid, 0) as b,
+LATERAL pg_get_object_address(b.type, b.object_names, b.object_args) as a
+ ORDER BY e.evtname;
+
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
signature.asc
Description: PGP signature
Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
On Tue, 27 Apr 2021 at 13:16, Joel Jacobson 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.
Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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 fix_event_trigger_pg_identify_object_as_address2.patch Description: Binary data
Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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 signature.asc Description: PGP signature
Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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
Re: [PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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
[PATCH] Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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
fix-pg_identify_object_as_address-for-event-triggers.patch
Description: Binary data
Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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
Re: pg_identify_object_as_address() doesn't support pg_event_trigger oids
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 Herrera39°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!"
pg_identify_object_as_address() doesn't support pg_event_trigger oids
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
