Thread

1. Re: Fix DROP PROPERTY GRAPH "unsupported object class" error

   Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-05-05T06:00:23Z

   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
   

2. Re: Fix DROP PROPERTY GRAPH "unsupported object class" error

   Alex Guo <guo.alex.hengchen@gmail.com> — 2026-05-06T02:38:32Z

   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
   
   
   
   
   

3. Re: Fix DROP PROPERTY GRAPH "unsupported object class" error

   Michael Paquier <michael@paquier.xyz> — 2026-05-06T23:22:49Z

   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
   

4. Re: Fix DROP PROPERTY GRAPH "unsupported object class" error

   Michael Paquier <michael@paquier.xyz> — 2026-05-07T01:47:32Z

   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
   

5. Re: Fix DROP PROPERTY GRAPH "unsupported object class" error

   Peter Eisentraut <peter@eisentraut.org> — 2026-05-07T09:00:48Z

   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.
   
   
   
   
   

6. Re: Fix DROP PROPERTY GRAPH "unsupported object class" error

   Michael Paquier <michael@paquier.xyz> — 2026-05-07T22:06:52Z

   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