> On Apr 28, 2026, at 23:02, Ashutosh Bapat <[email protected]> > wrote: > > On Tue, Apr 21, 2026 at 5:49 PM Ashutosh Bapat > <[email protected]> wrote: >> >> On Tue, Apr 21, 2026 at 1:29 PM SATYANARAYANA NARLAPURAM >> <[email protected]> wrote: >>> >>> Hi, >>> >>> On Mon, Apr 20, 2026 at 11:58 PM Ashutosh Bapat >>> <[email protected]> wrote: >>>> >>>> On Tue, Apr 21, 2026 at 11:28 AM SATYANARAYANA NARLAPURAM >>>> <[email protected]> wrote: >>>>> >>>>> HI Ashutosh, >>>>> >>>>> On Mon, Apr 20, 2026 at 10:34 PM Ashutosh Bapat >>>>> <[email protected]> wrote: >>>>>> >>>>>> On Mon, Apr 20, 2026 at 11:42 PM SATYANARAYANA NARLAPURAM >>>>>> <[email protected]> wrote: >>>>>>> >>>>>>> Hi hackers, >>>>>>> >>>>>>> ALTER PROPERTY GRAPH ... ALTER ... DROP LABEL currently allows removing >>>>>>> the last label from an element, leaving it with zero labels. >>>>>>> >>>>>>> On assert-enabled builds, pg_get_propgraphdef() hits >>>>>>> TRAP: failed Assert("count > 0"), File: "ruleutils.c", Line: 1837, PID: >>>>>>> 1821840 >>>>>>> >>>>>>> Repro: >>>>>>> >>>>>>> CREATE TABLE t (x int PRIMARY KEY, y int, z int); >>>>>>> CREATE PROPERTY GRAPH g VERTEX TABLES (t KEY (x) LABEL l1 LABEL l2); >>>>>>> ALTER PROPERTY GRAPH g ALTER VERTEX TABLE t DROP LABEL l2; >>>>>>> ALTER PROPERTY GRAPH g ALTER VERTEX TABLE t DROP LABEL l1; >>>>>>> SELECT pg_get_propgraphdef('g'::regclass); >>>>>>> >>>>>>> We can fix it two ways, (1) Prevent dropping the last label; (2) handle >>>>>>> zero labels. >>>>>>> I feel it is easier to prevent dropping the last label than handling >>>>>>> zero labels. Thoughts? >>>>>>> >>>>>> >>>>>> SQL/PGQ standard section 11.25 syntax rule 6 says >>>>>> "Element table descriptor shall include two or more labels, one of >>>>>> which has an <identifier> that is equivalent to the <label name> >>>>>> simply contained in the <drop element table label clause>." >>>>>> >>>>>> IIUC this simply means that the last label can not be dropped. That >>>>>> agrees with your chosen option. >>>>>> >>>>>> In the patch, >>>>>> + while (HeapTupleIsValid(systable_getnext(elscan))) >>>>>> + nlabels++; >>>>>> >>>>>> It's better to break from the while loop after incrementing nlabels >>>>>> and avoid scanning the entire table in the worst case. All we want to >>>>>> check is whether another label exists and not all the labels. >>>>> >>>>> >>>>> Please find the attached v2 patch. >>>>> >>>>>> >>>>>> >>>>>>> The attached patch adds a check in AlterPropGraph() before >>>>>>> performDeletion(). It scans pg_propgraph_element_label to count labels >>>>>>> for the element, and raises an error if only one remains. A regression >>>>>>> test is included >>>>>>> that drops labels down to the last one, verifies the error, then >>>>>>> re-adds them back. >>>>>> >>>>>> I would add a test to make sure ADD LABEL ... DROP LABEL .. is allowed. >>>>> >>>>> >>>>> +ALTER PROPERTY GRAPH g3 ALTER VERTEX TABLE t3 DROP LABEL t3l1; -- >>>>> error: last label >>>>> +ALTER PROPERTY GRAPH g3 ALTER VERTEX TABLE t3 ADD LABEL t3l2 PROPERTIES >>>>> ALL COLUMNS; >>>>> >>>>> Are you looking for any additional coverage? >>>> >>>> I thought specifying ADD LABEL and DROP LABEL is supported in the same >>>> DDL. But that's not the case. Sorry. >>>> >>>> Will review the patch soon. >>>> >>>> Additionally, the patch should update the DDL document to mention that >>>> the DDL does not allow dropping the last label on the given graph >>>> element table. >>> >>> >>> Addressed this in v3 patch. >> >> Thanks. >> >> I questioned whether it makes sense to move the code to check the >> existence of at least one other label into its own function so the >> code is more readable. AlterPropGraph is already about 500 lines and >> this code increases it further. There are two possibilities: >> a. The function returns true if the given element has more than two >> labels associated with it. The choice of 2 makes sense only in the >> drop label context but not otherwise. >> b. The function returns true if it finds one label other than the one >> with the given label oid. That looks a bit more elegant. But still >> seems too tied to drop label context. >> >> So I decided against it. But instead removed the extra indentation. >> The earliest variable declaration block is not farther from that code. >> >> I changed the errcode to ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE >> since we expect the element to be in a specific state to drop the >> label. ERRCODE_INVALID_OBJECT_DEFINITION would mean that the >> definition of the element or the label is itself invalid. That's not >> the case. >> >> Here's a patch with some minor edits. >> > > We are looking up element label catalogs twice in this patch - first > to find the label to be dropped and then to find the number of labels > associated with the given element. I combined these two into a single > while loop. > > This patch causes a conflict with the patch posted at [1]. Hence > posting both the patches together here, so that they can be committed > without conflict. > > [1] > https://www.postgresql.org/message-id/[email protected] > > > > -- > Best Wishes, > Ashutosh Bapat > <v20260428-0001-Prevent-dropping-the-last-label-from-a-pro.patch><v20260428-0002-Dropping-a-property-not-associated-with-th.patch>
As Alvaro commented at [1], I added 0003 to use OidIsValid macro. 0001 and 0002 are unchanged. [1] https://www.postgresql.org/message-id/[email protected] Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v20260429-0001-Prevent-dropping-the-last-label-from-a-pro.patch
Description: Binary data
v20260429-0002-Dropping-a-property-not-associated-with-th.patch
Description: Binary data
v20260429-0003-Use-OidIsValid-macro-in-propgraphcmds.c.patch
Description: Binary data
