Обсуждение: Fix DROP PROPERTY GRAPH "unsupported object class" error
Hi hackers, While testing the Property Graphs, I observed that DROP PROPERTY GRAPH could generate the "unsupported object class" error. 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". The bug only manifests when an event trigger is active, because that is what calls these functions. 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. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
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] 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] https://www.postgresql.org/message-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]). 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] https://www.postgresql.org/message-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