Fix DROP PROPERTY GRAPH "unsupported object class" error

On Wed, Apr 22, 2026 at 04:19:26PM +0000, Bertrand Drouvot wrote:

Indeed, getObjectTypeDescription() and getObjectIdentityParts() are missing switch
cases for PropgraphElementLabelRelationId and PropgraphLabelPropertyRelationId,
causing DROP PROPERTY GRAPH to hit the default case and error out with
"unsupported object class".

Hmm. Couldn't these code paths be reached as well with the object
functions like pg_describe_object(), pg_get_object_address(),
pg_identify_object_as_address() or pg_identify_object()? Object
descriptions usually stick within object_address.sql. The new objects
you would want to stick should be covered as well in this test suite,
and the file already has some property graphs in it.

The bug only manifests when an event trigger is active, because that is what
calls these functions.

--- a/src/test/regress/expected/create_property_graph.out
+++ b/src/test/regress/expected/create_property_graph.out
[...]
+CREATE EVENT TRIGGER dpg_evt ON ddl_command_end EXECUTE FUNCTION dpg_evt_func();

Event triggers are avoided in parallel groups because they are not
reliable (see also fast_default), and we should avoid what you are
doing in this test.

The attached adds the missing cases so that DROP PROPERTY GRAPH, DROP PROPERTY GRAPH
IF EXISTS, and DROP SCHEMA CASCADE on schemas containing property graphs all work
correctly when event triggers are present.

It also adds test cases that create an event trigger and then exercise DROP PROPERTY
GRAPH and DROP SCHEMA CASCADE with property graphs.

I think that's worth an open item and I'll add one for this issue.

This should be an open item, I guess, yes. Could you add one? Even
if Peter discards the issue at the end, the issue still needs to be
discussed so we had better to track it anyway.
--
Michael

On Thu, Apr 23, 2026 at 12:09 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Apr 22, 2026 at 04:19:26PM +0000, Bertrand Drouvot wrote:

Indeed, getObjectTypeDescription() and getObjectIdentityParts() are missing switch
cases for PropgraphElementLabelRelationId and PropgraphLabelPropertyRelationId,
causing DROP PROPERTY GRAPH to hit the default case and error out with
"unsupported object class".

Hmm. Couldn't these code paths be reached as well with the object
functions like pg_describe_object(), pg_get_object_address(),
pg_identify_object_as_address() or pg_identify_object()? Object
descriptions usually stick within object_address.sql. The new objects
you would want to stick should be covered as well in this test suite,
and the file already has some property graphs in it.

+1. See emails around [1]/messages/by-id/CAExHW5sgVFHWYkcxZjH0LF4Qbyx2Zyri5ZLave7tZdMbvTLiqg@mail.gmail.com for some discussion about existing property
graph object definitions.

The bug only manifests when an event trigger is active, because that is what
calls these functions.

--- a/src/test/regress/expected/create_property_graph.out
+++ b/src/test/regress/expected/create_property_graph.out
[...]
+CREATE EVENT TRIGGER dpg_evt ON ddl_command_end EXECUTE FUNCTION dpg_evt_func();

Event triggers are avoided in parallel groups because they are not
reliable (see also fast_default), and we should avoid what you are
doing in this test.

The attached adds the missing cases so that DROP PROPERTY GRAPH, DROP PROPERTY GRAPH
IF EXISTS, and DROP SCHEMA CASCADE on schemas containing property graphs all work
correctly when event triggers are present.

It also adds test cases that create an event trigger and then exercise DROP PROPERTY
GRAPH and DROP SCHEMA CASCADE with property graphs.

There are already tests for DROP PROPERTY GRAPH and DROP PROPERTY
GRAPH IF EXISTS.

We don't have DROP SCHEMA CASCADE tests in create_table.sql or
create_function.sql. It seems like we don't explicitly test it for
every object separately. Why do we want to add it for property graphs
then?

I could reproduce your issue with a smaller change to the file

diff --git a/src/test/regress/sql/create_property_graph.sql
b/src/test/regress/sql/create_property_graph.sql
index 4cf771596a8..d9cbdf9f1a9 100644
--- a/src/test/regress/sql/create_property_graph.sql
+++ b/src/test/regress/sql/create_property_graph.sql
@@ -151,6 +151,11 @@ CREATE PROPERTY GRAPH gx
         t1x KEY (a) PROPERTIES (b::varchar(20) AS p1),
         t2x KEY (i) PROPERTIES (j::varchar(20) AS p1)  -- matching
