Re: Fix DROP PROPERTY GRAPH "unsupported object class" error - Mailing list pgsql-hackers

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.