| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Property graph: fix error handling when dropping non-existent label property |
| Date: | 2026-04-26 07:21:14 |
| Message-ID: | E3D6552E-DB26-47BC-9C7C-0F9CB936BF5B@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Apr 24, 2026, at 20:38, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Fri, Apr 24, 2026 at 1:03 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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/
| Attachment | Content-Type | Size |
|---|---|---|
| v2-0001-Fix-DROP-PROPERTIES-check-for-property-graph-labe.patch | application/octet-stream | 8.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chao Li | 2026-04-26 07:35:59 | Re: repack: fix a bug to reject deferrable primary key fallback for concurrent mode |
| Previous Message | Antonin Houska | 2026-04-26 07:12:36 | Re: [PATCH] Compressed TOAST data corruption with REPACK CONCURRENTLY |