typmods by casting works
     );
+CREATE FUNCTION gx_evt_func() RETURNS event_trigger
+LANGUAGE plpgsql AS $$
+BEGIN END;
+$$;
+CREATE EVENT TRIGGER gx_evt ON ddl_command_end EXECUTE FUNCTION gx_evt_func();
 DROP PROPERTY GRAPH gx;
 DROP TABLE t1x, t2x;

That covers everything you want to test and its minimal change to the
test. But I see that the event triggers for all objects are tested in
event_triggers.sql. So I think the new test should be added to that
file.

I think that's worth an open item and I'll add one for this issue.

This should be an open item, I guess, yes. Could you add one? Even
if Peter discards the issue at the end, the issue still needs to be
discussed so we had better to track it anyway.

Element label, label property are not user visible objects per say, so
I am not sure whether the code changes are in the right direction. But
it's also true that we shouldn't get an error in the presence of an
event trigger. We need to fix that.

[1]: /messages/by-id/CAExHW5sgVFHWYkcxZjH0LF4Qbyx2Zyri5ZLave7tZdMbvTLiqg@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat

Hi,

On Thu, Apr 23, 2026 at 12:57:37PM +0530, Ashutosh Bapat wrote:

On Thu, Apr 23, 2026 at 12:09 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Apr 22, 2026 at 04:19:26PM +0000, Bertrand Drouvot wrote:

Indeed, getObjectTypeDescription() and getObjectIdentityParts() are missing switch
cases for PropgraphElementLabelRelationId and PropgraphLabelPropertyRelationId,
causing DROP PROPERTY GRAPH to hit the default case and error out with
"unsupported object class".

Hmm. Couldn't these code paths be reached as well with the object
functions like pg_describe_object(), pg_get_object_address(),
pg_identify_object_as_address() or pg_identify_object()? Object
descriptions usually stick within object_address.sql. The new objects
you would want to stick should be covered as well in this test suite,
and the file already has some property graphs in it.

+1. See emails around [1] for some discussion about existing property
graph object definitions.

Yeah that makes sense to also add some tests here, done in the attached.

The bug only manifests when an event trigger is active, because that is what
calls these functions.

That covers everything you want to test and its minimal change to the
test. But I see that the event triggers for all objects are tested in
event_triggers.sql. So I think the new test should be added to that
file.

A simpler test has been done in event_trigger.sql instead.

I think that's worth an open item and I'll add one for this issue.

This should be an open item, I guess, yes. Could you add one?

One has already been created (I think you need to be logged in to see the
updates).

Element label, label property are not user visible objects per say, so
I am not sure whether the code changes are in the right direction. But
it's also true that we shouldn't get an error in the presence of an
event trigger. We need to fix that.

Agreed.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Thu, Apr 23, 2026 at 4:31 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Thu, Apr 23, 2026 at 12:57:37PM +0530, Ashutosh Bapat wrote:

On Thu, Apr 23, 2026 at 12:09 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Apr 22, 2026 at 04:19:26PM +0000, Bertrand Drouvot wrote:

Indeed, getObjectTypeDescription() and getObjectIdentityParts() are missing switch
cases for PropgraphElementLabelRelationId and PropgraphLabelPropertyRelationId,
causing DROP PROPERTY GRAPH to hit the default case and error out with
"unsupported object class".

Hmm. Couldn't these code paths be reached as well with the object
functions like pg_describe_object(), pg_get_object_address(),
pg_identify_object_as_address() or pg_identify_object()? Object
descriptions usually stick within object_address.sql. The new objects
you would want to stick should be covered as well in this test suite,
and the file already has some property graphs in it.

+1. See emails around [1] for some discussion about existing property
graph object definitions.

Yeah that makes sense to also add some tests here, done in the attached.

The bug only manifests when an event trigger is active, because that is what
calls these functions.

That covers everything you want to test and its minimal change to the
test. But I see that the event triggers for all objects are tested in
event_triggers.sql. So I think the new test should be added to that
file.

A simpler test has been done in event_trigger.sql instead.

