Re: [Bug][patch]: After dropping the last label from a property graph element, invoking pg_get_propgraphdef() triggers an assertion failure - Mailing list pgsql-hackers

> On Apr 28, 2026, at 23:02, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>
> On Tue, Apr 21, 2026 at 5:49 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
>>
>> On Tue, Apr 21, 2026 at 1:29 PM SATYANARAYANA NARLAPURAM
>> <satyanarlapuram@gmail.com> wrote:
>>>
>>> Hi,
>>>
>>> On Mon, Apr 20, 2026 at 11:58 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>>>>
>>>> On Tue, Apr 21, 2026 at 11:28 AM SATYANARAYANA NARLAPURAM
>>>> <satyanarlapuram@gmail.com> wrote:
>>>>>
>>>>> HI Ashutosh,
>>>>>
>>>>> On Mon, Apr 20, 2026 at 10:34 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>>>>>>
>>>>>> On Mon, Apr 20, 2026 at 11:42 PM SATYANARAYANA NARLAPURAM
>>>>>> <satyanarlapuram@gmail.com> 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/E3D6552E-DB26-47BC-9C7C-0F9CB936BF5B@gmail.com
>
>
>
> --
> 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/02fe13db-4fba-4e9d-9b4c-e6271a133502@app.fastmail.com

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