> 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 it as-is.

Regarding the test comment: I kept the original wording. The current comment 
already describes the case accurately (zz is 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.

--
Regards,
Man Zeng

Attachment: 0003-Properties-orphaned-by-dropping-a-label.patch
Description: Binary data

Reply via email to