I think that's worth an open item and I'll add one for this issue.

This should be an open item, I guess, yes. Could you add one?

One has already been created (I think you need to be logged in to see the
updates).

Element label, label property are not user visible objects per say, so
I am not sure whether the code changes are in the right direction. But
it's also true that we shouldn't get an error in the presence of an
event trigger. We need to fix that.

Agreed.

I was wrong when I said that element label and label property are not
user visible objects. Sorry.

They are very much user visible and can be operated upon separately.
For example ALTER PROPERTY GRAPH ... ALTER VERTEX TABLE ... ALTER
LABEL ... DROP PROPERTIES ... drop label property objects. Similarly
for ALTER PROPERTY GRAPH ... ALTER VERTEX TABLE ... DROP LABEL ... .
So your code changes are needed. However I think the test cases added
in the patch are not sufficient.
1. Earlier in object_address.sql there are instances of property graph
element, property graph property etc. But I don't see property graph
element label and property graph label property there.
2. In create_graph_table.sql there are tests for pg_describe_object(),
pg_identify_object_as_address() and pg_identify_object() for property
graph property, property graph element and property graph label
objects. But I don't see tests added for the objects covered by the
patch.

For create_graph_table.sql I think what we need to do is add a
RECURSIVE CTE like
WITH RECURSIVE deps (classid, objid, objsubid, refclassid, refobjid,
refobjsubid) AS
(
SELECT classid, objid, objsubid,
refclassid, refobjid, refobjsubid
FROM pg_depend
WHERE refclassid = 'pg_class'::regclass AND
refobjid = 'create_property_graph_tests.g2'::regclass

UNION ALL

SELECT d.classid, d.objid, d.objsubid,
d.refclassid, d.refobjid, d.refobjsubid
FROM pg_depend d
JOIN deps dp ON d.refclassid = dp.classid AND d.refobjid =
dp.objid AND d.refobjsubid = dp.objsubid
)
SELECT pg_describe_object(classid, objid, objsubid) as obj,
pg_describe_object(refclassid, refobjid, refobjsubid) as reference_graph
FROM deps
ORDER BY 1, 2;

for each of the above functions. This query traverses the dependency
tree thus covering every object that can appear in a property graph.
For 'create_property_graph_tests.g2' the output of the above query
100 rows long which doesn't seem to be worth the code coverage we get.
I guess, we need to choose a property graph with a smaller dependency
tree like gt. I haven't examined whether that graph would cover all
the cases, but it will certainly cover all the objects.

I think the proper description of property graph label property object
is property graph element label property since we are reporting
property of an element through a label. But then that means the
description would be inconsistent with the catalog name and adding
"element" to the catalog name would make it much longer. I am not able
to decide whether to add "element" in the description or not, ATM.

--
Best Wishes,
Ashutosh Bapat

Hi,

On Fri, Apr 24, 2026 at 05:37:56PM +0530, Ashutosh Bapat wrote:

So your code changes are needed. However I think the test cases added
in the patch are not sufficient.
1. Earlier in object_address.sql there are instances of property graph
element, property graph property etc. But I don't see property graph
element label and property graph label property there.

I did not add them because they would not produce an error without the
patch in place. That said, that's probably better to add them for consistency,
done in the attached.

2. In create_graph_table.sql there are tests for pg_describe_object(),
pg_identify_object_as_address() and pg_identify_object() for property
graph property, property graph element and property graph label
objects. But I don't see tests added for the objects covered by the
patch.

For create_graph_table.sql I think what we need to do is add a
RECURSIVE CTE like
WITH RECURSIVE deps (classid, objid, objsubid, refclassid, refobjid,
refobjsubid) AS
(
SELECT classid, objid, objsubid,
refclassid, refobjid, refobjsubid
FROM pg_depend
WHERE refclassid = 'pg_class'::regclass AND
refobjid = 'create_property_graph_tests.g2'::regclass

UNION ALL

SELECT d.classid, d.objid, d.objsubid,
d.refclassid, d.refobjid, d.refobjsubid
FROM pg_depend d
JOIN deps dp ON d.refclassid = dp.classid AND d.refobjid =
dp.objid AND d.refobjsubid = dp.objsubid
)
SELECT pg_describe_object(classid, objid, objsubid) as obj,
pg_describe_object(refclassid, refobjid, refobjsubid) as reference_graph
FROM deps
ORDER BY 1, 2;

