public inbox for [email protected]
help / color / mirror / Atom feedProperty graph: fix error handling when dropping non-existent label property
7+ messages / 3 participants
[nested] [flat]
* Property graph: fix error handling when dropping non-existent label property
@ 2026-04-24 07:32 Chao Li <[email protected]>
2026-04-24 12:38 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Chao Li @ 2026-04-24 07:32 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
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/
Attachments:
[application/octet-stream] v1-0001-Fix-DROP-PROPERTIES-check-for-property-graph-labe.patch (3.9K, 2-v1-0001-Fix-DROP-PROPERTIES-check-for-property-graph-labe.patch)
download | inline diff:
From e9b4fb61856887f8bb3b34e60315382c0282b02f Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Fri, 24 Apr 2026 15:12:17 +0800
Subject: [PATCH v1] Fix DROP PROPERTIES check for property graph labels
When ALTER PROPERTY GRAPH ... DROP PROPERTIES drops a property from a
label, the code checked only whether the property existed in the graph's
property catalog. It did not verify that the property was actually
associated with the target label.
As a result, if the property was not present on the label,
GetSysCacheOid2(PROPGRAPHLABELPROP, ...) could return InvalidOid, which
was then passed to performDeletion().
Check the label-property mapping explicitly and report an undefined
property error when it is missing. Add a regression test for dropping a
nonexistent property from a label.
Author: Chao Li <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/commands/propgraphcmds.c | 13 +++++++++----
src/test/regress/expected/create_property_graph.out | 2 ++
src/test/regress/sql/create_property_graph.sql | 1 +
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/propgraphcmds.c b/src/backend/commands/propgraphcmds.c
index 6d509fccf46..9a76332f2a8 100644
--- a/src/backend/commands/propgraphcmds.c
+++ b/src/backend/commands/propgraphcmds.c
@@ -1615,21 +1615,26 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt)
{
char *propname = strVal(lfirst(lc));
Oid propoid;
- Oid plpoid;
+ Oid plpoid = InvalidOid;
propoid = GetSysCacheOid2(PROPGRAPHPROPNAME,
Anum_pg_propgraph_property_oid,
ObjectIdGetDatum(pgrelid),
CStringGetDatum(propname));
- if (!propoid)
+
+ if (OidIsValid(propoid))
+ plpoid = GetSysCacheOid2(PROPGRAPHLABELPROP,
+ Anum_pg_propgraph_label_property_oid,
+ ObjectIdGetDatum(ellabeloid),
+ ObjectIdGetDatum(propoid));
+
+ if (!OidIsValid(plpoid))
ereport(ERROR,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("property graph \"%s\" element \"%s\" label \"%s\" has no property \"%s\"",
get_rel_name(pgrelid), stmt->element_alias, stmt->alter_label, propname),
parser_errposition(pstate, -1));
- plpoid = GetSysCacheOid2(PROPGRAPHLABELPROP, Anum_pg_propgraph_label_property_oid, ObjectIdGetDatum(ellabeloid), ObjectIdGetDatum(propoid));
-
ObjectAddressSet(obj, PropgraphLabelPropertyRelationId, plpoid);
performDeletion(&obj, stmt->drop_behavior, 0);
}
diff --git a/src/test/regress/expected/create_property_graph.out b/src/test/regress/expected/create_property_graph.out
index bc9a596ec89..d8c1c061c62 100644
--- a/src/test/regress/expected/create_property_graph.out
+++ b/src/test/regress/expected/create_property_graph.out
@@ -82,6 +82,8 @@ CREATE PROPERTY GRAPH g4
);
ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 ADD PROPERTIES (k * 2 AS kk);
ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 DROP PROPERTIES (k);
+ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 DROP PROPERTIES (yy); -- error
+ERROR: property graph "g4" element "t2" label "t2" has no property "yy"
CREATE TABLE t11 (a int PRIMARY KEY);
CREATE TABLE t12 (b int PRIMARY KEY);
CREATE TABLE t13 (
diff --git a/src/test/regress/sql/create_property_graph.sql b/src/test/regress/sql/create_property_graph.sql
index 241f93df302..8157f2b3c0a 100644
--- a/src/test/regress/sql/create_property_graph.sql
+++ b/src/test/regress/sql/create_property_graph.sql
@@ -75,6 +75,7 @@ CREATE PROPERTY GRAPH g4
ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 ADD PROPERTIES (k * 2 AS kk);
ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 DROP PROPERTIES (k);
+ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 DROP PROPERTIES (yy); -- error
CREATE TABLE t11 (a int PRIMARY KEY);
CREATE TABLE t12 (b int PRIMARY KEY);
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Property graph: fix error handling when dropping non-existent label property
2026-04-24 07:32 Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
@ 2026-04-24 12:38 ` Ashutosh Bapat <[email protected]>
2026-04-26 07:21 ` Re: Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Ashutosh Bapat @ 2026-04-24 12:38 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Fri, Apr 24, 2026 at 1:03 PM Chao Li <[email protected]> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Property graph: fix error handling when dropping non-existent label property
2026-04-24 07:32 Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-24 12:38 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
@ 2026-04-26 07:21 ` Chao Li <[email protected]>
2026-04-28 15:01 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Chao Li @ 2026-04-26 07:21 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
> On Apr 24, 2026, at 20:38, Ashutosh Bapat <[email protected]> wrote:
>
> On Fri, Apr 24, 2026 at 1:03 PM Chao Li <[email protected]> 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/
Attachments:
[application/octet-stream] v2-0001-Fix-DROP-PROPERTIES-check-for-property-graph-labe.patch (8.0K, 2-v2-0001-Fix-DROP-PROPERTIES-check-for-property-graph-labe.patch)
download | inline diff:
From 4eba672374aac4274bd355e183fb09c7a20a60dd Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Fri, 24 Apr 2026 15:12:17 +0800
Subject: [PATCH v2] Fix DROP PROPERTIES check for property graph labels
When ALTER PROPERTY GRAPH ... DROP PROPERTIES drops a property from a
label, the code checked only whether the property existed in the graph's
property catalog. It did not verify that the property was actually
associated with the target label.
As a result, if the property was not present on the label,
GetSysCacheOid2(PROPGRAPHLABELPROP, ...) could return InvalidOid, which
was then passed to performDeletion().
Check the label-property mapping explicitly and report an undefined
property error when it is missing. Add a regression test for dropping a
nonexistent property from a label.
Author: Chao Li <[email protected]>
Reviewed-by: Ashutosh Bapat <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/commands/propgraphcmds.c | 71 ++++++++-----------
.../expected/create_property_graph.out | 2 +
.../regress/sql/create_property_graph.sql | 1 +
3 files changed, 34 insertions(+), 40 deletions(-)
diff --git a/src/backend/commands/propgraphcmds.c b/src/backend/commands/propgraphcmds.c
index 6d509fccf46..bdf8dd75b0e 100644
--- a/src/backend/commands/propgraphcmds.c
+++ b/src/backend/commands/propgraphcmds.c
@@ -1491,7 +1491,7 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt)
{
Oid peoid;
Oid labeloid;
- Oid ellabeloid;
+ Oid ellabeloid = InvalidOid;
ObjectAddress obj;
if (stmt->element_kind == PROPGRAPH_ELEMENT_KIND_VERTEX)
@@ -1503,19 +1503,14 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt)
Anum_pg_propgraph_label_oid,
ObjectIdGetDatum(pgrelid),
CStringGetDatum(stmt->drop_label));
- if (!labeloid)
- ereport(ERROR,
- errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("property graph \"%s\" element \"%s\" has no label \"%s\"",
- get_rel_name(pgrelid), stmt->element_alias, stmt->drop_label),
- parser_errposition(pstate, -1));
- ellabeloid = GetSysCacheOid2(PROPGRAPHELEMENTLABELELEMENTLABEL,
- Anum_pg_propgraph_element_label_oid,
- ObjectIdGetDatum(peoid),
- ObjectIdGetDatum(labeloid));
+ if (OidIsValid(labeloid))
+ ellabeloid = GetSysCacheOid2(PROPGRAPHELEMENTLABELELEMENTLABEL,
+ Anum_pg_propgraph_element_label_oid,
+ ObjectIdGetDatum(peoid),
+ ObjectIdGetDatum(labeloid));
- if (!ellabeloid)
+ if (!OidIsValid(ellabeloid))
ereport(ERROR,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("property graph \"%s\" element \"%s\" has no label \"%s\"",
@@ -1538,7 +1533,7 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt)
Oid peoid;
Oid pgerelid;
Oid labeloid;
- Oid ellabeloid;
+ Oid ellabeloid = InvalidOid;
if (stmt->element_kind == PROPGRAPH_ELEMENT_KIND_VERTEX)
peoid = get_vertex_oid(pstate, pgrelid, stmt->element_alias, -1);
@@ -1549,18 +1544,14 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt)
Anum_pg_propgraph_label_oid,
ObjectIdGetDatum(pgrelid),
CStringGetDatum(stmt->alter_label));
- if (!labeloid)
- ereport(ERROR,
- errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("property graph \"%s\" element \"%s\" has no label \"%s\"",
- get_rel_name(pgrelid), stmt->element_alias, stmt->alter_label),
- parser_errposition(pstate, -1));
- ellabeloid = GetSysCacheOid2(PROPGRAPHELEMENTLABELELEMENTLABEL,
- Anum_pg_propgraph_element_label_oid,
- ObjectIdGetDatum(peoid),
- ObjectIdGetDatum(labeloid));
- if (!ellabeloid)
+ if (OidIsValid(labeloid))
+ ellabeloid = GetSysCacheOid2(PROPGRAPHELEMENTLABELELEMENTLABEL,
+ Anum_pg_propgraph_element_label_oid,
+ ObjectIdGetDatum(peoid),
+ ObjectIdGetDatum(labeloid));
+
+ if (!OidIsValid(ellabeloid))
ereport(ERROR,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("property graph \"%s\" element \"%s\" has no label \"%s\"",
@@ -1580,7 +1571,7 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt)
{
Oid peoid;
Oid labeloid;
- Oid ellabeloid;
+ Oid ellabeloid = InvalidOid;
ObjectAddress obj;
if (stmt->element_kind == PROPGRAPH_ELEMENT_KIND_VERTEX)
@@ -1592,19 +1583,14 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt)
Anum_pg_propgraph_label_oid,
ObjectIdGetDatum(pgrelid),
CStringGetDatum(stmt->alter_label));
- if (!labeloid)
- ereport(ERROR,
- errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("property graph \"%s\" element \"%s\" has no label \"%s\"",
- get_rel_name(pgrelid), stmt->element_alias, stmt->alter_label),
- parser_errposition(pstate, -1));
- ellabeloid = GetSysCacheOid2(PROPGRAPHELEMENTLABELELEMENTLABEL,
- Anum_pg_propgraph_element_label_oid,
- ObjectIdGetDatum(peoid),
- ObjectIdGetDatum(labeloid));
+ if (OidIsValid(labeloid))
+ ellabeloid = GetSysCacheOid2(PROPGRAPHELEMENTLABELELEMENTLABEL,
+ Anum_pg_propgraph_element_label_oid,
+ ObjectIdGetDatum(peoid),
+ ObjectIdGetDatum(labeloid));
- if (!ellabeloid)
+ if (!OidIsValid(ellabeloid))
ereport(ERROR,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("property graph \"%s\" element \"%s\" has no label \"%s\"",
@@ -1615,21 +1601,26 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt)
{
char *propname = strVal(lfirst(lc));
Oid propoid;
- Oid plpoid;
+ Oid plpoid = InvalidOid;
propoid = GetSysCacheOid2(PROPGRAPHPROPNAME,
Anum_pg_propgraph_property_oid,
ObjectIdGetDatum(pgrelid),
CStringGetDatum(propname));
- if (!propoid)
+
+ if (OidIsValid(propoid))
+ plpoid = GetSysCacheOid2(PROPGRAPHLABELPROP,
+ Anum_pg_propgraph_label_property_oid,
+ ObjectIdGetDatum(ellabeloid),
+ ObjectIdGetDatum(propoid));
+
+ if (!OidIsValid(plpoid))
ereport(ERROR,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("property graph \"%s\" element \"%s\" label \"%s\" has no property \"%s\"",
get_rel_name(pgrelid), stmt->element_alias, stmt->alter_label, propname),
parser_errposition(pstate, -1));
- plpoid = GetSysCacheOid2(PROPGRAPHLABELPROP, Anum_pg_propgraph_label_property_oid, ObjectIdGetDatum(ellabeloid), ObjectIdGetDatum(propoid));
-
ObjectAddressSet(obj, PropgraphLabelPropertyRelationId, plpoid);
performDeletion(&obj, stmt->drop_behavior, 0);
}
diff --git a/src/test/regress/expected/create_property_graph.out b/src/test/regress/expected/create_property_graph.out
index bc9a596ec89..d8c1c061c62 100644
--- a/src/test/regress/expected/create_property_graph.out
+++ b/src/test/regress/expected/create_property_graph.out
@@ -82,6 +82,8 @@ CREATE PROPERTY GRAPH g4
);
ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 ADD PROPERTIES (k * 2 AS kk);
ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 DROP PROPERTIES (k);
+ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 DROP PROPERTIES (yy); -- error
+ERROR: property graph "g4" element "t2" label "t2" has no property "yy"
CREATE TABLE t11 (a int PRIMARY KEY);
CREATE TABLE t12 (b int PRIMARY KEY);
CREATE TABLE t13 (
diff --git a/src/test/regress/sql/create_property_graph.sql b/src/test/regress/sql/create_property_graph.sql
index 241f93df302..8157f2b3c0a 100644
--- a/src/test/regress/sql/create_property_graph.sql
+++ b/src/test/regress/sql/create_property_graph.sql
@@ -75,6 +75,7 @@ CREATE PROPERTY GRAPH g4
ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 ADD PROPERTIES (k * 2 AS kk);
ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 DROP PROPERTIES (k);
+ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 DROP PROPERTIES (yy); -- error
CREATE TABLE t11 (a int PRIMARY KEY);
CREATE TABLE t12 (b int PRIMARY KEY);
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Property graph: fix error handling when dropping non-existent label property
2026-04-24 07:32 Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-24 12:38 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
2026-04-26 07:21 ` Re: Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
@ 2026-04-28 15:01 ` Ashutosh Bapat <[email protected]>
2026-04-29 06:01 ` Re: Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Ashutosh Bapat @ 2026-04-28 15:01 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Sun, Apr 26, 2026 at 12:51 PM Chao Li <[email protected]> wrote:
>
>
>
> > On Apr 24, 2026, at 20:38, Ashutosh Bapat <[email protected]> wrote:
> >
> > On Fri, Apr 24, 2026 at 1:03 PM Chao Li <[email protected]> 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/[email protected]...
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Property graph: fix error handling when dropping non-existent label property
2026-04-24 07:32 Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-24 12:38 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
2026-04-26 07:21 ` Re: Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-28 15:01 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
@ 2026-04-29 06:01 ` Chao Li <[email protected]>
2026-04-29 09:51 ` Re: Property graph: fix error handling when dropping non-existent label property Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Chao Li @ 2026-04-29 06:01 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
> On Apr 28, 2026, at 23:01, Ashutosh Bapat <[email protected]> wrote:
>
> On Sun, Apr 26, 2026 at 12:51 PM Chao Li <[email protected]> wrote:
>>
>>
>>
>>> On Apr 24, 2026, at 20:38, Ashutosh Bapat <[email protected]> wrote:
>>>
>>> On Fri, Apr 24, 2026 at 1:03 PM Chao Li <[email protected]> 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/[email protected]...
>
>
> --
> Best Wishes,
> Ashutosh Bapat
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Property graph: fix error handling when dropping non-existent label property
2026-04-24 07:32 Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-24 12:38 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
2026-04-26 07:21 ` Re: Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-28 15:01 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
2026-04-29 06:01 ` Re: Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
@ 2026-04-29 09:51 ` Álvaro Herrera <[email protected]>
2026-04-29 12:10 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Álvaro Herrera @ 2026-04-29 09:51 UTC (permalink / raw)
To: Chao Li <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: L. pgsql-hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Property graph: fix error handling when dropping non-existent label property
2026-04-24 07:32 Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-24 12:38 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
2026-04-26 07:21 ` Re: Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-28 15:01 ` Re: Property graph: fix error handling when dropping non-existent label property Ashutosh Bapat <[email protected]>
2026-04-29 06:01 ` Re: Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-29 09:51 ` Re: Property graph: fix error handling when dropping non-existent label property Álvaro Herrera <[email protected]>
@ 2026-04-29 12:10 ` Ashutosh Bapat <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Ashutosh Bapat @ 2026-04-29 12:10 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Chao Li <[email protected]>; L. pgsql-hackers <[email protected]>
On Wed, Apr 29, 2026 at 3:22 PM Álvaro Herrera <[email protected]> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-04-29 12:10 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-24 07:32 Property graph: fix error handling when dropping non-existent label property Chao Li <[email protected]>
2026-04-24 12:38 ` Ashutosh Bapat <[email protected]>
2026-04-26 07:21 ` Chao Li <[email protected]>
2026-04-28 15:01 ` Ashutosh Bapat <[email protected]>
2026-04-29 06:01 ` Chao Li <[email protected]>
2026-04-29 09:51 ` Álvaro Herrera <[email protected]>
2026-04-29 12:10 ` Ashutosh Bapat <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox