Property graph: fix error handling when dropping non-existent label property

  • Jump to comment-1
    Chao Li<li.evan.chao@gmail.com>
    Apr 24, 2026, 7:33 AM UTC
    Hi,
    I am testing graph tables today and noticed an improper error message when dropping a property.
    Here is a simple repro:
    evantest=# CREATE TABLE t1 (a int PRIMARY KEY, b int);
    CREATE TABLE
    evantest=#
    evantest=# CREATE PROPERTY GRAPH g
    evantest-#     VERTEX TABLES (
    evantest(#         t1
    evantest(#             LABEL l1 PROPERTIES (a AS p1)
    evantest(#             LABEL l2 PROPERTIES (b AS p2)
    evantest(#     );
    CREATE PROPERTY GRAPH
    evantest=#
    evantest=# ALTER PROPERTY GRAPH g
    evantest-#     ALTER VERTEX TABLE t1
    evantest-#     ALTER LABEL l1 DROP PROPERTIES (p2);
    ERROR:  could not find tuple for label property 0
    This does not look like a normal user-facing SQL error message.
    Looking into the code, AlterPropGraph() checks whether the property exists in the graph, but does not check if label property exists. As a result, InvalidOid can be passed to ObjectAddressSet() and then to performDeletion().
    The fix is simple, just check the label-property OID before trying to delete it. I also added a regress test.
    With the patch, the error becomes:
    evantest=# ALTER PROPERTY GRAPH g
    evantest-#     ALTER VERTEX TABLE t1
    evantest-#     ALTER LABEL l1 DROP PROPERTIES (p2);
    ERROR:  property graph "g" element "t1" label "l1" has no property "p2"
    Best regards,
    --
    Chao Li (Evan)
    HighGo Software Co., Ltd.
    https://www.highgo.com/
    • Jump to comment-1
      Ashutosh Bapat<ashutosh.bapat.oss@gmail.com>
      Apr 24, 2026, 12:38 PM UTC
      On Fri, Apr 24, 2026 at 1:03 PM Chao Li <li.evan.chao@gmail.com> wrote:

      Hi,

      I am testing graph tables today and noticed an improper error message when dropping a property.

      Here is a simple repro:
      ```
      evantest=# CREATE TABLE t1 (a int PRIMARY KEY, b int);
      CREATE TABLE
      evantest=#
      evantest=# CREATE PROPERTY GRAPH g
      evantest-# VERTEX TABLES (
      evantest(# t1
      evantest(# LABEL l1 PROPERTIES (a AS p1)
      evantest(# LABEL l2 PROPERTIES (b AS p2)
      evantest(# );
      CREATE PROPERTY GRAPH
      evantest=#
      evantest=# ALTER PROPERTY GRAPH g
      evantest-# ALTER VERTEX TABLE t1
      evantest-# ALTER LABEL l1 DROP PROPERTIES (p2);
      ERROR: could not find tuple for label property 0
      ```

      This does not look like a normal user-facing SQL error message.

      Looking into the code, AlterPropGraph() checks whether the property exists in the graph, but does not check if label property exists. As a result, InvalidOid can be passed to ObjectAddressSet() and then to performDeletion().

      The fix is simple, just check the label-property OID before trying to delete it. I also added a regress test.

      With the patch, the error becomes:
      ```
      evantest=# ALTER PROPERTY GRAPH g
      evantest-# ALTER VERTEX TABLE t1
      evantest-# ALTER LABEL l1 DROP PROPERTIES (p2);
      ERROR: property graph "g" element "t1" label "l1" has no property "p2"
      ```
      Thanks for the report. Agree that we need to provide a correct error
      message. The code changes look good to me. However, the way you have
      written this code is different from similar code earlier in the
      function. Either your code should match that style or that code should
      be changed to your style. I like your way - reduces code a bit and
      does not repeat ereport. I also noticed that the code to fetch the
      element label oid from element_alias and label name is repeated along
      with the ereport in a few places. I think we could instead write a
      function to do that and call that function in those places. When
      writing the function, we could change that code to use your style.
      --
      Best Wishes,
      Ashutosh Bapat
      • Jump to comment-1
        Chao Li<li.evan.chao@gmail.com>
        Apr 26, 2026, 7:22 AM UTC
        On Apr 24, 2026, at 20:38, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
        On Fri, Apr 24, 2026 at 1:03 PM Chao Li <li.evan.chao@gmail.com> wrote:

        Hi,

        I am testing graph tables today and noticed an improper error message when dropping a property.

        Here is a simple repro:
        ```
        evantest=# CREATE TABLE t1 (a int PRIMARY KEY, b int);
        CREATE TABLE
        evantest=#
        evantest=# CREATE PROPERTY GRAPH g
        evantest-# VERTEX TABLES (
        evantest(# t1
        evantest(# LABEL l1 PROPERTIES (a AS p1)
        evantest(# LABEL l2 PROPERTIES (b AS p2)
        evantest(# );
        CREATE PROPERTY GRAPH
        evantest=#
        evantest=# ALTER PROPERTY GRAPH g
        evantest-# ALTER VERTEX TABLE t1
        evantest-# ALTER LABEL l1 DROP PROPERTIES (p2);
        ERROR: could not find tuple for label property 0
        ```

        This does not look like a normal user-facing SQL error message.

        Looking into the code, AlterPropGraph() checks whether the property exists in the graph, but does not check if label property exists. As a result, InvalidOid can be passed to ObjectAddressSet() and then to performDeletion().

        The fix is simple, just check the label-property OID before trying to delete it. I also added a regress test.

        With the patch, the error becomes:
        ```
        evantest=# ALTER PROPERTY GRAPH g
        evantest-# ALTER VERTEX TABLE t1
        evantest-# ALTER LABEL l1 DROP PROPERTIES (p2);
        ERROR: property graph "g" element "t1" label "l1" has no property "p2"
        ```

        Thanks for the report. Agree that we need to provide a correct error
        message. The code changes look good to me.
        Cool! Thanks for reviewing and confirming.
        However, the way you have
        written this code is different from similar code earlier in the
        function. Either your code should match that style or that code should
        be changed to your style. I like your way - reduces code a bit and
        does not repeat ereport. I also noticed that the code to fetch the
        element label oid from element_alias and label name is repeated along
        with the ereport in a few places. I think we could instead write a
        function to do that and call that function in those places. When
        writing the function, we could change that code to use your style.
        I updated the nearby code to align with my style. PFA v2.
        Best regards,
        --
        Chao Li (Evan)
        HighGo Software Co., Ltd.
        https://www.highgo.com/
        • Jump to comment-1
          Ashutosh Bapat<ashutosh.bapat.oss@gmail.com>
          Apr 28, 2026, 3:03 PM UTC
          On Sun, Apr 26, 2026 at 12:51 PM Chao Li <li.evan.chao@gmail.com> wrote:


          On Apr 24, 2026, at 20:38, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
          On Fri, Apr 24, 2026 at 1:03 PM Chao Li <li.evan.chao@gmail.com> wrote:

          Hi,

          I am testing graph tables today and noticed an improper error message when dropping a property.

          Here is a simple repro:
          ```
          evantest=# CREATE TABLE t1 (a int PRIMARY KEY, b int);
          CREATE TABLE
          evantest=#
          evantest=# CREATE PROPERTY GRAPH g
          evantest-# VERTEX TABLES (
          evantest(# t1
          evantest(# LABEL l1 PROPERTIES (a AS p1)
          evantest(# LABEL l2 PROPERTIES (b AS p2)
          evantest(# );
          CREATE PROPERTY GRAPH
          evantest=#
          evantest=# ALTER PROPERTY GRAPH g
          evantest-# ALTER VERTEX TABLE t1
          evantest-# ALTER LABEL l1 DROP PROPERTIES (p2);
          ERROR: could not find tuple for label property 0
          ```

          This does not look like a normal user-facing SQL error message.

          Looking into the code, AlterPropGraph() checks whether the property exists in the graph, but does not check if label property exists. As a result, InvalidOid can be passed to ObjectAddressSet() and then to performDeletion().

          The fix is simple, just check the label-property OID before trying to delete it. I also added a regress test.

          With the patch, the error becomes:
          ```
          evantest=# ALTER PROPERTY GRAPH g
          evantest-# ALTER VERTEX TABLE t1
          evantest-# ALTER LABEL l1 DROP PROPERTIES (p2);
          ERROR: property graph "g" element "t1" label "l1" has no property "p2"
          ```

          Thanks for the report. Agree that we need to provide a correct error
          message. The code changes look good to me.

          Cool! Thanks for reviewing and confirming.
          However, the way you have
          written this code is different from similar code earlier in the
          function. Either your code should match that style or that code should
          be changed to your style. I like your way - reduces code a bit and
          does not repeat ereport. I also noticed that the code to fetch the
          element label oid from element_alias and label name is repeated along
          with the ereport in a few places. I think we could instead write a
          function to do that and call that function in those places. When
          writing the function, we could change that code to use your style.

          I updated the nearby code to align with my style. PFA v2.
          Looks good to me. However, I did change OidIsValid() and !OidIsValid()
          back to (oid) and (!oid) conditions to be consistent with the rest of
          the code.
          I tried to deduplicate the code which finds element labelid from given
          element alias and label name. But there is one place where we also use
          the label oid and or the element oid that the code extracts. So it
          didn't end up being a lot of improvement.
          Since this patch conflicts with the patch at [1], I am posting this
          patch along with the patch there on that thread so that both can be
          committed without causing a merge conflict. If necessary we can
          continue discussion here.
          [1] https://www.postgresql.org/message-id/CAExHW5uThebnwqyWHrsF2Z+no-fT8VrM0Yj+RU7YWrYzdavUZg@mail.gmail.com
          --
          Best Wishes,
          Ashutosh Bapat
          • Jump to comment-1
            Chao Li<li.evan.chao@gmail.com>
            Apr 29, 2026, 6:02 AM UTC
            On Apr 28, 2026, at 23:01, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
            On Sun, Apr 26, 2026 at 12:51 PM Chao Li <li.evan.chao@gmail.com> wrote:


            On Apr 24, 2026, at 20:38, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
            On Fri, Apr 24, 2026 at 1:03 PM Chao Li <li.evan.chao@gmail.com> wrote:

            Hi,

            I am testing graph tables today and noticed an improper error message when dropping a property.

            Here is a simple repro:
            ```
            evantest=# CREATE TABLE t1 (a int PRIMARY KEY, b int);
            CREATE TABLE
            evantest=#
            evantest=# CREATE PROPERTY GRAPH g
            evantest-# VERTEX TABLES (
            evantest(# t1
            evantest(# LABEL l1 PROPERTIES (a AS p1)
            evantest(# LABEL l2 PROPERTIES (b AS p2)
            evantest(# );
            CREATE PROPERTY GRAPH
            evantest=#
            evantest=# ALTER PROPERTY GRAPH g
            evantest-# ALTER VERTEX TABLE t1
            evantest-# ALTER LABEL l1 DROP PROPERTIES (p2);
            ERROR: could not find tuple for label property 0
            ```

            This does not look like a normal user-facing SQL error message.

            Looking into the code, AlterPropGraph() checks whether the property exists in the graph, but does not check if label property exists. As a result, InvalidOid can be passed to ObjectAddressSet() and then to performDeletion().

            The fix is simple, just check the label-property OID before trying to delete it. I also added a regress test.

            With the patch, the error becomes:
            ```
            evantest=# ALTER PROPERTY GRAPH g
            evantest-# ALTER VERTEX TABLE t1
            evantest-# ALTER LABEL l1 DROP PROPERTIES (p2);
            ERROR: property graph "g" element "t1" label "l1" has no property "p2"
            ```

            Thanks for the report. Agree that we need to provide a correct error
            message. The code changes look good to me.

            Cool! Thanks for reviewing and confirming.
            However, the way you have
            written this code is different from similar code earlier in the
            function. Either your code should match that style or that code should
            be changed to your style. I like your way - reduces code a bit and
            does not repeat ereport. I also noticed that the code to fetch the
            element label oid from element_alias and label name is repeated along
            with the ereport in a few places. I think we could instead write a
            function to do that and call that function in those places. When
            writing the function, we could change that code to use your style.

            I updated the nearby code to align with my style. PFA v2.

            Looks good to me. However, I did change OidIsValid() and !OidIsValid()
            back to (oid) and (!oid) conditions to be consistent with the rest of
            the code.
            In the file, I also see:
                if (pgrelid == InvalidOid)
            Should we take this opportunity to change to use OidIsValid() everywhere in the file? As this feature is new to PG19, we can cleanup the inconsistency before releasing v19. Otherwise some people might also file a cleanup patch for this in the future.

            I tried to deduplicate the code which finds element labelid from given
            element alias and label name. But there is one place where we also use
            the label oid and or the element oid that the code extracts. So it
            didn't end up being a lot of improvement.

            Since this patch conflicts with the patch at [1], I am posting this
            patch along with the patch there on that thread so that both can be
            committed without causing a merge conflict. If necessary we can
            continue discussion here.
            Reasonable.

            [1] https://www.postgresql.org/message-id/CAExHW5uThebnwqyWHrsF2Z+no-fT8VrM0Yj+RU7YWrYzdavUZg@mail.gmail.com


            --
            Best Wishes,
            Ashutosh Bapat
            Best regards,
            --
            Chao Li (Evan)
            HighGo Software Co., Ltd.
            https://www.highgo.com/
            • Jump to comment-1
              Álvaro Herrera<alvherre@kurilemu.de>
              Apr 29, 2026, 9:52 AM UTC
              On 2026-04-29, Chao Li wrote:
              Looks good to me. However, I did change OidIsValid() and !OidIsValid()
              back to (oid) and (!oid) conditions to be consistent with the rest of
              the code.

              In the file, I also see:
              ```
              if (pgrelid == InvalidOid)
              ```

              Should we take this opportunity to change to use OidIsValid()
              everywhere in the file? As this feature is new to PG19, we can cleanup
              the inconsistency before releasing v19. Otherwise some people might
              also file a cleanup patch for this in the future.
              Yeah, I find "if (oid)" a rather terrible coding pattern. The negative one is perhaps not so bad, but I'd keep both cases similar by using the macro in both, for consistency.
              --
              Álvaro Herrera
              • Jump to comment-1
                Ashutosh Bapat<ashutosh.bapat.oss@gmail.com>
                Apr 29, 2026, 12:10 PM UTC
                On Wed, Apr 29, 2026 at 3:22 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

                On 2026-04-29, Chao Li wrote:
                Looks good to me. However, I did change OidIsValid() and !OidIsValid()
                back to (oid) and (!oid) conditions to be consistent with the rest of
                the code.

                In the file, I also see:
                ```
                if (pgrelid == InvalidOid)
                ```

                Should we take this opportunity to change to use OidIsValid()
                everywhere in the file? As this feature is new to PG19, we can cleanup
                the inconsistency before releasing v19. Otherwise some people might
                also file a cleanup patch for this in the future.

                Yeah, I find "if (oid)" a rather terrible coding pattern. The negative one is perhaps not so bad, but I'd keep both cases similar by using the macro in both, for consistency.
                I am in favour of doing this change, but let's do it in a separate patch.
                --
                Best Wishes,
                Ashutosh Bapat