Allow non-btree operator pushdown in UPDATE/DELETE queries on compressed chunks #7649
Conversation
|
@mkindahl, @antekresic: please review this pull request.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7649 +/- ##
==========================================
+ Coverage 80.06% 81.30% +1.23%
==========================================
Files 190 241 +51
Lines 37181 44754 +7573
Branches 9450 11173 +1723
==========================================
+ Hits 29770 36385 +6615
- Misses 2997 3977 +980
+ Partials 4414 4392 -22 ☔ View full report in Codecov by Sentry. |
svenklemm
force-pushed
the
non-btree-operator
branch
2 times, most recently
from
1beea0c to
8974147
Compare
| create_segment_filter_scankey(Relation in_rel, char *segment_filter_col_name, | ||
| StrategyNumber strategy, Oid subtype, ScanKeyData *scankeys, | ||
| int *num_scankeys, Bitmapset **null_columns, Datum value, | ||
| bool is_null_check, bool is_array_op) | ||
| StrategyNumber strategy, Oid subtype, Oid opcode, | ||
| ScanKeyData *scankeys, int *num_scankeys, Bitmapset **null_columns, | ||
| Datum value, bool is_null_check, bool is_array_op) |
There was a problem hiding this comment.
This function has a little too many parameters, maybe we should make it take the BatchFilter?
There was a problem hiding this comment.
i had the same thought but not all callers have a batchfilter
| Assert(OidIsValid(opr)); | ||
| /* We should never end up here but: no opcode, no optimization */ | ||
| if (!OidIsValid(opr)) | ||
| return false; |
There was a problem hiding this comment.
Hmm... several of the callers assume that setting up the scan key succeeds, which means that in a production build, if this for any reason does not have an opcode, the scan key will not be set up, but execution will continue in the caller under the assumption that the scankey is set up.
I see that we do not increase the num_scankeys here, so this might be safe, but give that there is an assert here it seems like we should deal with this as an error.
For example, if the scankey is not added in this function and it is not added as a filter (because the assumption was that this function succeeded), then we will actually end up in a similar position as before and delete more data than what should be deleted.
One other option is to raise an error here instead, e.g., using Ensure?
There was a problem hiding this comment.
Ensure would be a breaking change and certainly not appropriate for a patch release. This function has multiple return false when certain conditions are not met and num_scankeys is only increased when we actually add a scankey. None of the callers assume this always succeeds.
There was a problem hiding this comment.
But what happens with the scan key in that case? Is it kept as a filter?
There was a problem hiding this comment.
The scankey is only allocated at the end of the function when everything succeeds. I've removed the assert since it didnt serve a purpose.
svenklemm
force-pushed
the
non-btree-operator
branch
4 times, most recently
from
2035a88 to
24c9f1b
Compare
…sed chunks When pushing down expressions into the compressed scan we assumed all valid expressions use btree operators and dropped any that weren't. This patch changes the behaviour to keep those expressions and use them as heap filter on the compressed scan for UPDATE and DELETE on compressed chunks.
svenklemm
force-pushed
the
non-btree-operator
branch
from
24c9f1b to
8f153bd
Compare
When pushing down expressions into the compressed scan we assumed
all valid expressions use btree operators and dropped any that weren't.
This patch changes the behaviour to keep those expressions and use
them as heap filter on the compressed scan for UPDATE and DELETE
on compressed chunks.