Modify dimension slice catalog update API #6838
Conversation
antekresic
force-pushed
the
slice-update
branch
from
95bf761
to
0153e88
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6838 +/- ##
==========================================
+ Coverage 80.06% 81.71% +1.64%
==========================================
Files 190 199 +9
Lines 37181 36932 -249
Branches 9450 9647 +197
==========================================
+ Hits 29770 30178 +408
+ Misses 2997 2866 -131
+ Partials 4414 3888 -526 ☔ View full report in Codecov by Sentry. |
antekresic
force-pushed
the
slice-update
branch
3 times, most recently
from
3e41345
to
12e7400
Compare
| * call dimension_slice_delete_catalog_tuple to complete the delete. | ||
| * This ensures correct tuple locking and tuple deletes in the presence of | ||
| * concurrent transactions. Failure to follow this results in catalog corruption | ||
| */ |
There was a problem hiding this comment.
Should we add a check for this sequence e.g. with coccinelle?
There was a problem hiding this comment.
I'm not sure tbh. There is clearly more places in the codebase which don't update/delete catalog tuples this way so it might be premature at this point.
There was a problem hiding this comment.
I think it would be good to check this sequence via coccinelle. Not blocking this PR, but as a follow up safety check. I believe we have fixed all the known places that violate this sequence with this PR.
Use the same logic as PR 6773 while updating dimension slice catalog tuples. PR 6773 addresses chunk catalog updates. We first lock the tuple and then modify the values and update the locked tuple. Replace ts_dimension_slice_update_by_id with field specific APIs and use dimension_slice_update_catalog_tuple calls consistently.
| * call dimension_slice_delete_catalog_tuple to complete the delete. | ||
| * This ensures correct tuple locking and tuple deletes in the presence of | ||
| * concurrent transactions. Failure to follow this results in catalog corruption | ||
| */ |
There was a problem hiding this comment.
I think it would be good to check this sequence via coccinelle. Not blocking this PR, but as a follow up safety check. I believe we have fixed all the known places that violate this sequence with this PR.
| @@ -638,23 +799,14 @@ ts_dimension_slice_delete_by_dimension_id(int32 dimension_id, bool delete_constr | |||
| int | |||
| ts_dimension_slice_delete_by_id(int32 dimension_slice_id, bool delete_constraints) | |||
| { | |||
| ScanKeyData scankey[1]; | |||
| FormData_dimension_slice form; | |||
There was a problem hiding this comment.
don't we also have to modify ts_dimension_slice_delete_by_dimension_id function (line 777) to follow the same pattern?
Use the same logic as PR 6773 while updating dimension slice catalog tuples. PR 6773 addresses chunk catalog updates. We first lock the tuple and then modify the values and update the locked tuple. Replace ts_dimension_slice_update_by_id with field specific APIs and use dimension_slice_update_catalog_tuple calls consistently.
Disable-check: force-changelog-file