Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Property graph: fix error handling when dropping non-existent
 label property
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAExHW5s-4MHjo0wn=bNGQQd=Q+3SW_HGxJCFnoBF+hiy5ni8wA@mail.gmail.com>
Date: Wed, 29 Apr 2026 14:01:19 +0800
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <59B04C7C-8DE7-48CE-9CD4-6E188D16A0C4@gmail.com>
References: <1DA5D52A-4AFA-426E-83F7-42ED974D682B@gmail.com>
 <CAExHW5s1q7eyp4qJLPzCgnsE9tQ8uKdLyOwKt3cijdMR_0E+Cg@mail.gmail.com>
 <E3D6552E-DB26-47BC-9C7C-0F9CB936BF5B@gmail.com>
 <CAExHW5s-4MHjo0wn=bNGQQd=Q+3SW_HGxJCFnoBF+hiy5ni8wA@mail.gmail.com>
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/59B04C7C-8DE7-48CE-9CD4-6E188D16A0C4%40gmail.com>
Precedence: bulk


> On Apr 28, 2026, at 23:01, Ashutosh Bapat =
<ashutosh.bapat.oss@gmail.com> wrote:
>=20
> On Sun, Apr 26, 2026 at 12:51=E2=80=AFPM Chao Li =
<li.evan.chao@gmail.com> wrote:
>>=20
>>=20
>>=20
>>> On Apr 24, 2026, at 20:38, Ashutosh Bapat =
<ashutosh.bapat.oss@gmail.com> wrote:
>>>=20
>>> On Fri, Apr 24, 2026 at 1:03=E2=80=AFPM Chao Li =
<li.evan.chao@gmail.com> wrote:
>>>>=20
>>>> Hi,
>>>>=20
>>>> I am testing graph tables today and noticed an improper error =
message when dropping a property.
>>>>=20
>>>> Here is a simple repro:
>>>> ```
>>>> evantest=3D# CREATE TABLE t1 (a int PRIMARY KEY, b int);
>>>> CREATE TABLE
>>>> evantest=3D#
>>>> evantest=3D# 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=3D#
>>>> evantest=3D# 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
>>>> ```
>>>>=20
>>>> This does not look like a normal user-facing SQL error message.
>>>>=20
>>>> 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().
>>>>=20
>>>> The fix is simple, just check the label-property OID before trying =
to delete it. I also added a regress test.
>>>>=20
>>>> With the patch, the error becomes:
>>>> ```
>>>> evantest=3D# 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"
>>>> ```
>>>=20
>>> Thanks for the report. Agree that we need to provide a correct error
>>> message. The code changes look good to me.
>>=20
>> Cool! Thanks for reviewing and confirming.
>>=20
>>> 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.
>>=20
>> I updated the nearby code to align with my style. PFA v2.
>=20
> 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 =3D=3D 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.


>=20
> 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.
>=20
> 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.

>=20
> [1] =
https://www.postgresql.org/message-id/CAExHW5uThebnwqyWHrsF2Z+no-fT8VrM0Yj=
+RU7YWrYzdavUZg@mail.gmail.com
>=20
>=20
> --=20
> Best Wishes,
> Ashutosh Bapat

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/