-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Forbid Batch Sorted Merge on multikey minmax index #9955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fixes: #9955 Fix wrong results when using Batch Sorted Merge with no first-last index on a non-leading order by column |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2925,15 +2925,33 @@ match_pathkeys_to_compression_orderby(List *pathkeys, List *chunk_em_exprs, | |
| return false; | ||
| } | ||
|
|
||
| /* Bail out on BSM if orderby column is nullable, | ||
| * as at the moment the minmax metadata we have doesn't include NULLs, | ||
| * so it's difficult to use it for null-sensitive ordering. | ||
| * But this restriction can be lifted in the future on new type of chunks | ||
| * with NULL-handling metadata. | ||
| */ | ||
| if (for_bsm && !is_var_notnull(compression_info, var)) | ||
| if (for_bsm) | ||
| { | ||
| return false; | ||
| /* Bail out on Batch Sorted Merge if orderby column is nullable, | ||
| * as at the moment the minmax metadata we have doesn't include NULLs, | ||
| * so it's difficult to use it for null-sensitive ordering. | ||
| * But this restriction can be lifted in the future on new type of chunks | ||
| * with NULL-handling metadata. | ||
| */ | ||
| if (!is_var_notnull(compression_info, var)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| /* Bail out on Batch Sorted Merge with multiple order by keys | ||
| * if non-leading keys don't use firstlast index. | ||
| * Batches can be sorted incorrectly on multikey minmax index, | ||
| * for example | ||
| * [(1, 20) .. (1, 30), (2,0)...(2,30)] with min(1),(0) | ||
| * will be sorted before [(1,1) .. (1,19)] with min(1),(1) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we just continue adding the batches to the heap if there's a tie on leading columns? Disabling it altogether looks too heavy-handed.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is only disabled if there are no firstlast indexes on the non-leading columns. No firstlast on leading column is OK, BSM is not disabled in this case, there is a unit test for it.
Batch Sorted Merge depends on batches added in correct sort order of their first tuple, adding |
||
| * but it should be sorted after as (1,20) > (1,1): correct with firstlast index. | ||
| */ | ||
| if (compressed_pk_index > 1 && | ||
| orderby_sparse_kind(compression_info->settings, orderby_index) != | ||
| ORDERBY_SPARSE_FIRSTLAST) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| bool orderby_desc = | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.