| From: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
|---|---|
| To: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
| Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Peter Eisentraut <peter(at)eisentraut(dot)org> |
| Subject: | Re: Fix DROP PROPERTY GRAPH "unsupported object class" error |
| Date: | 2026-04-24 12:07:56 |
| Message-ID: | CAExHW5vfxVCQFADhiEmi3goyOgGb5qQjURHaM2QDtW6CksUzWA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Thu, Apr 23, 2026 at 4:31 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)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(at)paquier(dot)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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Álvaro Herrera | 2026-04-24 12:14:30 | Re: Redundant/mis-use of _(x) gettext macro? |
| Previous Message | David Rowley | 2026-04-24 12:06:57 | Selective tuple deformation |