| From: | Peter Eisentraut <peter(at)eisentraut(dot)org> |
|---|---|
| To: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
| Cc: | SATYANARAYANA NARLAPURAM <satyanarlapuram(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: [Bug][patch]: After dropping the last label from a property graph element, invoking pg_get_propgraphdef() triggers an assertion failure |
| Date: | 2026-07-03 14:45:54 |
| Message-ID: | 0eff0ce4-a5fe-44ba-bf3b-19524f8db2e9@eisentraut.org |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On 19.06.26 14:05, Ashutosh Bapat wrote:
> On Tue, May 12, 2026 at 12:31 PM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>>
>> On Tue, May 12, 2026 at 7:05 AM Ashutosh Bapat
>> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>>>
>>> On Mon, May 4, 2026 at 8:16 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>>>>
>>>> On 28.04.26 17:02, Ashutosh Bapat wrote:
>>>>> 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.
>>>>
>>>> That looks okay, but I think the names of the local variables are now a
>>>> bit off. I would expect elrel and elscan to refer to
>>>> pg_propgraph_element, not pg_propgraph_element_label. Maybe use
>>>> ellabelrel etc.
>>>
>>> Done.
>>>
>>>>
>>>> Also, I think this code needs to think a bit about locking to handle the
>>>> situation where more than one DROP LABEL operation happens concurrently.
>>>>
>>>
>>> AlterPropGraph already takes ShareRowExclusiveLock at the beginning so
>>> only one label can be dropped at a time. I have added an isolation
>>> test to test the scenario. We could further add some more tests to
>>> make sure that properties can not be added to a label being dropped,
>>> adding label to an element being dropped, adding label to an element
>>> being added etc. Would that be an overkill?
>>
>> Here's the patchset without the extra tests.
>
> I decided to go ahead and added those extra tests as a separate patch.
> They don't cover all the possible concurrent modifications, but the
> ones which may render a property or a label orphan.
>
> 0001 - patch to avoid dropping last label from an element
> 0002 - patch to test concurrent ALTER PROPERTY GRAPH
> 0003 - patch from [1] which would conflict with 0001, if not included here.
>
> [1] https://www.postgresql.org/message-id/E3D6552E-DB26-47BC-9C7C-0F9CB936BF5B@gmail.com
I have committed 0001 and 0003. You are right that the lock prevents
concurrent modifications. This is pretty straightforward, so I have
omitted the patch 0002 with the new tests. I have added a brief comment
where the lock is taken.
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Eisentraut | 2026-07-03 14:47:53 | Re: Property graph: fix error handling when dropping non-existent label property |
| Previous Message | Nazir Bilal Yavuz | 2026-07-03 14:26:30 | ci: namespace ccache by PostgreSQL major version |