for each of the above functions. This query traverses the dependency
tree thus covering every object that can appear in a property graph.
For 'create_property_graph_tests.g2' the output of the above query
100 rows long which doesn't seem to be worth the code coverage we get.
I guess, we need to choose a property graph with a smaller dependency
tree like gt. I haven't examined whether that graph would cover all
the cases, but it will certainly cover all the objects.

Yeah, gt is enough to cover all the objects. v3 attached makes use of it and
1/ get rid of the "reference_graph" as it's not needed for the test and 2/
use COLLATE "C" in the order by to be on the safe side of things.

I think the proper description of property graph label property object
is property graph element label property since we are reporting
property of an element through a label. But then that means the
description would be inconsistent with the catalog name and adding
"element" to the catalog name would make it much longer. I am not able
to decide whether to add "element" in the description or not, ATM.

I think it's better to be consistent with the current catalog here to stay
focused on the main purpose of this patch.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, Apr 24, 2026 at 7:48 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

for each of the above functions. This query traverses the dependency
tree thus covering every object that can appear in a property graph.
For 'create_property_graph_tests.g2' the output of the above query
100 rows long which doesn't seem to be worth the code coverage we get.
I guess, we need to choose a property graph with a smaller dependency
tree like gt. I haven't examined whether that graph would cover all
the cases, but it will certainly cover all the objects.

Yeah, gt is enough to cover all the objects. v3 attached makes use of it and
1/ get rid of the "reference_graph" as it's not needed for the test and 2/
use COLLATE "C" in the order by to be on the safe side of things.

I was thinking of modifying the existing queries as attached.

I think COLLATE "C" is safer, however, it doesn't go well with indexes
in ORDER BY, which make this query easy to write. (see [1]/messages/by-id/2173dc58-6697-1e10-9604-a7c9f2a8bb55@lab.ntt.co.jp). So far we
haven't seen any buildfarm failures so far with the current ORDER BY.
That makes me think that the query output will be stable even without
COLLATE "C". So keeping it that way. But we can add COLLATE "C" if we
see buildfarm instability.

I think the proper description of property graph label property object
is property graph element label property since we are reporting
property of an element through a label. But then that means the
description would be inconsistent with the catalog name and adding
"element" to the catalog name would make it much longer. I am not able
to decide whether to add "element" in the description or not, ATM.

I think it's better to be consistent with the current catalog here to stay
focused on the main purpose of this patch.

Though getObjectTypeDescription() usually prints the description which
is closer to the name of the catalog, that's not always true. E.g
AccessMethodProcedureRelationId maps to "function of access method",
StatisticExtRelationId maps to "statistics object". I think "property
graph element label property" is more appropriate for
PropgraphLabelPropertyRelationId since the property is associated with
the label which in turn is associated with the element. Attached patch
has that change. If Peter feels "property graph label property" is
better, we will use that.

Some more changes as listed below
1. In event_trigger.sql I have modified the names of the tables a bit
so that I can also add an edge element and then test ALTER PROPERTY
GRAPH as well.
2. Used get_catalog_object_by_oid(), which appropriately uses the
cache or table scan in getObjectIdentityParts(). It performs an extra
lookup but I think that's ok, since the latter function is not in a
hot path. I wonder whether get_catalog_object_by_oid() is intended to
be called whenever we want to get the catalog tuple by OID instead of
directly calling sys cache lookup function or catalog scan. At least
the new code you are adding can make use of this function.
3. The cases in getObjectTypeDescription() and
getObjectIdentityParts() are arranged roughly in alphabetical order,
but not exactly. I took some liberty to rearrange the property graph
related cases such that the cases directly dependent on property graph
appear first in the alphabetical order followed by those dependent
through one hop and then those through two hops. That way the code
appears in the order of their dependency and is easy to read. Also
arranged the new entries in objectaddress.sql to follow that order as
per a comment in that file. The way you had arranged it appeared in
alphabetical order if we considered RelationId to be part of the name
not otherwise.
4. getObjectIdentityParts() should be closer to getObjectDescription()
when creating the array of object names and also the identity. For
example getObjectIdentityParts() constructs identity of the property
as "a of create_property_graph_tests.gt" losing the name of the label
whereas getObjectDescription() constructs it as property "a of label
v1 of vertex v1 of property graph gt". Fixed that as well.

