public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Ashutosh Bapat <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Property graph: fix error handling when dropping non-existent label property
Date: Sun, 26 Apr 2026 15:21:14 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAExHW5s1q7eyp4qJLPzCgnsE9tQ8uKdLyOwKt3cijdMR_0E+Cg@mail.gmail.com>
References: <[email protected]>
<CAExHW5s1q7eyp4qJLPzCgnsE9tQ8uKdLyOwKt3cijdMR_0E+Cg@mail.gmail.com>
> 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)
view thread (7+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Property graph: fix error handling when dropping non-existent label property
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox