On 02.07.26 05:40, zengman wrote:
>> The word properties in the commit message means different things in
>> different contexts leading to a possible confusion. Commit message
>> below is clearer, I think
>>
>> AlterPropGraph() cleans up pg_propgraph_property entries that are
>> orphaned by dropping an element or by dropping properties associated
>> with an element. But it doesn't clean up pg_propgraph_property entries
>> that are orphaned by dropping labels associated with an element. Fix this
>> missing case.
>>
>> Also the comment in the test may read better if rewritten like below
>>
>> -- Dropping a label should drop only orphaned properties. Dropping label t3l1
>> -- should also drop zz because it is only associated with label t3l2. But x is
>> -- not dropped, even if it is associated with t3l2, because it remains
>> -- associated with t3l1. zz will not appear in the information schema queries
>> -- outputs below, but x will.
>>
>> I did not change the comment to mention pg_propgraph_properties since
>> the comment is clear even without mentioning it.
>>
>> What do you think?
>>
>> I have verified that the patch you have attached applies cleanly on
>> master. But it may not apply cleanly in case another change to
>> create_property_graph.sql goes in first. We will provide rebased patch
>> if that happens.
>
> Hi Ashutosh,
>
> Thanks for the review. I've updated the commit message per your suggestion — your version is clearer, so I've adopted
itas-is.
>
> Regarding the test comment: I kept the original wording. The current comment already describes the case accurately
(zzis orphaned because it's only associated with the dropped
> label t3l2, while x is not orphaned because it remains associated with t3l1), so I left it unchanged.
>
> No worries — if a rebase is needed later, I'll take care of it.
>
> Attached is the updated patch, rebased on the current master.
Committed, thanks.