[1]: /messages/by-id/2173dc58-6697-1e10-9604-a7c9f2a8bb55@lab.ntt.co.jp

--
Best Wishes,
Ashutosh Bapat

Hi,

On Wed, Apr 29, 2026 at 06:02:57PM +0530, Ashutosh Bapat wrote:

On Fri, Apr 24, 2026 at 7:48 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

for each of the above functions. This query traverses the dependency
tree thus covering every object that can appear in a property graph.
For 'create_property_graph_tests.g2' the output of the above query
100 rows long which doesn't seem to be worth the code coverage we get.
I guess, we need to choose a property graph with a smaller dependency
tree like gt. I haven't examined whether that graph would cover all
the cases, but it will certainly cover all the objects.

1/ get rid of the "reference_graph" as it's not needed for the test and 2/
use COLLATE "C" in the order by to be on the safe side of things.

I was thinking of modifying the existing queries as attached.

Works for me.

I think COLLATE "C" is safer, however, it doesn't go well with indexes
in ORDER BY, which make this query easy to write. (see [1]). So far we
haven't seen any buildfarm failures so far with the current ORDER BY.
That makes me think that the query output will be stable even without
COLLATE "C". So keeping it that way. But we can add COLLATE "C" if we
see buildfarm instability.

I can see the the instability locally with en_US.UTF-8 (gt before _gt) and I
can see that, for example sifaka, has en_US.UTF-8 in locales. So, modifying the
impacted query a bit to add COLLATE "C" in the attached.

2. Used get_catalog_object_by_oid(), which appropriately uses the
cache or table scan in getObjectIdentityParts(). It performs an extra
lookup but I think that's ok, since the latter function is not in a
hot path. I wonder whether get_catalog_object_by_oid() is intended to
be called whenever we want to get the catalog tuple by OID instead of
directly calling sys cache lookup function or catalog scan. At least
the new code you are adding can make use of this function.

v3 was using the syscan to be consistent with what getObjectDescription() was
doing. That makes sense to me to be consistent, so in passing v5 makes use of
get_catalog_object_by_oid() in getObjectDescription() too.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Thu, Apr 30, 2026 at 4:56 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

I think COLLATE "C" is safer, however, it doesn't go well with indexes
in ORDER BY, which make this query easy to write. (see [1]). So far we
haven't seen any buildfarm failures so far with the current ORDER BY.
That makes me think that the query output will be stable even without
COLLATE "C". So keeping it that way. But we can add COLLATE "C" if we
see buildfarm instability.

I can see the the instability locally with en_US.UTF-8 (gt before _gt) and I
can see that, for example sifaka, has en_US.UTF-8 in locales. So, modifying the
impacted query a bit to add COLLATE "C" in the attached.

I did wonder where the instability is coming from the new query - it's
because now we are traversing the entire dependency tree which also
contains array type of reltype. I think we should just eliminate it
from the result just like we are eliminating the rule. There is slight
convenience in keeping the query as is - it need not change even
though the columns returned by the function change and it's more
readable. I also think that we don't need a type defined for a
property graph - it doesn't contain any row as well as the shape of
the result of GRAPH_TABLE depends upon the COLUMNS clause - so that's
not fixed as well. I will start a separate thread for that discussion.

2. Used get_catalog_object_by_oid(), which appropriately uses the
cache or table scan in getObjectIdentityParts(). It performs an extra
lookup but I think that's ok, since the latter function is not in a
hot path. I wonder whether get_catalog_object_by_oid() is intended to
be called whenever we want to get the catalog tuple by OID instead of
directly calling sys cache lookup function or catalog scan. At least
the new code you are adding can make use of this function.

v3 was using the syscan to be consistent with what getObjectDescription() was
doing. That makes sense to me to be consistent, so in passing v5 makes use of
get_catalog_object_by_oid() in getObjectDescription() too.

I was planning to start a separate discussion to change all catalog
lookups in those functions to use get_catalog_object_by_oid(), hence
didn't include changes in getObjectDescription(). But I am fine if we
want to do it in this patch just for the property graph related nodes.

--
Best Wishes,
Ashutosh Bapat

Hi,

On Thu, Apr 30, 2026 at 06:01:22PM +0530, Ashutosh Bapat wrote:

On Thu, Apr 30, 2026 at 4:56 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

I can see the the instability locally with en_US.UTF-8 (gt before _gt) and I
can see that, for example sifaka, has en_US.UTF-8 in locales. So, modifying the
impacted query a bit to add COLLATE "C" in the attached.

I did wonder where the instability is coming from the new query - it's
because now we are traversing the entire dependency tree which also
contains array type of reltype.

Right.

I think we should just eliminate it
from the result just like we are eliminating the rule. There is slight
convenience in keeping the query as is - it need not change even
though the columns returned by the function change and it's more
readable. I also think that we don't need a type defined for a
property graph - it doesn't contain any row as well as the shape of
the result of GRAPH_TABLE depends upon the COLUMNS clause - so that's
not fixed as well. I will start a separate thread for that discussion.

Yeah, now that 891a57c7394 is in, PFA a new version of the patch. This is just
a mandatory rebase and the COLLATE "C" removals.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 5/5/26 2:00 PM, Bertrand Drouvot wrote:

Hi,

On Thu, Apr 30, 2026 at 06:01:22PM +0530, Ashutosh Bapat wrote:

On Thu, Apr 30, 2026 at 4:56 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

I can see the the instability locally with en_US.UTF-8 (gt before _gt) and I
can see that, for example sifaka, has en_US.UTF-8 in locales. So, modifying the
impacted query a bit to add COLLATE "C" in the attached.

I did wonder where the instability is coming from the new query - it's
because now we are traversing the entire dependency tree which also
contains array type of reltype.

Right.

I think we should just eliminate it
from the result just like we are eliminating the rule. There is slight
convenience in keeping the query as is - it need not change even
though the columns returned by the function change and it's more
readable. I also think that we don't need a type defined for a
property graph - it doesn't contain any row as well as the shape of
the result of GRAPH_TABLE depends upon the COLUMNS clause - so that's
not fixed as well. I will start a separate thread for that discussion.

Yeah, now that 891a57c7394 is in, PFA a new version of the patch. This is just
a mandatory rebase and the COLLATE "C" removals.

Regards,

Thanks for the patch. The patch overall LGTM. There is a typo in the commit message:

"manipulated invdividually through ALTER PROPERTY GRAPH sub-commands. Hence they”

invdividually -> individually

Regards,
Alex Guo

On Thu, Apr 30, 2026 at 06:01:22PM +0530, Ashutosh Bapat wrote:

I was planning to start a separate discussion to change all catalog
lookups in those functions to use get_catalog_object_by_oid(), hence
didn't include changes in getObjectDescription(). But I am fine if we
want to do it in this patch just for the property graph related nodes.

It does not strike me as a big deal to use get_catalog_object_by_oid
for PropgraphLabelPropertyRelationId, not does it strike me as a big
deal to keep the code as is as this is new code. Changing the other
object types to use get_catalog_object_by_oid() is going overboard as
it is not directly related to the issue at hand, so a separate
discussion looks adapted. I do agree that it would be a good move to
reuse get_catalog_object_by_oid() when we can: a syscache scan (or if
one is added in the future) will be always cheaper than a systable
scan.
--
Michael

On Wed, May 06, 2026 at 10:38:32AM +0800, Alex Guo wrote:

On 5/5/26 2:00 PM, Bertrand Drouvot wrote:

On Thu, Apr 30, 2026 at 06:01:22PM +0530, Ashutosh Bapat wrote:

I think we should just eliminate it
from the result just like we are eliminating the rule. There is slight
convenience in keeping the query as is - it need not change even
though the columns returned by the function change and it's more
readable. I also think that we don't need a type defined for a
property graph - it doesn't contain any row as well as the shape of
the result of GRAPH_TABLE depends upon the COLUMNS clause - so that's
not fixed as well. I will start a separate thread for that
discussion.

Yeah, now that 891a57c7394 is in, PFA a new version of the patch. This is just
a mandatory rebase and the COLLATE "C" removals.

In order to move the needle, I have applied the simplification of
getObjectDescription() as an independent piece.

Label properties lead to part descriptions like that:
+ property graph label property |        |      | k1 of e of e of
create_property_graph_tests.gt
+ property graph label property |        |      | k2 of e of e of
create_property_graph_tests.gt

This is confusing and hard to act on for the reader, with the same
object name defined twice (worse matter: twice in a row). For the
reader, is the first "e" something different than the second? Do both
refer to the same object? Do they refer to different sub-objects
instead but named the same because the implementation dictates so?
This could gain in clarity.

This has been mentioned upthread. But, while reviewing the rest, I am
really puzzled by your choice of "property graph element label
property" over "property graph label property", which is inconsistent
with the catalog description. We usually try to be careful about the
wordings of the descriptions with the statis data in objectaddress.c,
and the extra "element" feels out of place to me.

I'll let Peter comment about these points, but it really looks like
the intention of the catalog leads to a result closer to the attached
(leaving the label property description aside for a minute).

Attaching a rebased v7 with the remaining pieces.
--
Michael

On 07.05.26 03:47, Michael Paquier wrote:

On Wed, May 06, 2026 at 10:38:32AM +0800, Alex Guo wrote:

On 5/5/26 2:00 PM, Bertrand Drouvot wrote:

On Thu, Apr 30, 2026 at 06:01:22PM +0530, Ashutosh Bapat wrote:

I think we should just eliminate it
from the result just like we are eliminating the rule. There is slight
convenience in keeping the query as is - it need not change even
though the columns returned by the function change and it's more
readable. I also think that we don't need a type defined for a
property graph - it doesn't contain any row as well as the shape of
the result of GRAPH_TABLE depends upon the COLUMNS clause - so that's
not fixed as well. I will start a separate thread for that
discussion.

Yeah, now that 891a57c7394 is in, PFA a new version of the patch. This is just
a mandatory rebase and the COLLATE "C" removals.

In order to move the needle, I have applied the simplification of
getObjectDescription() as an independent piece.

Label properties lead to part descriptions like that:
+ property graph label property |        |      | k1 of e of e of
create_property_graph_tests.gt
+ property graph label property |        |      | k2 of e of e of
create_property_graph_tests.gt

This is confusing and hard to act on for the reader, with the same
object name defined twice (worse matter: twice in a row). For the
reader, is the first "e" something different than the second? Do both
refer to the same object? Do they refer to different sub-objects
instead but named the same because the implementation dictates so?
This could gain in clarity.

Yes, this does not seem very useful.

This has been mentioned upthread. But, while reviewing the rest, I am
really puzzled by your choice of "property graph element label
property" over "property graph label property", which is inconsistent
with the catalog description. We usually try to be careful about the
wordings of the descriptions with the statis data in objectaddress.c,
and the extra "element" feels out of place to me.

I think your assessment is correct. (But you still have the previous
name in the commit message.)

I'll let Peter comment about these points, but it really looks like
the intention of the catalog leads to a result closer to the attached
(leaving the label property description aside for a minute).

Attaching a rebased v7 with the remaining pieces.

I had left out these two catalogs from the object address handling
semi-intentionally. It's not clear to me what an event trigger would
want to do with this, or whether they should. These catalog layouts are
implementation details, and if we expose this to event triggers, are we
locked into this catalog layout? Do we need to document catalog changes
as breaking user interfaces?

Obviously, we shouldn't leave "unsupported object class" errors lying
around, but I wonder whether we could also go the other way and
intentionally skip these catalogs.

On Thu, May 07, 2026 at 11:00:48AM +0200, Peter Eisentraut wrote:

(But you still have the previous name in the commit message.)

(Yes, I did not bother edit the commit message.)

I had left out these two catalogs from the object address handling
semi-intentionally. It's not clear to me what an event trigger would want
to do with this, or whether they should. These catalog layouts are
implementation details, and if we expose this to event triggers, are we
locked into this catalog layout? Do we need to document catalog changes as
breaking user interfaces?

Obviously, we shouldn't leave "unsupported object class" errors lying
around, but I wonder whether we could also go the other way and
intentionally skip these catalogs.

Skipping them feels a bit weird to me as objectaddress.c acts as an
interface to make a bit readable catalog dependencies. It's true that
we are in a weird spot in this representation due to the way these two
properties are stored in the catalogs, but if we can make the text
presented to the reader less confusing that seems like a benefit to
me. You have much more context than myself here, of course, so
perhaps my impression is wrong.
--
Michael