-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
MDEV-40383:innodb_gis.point_basic fails on replay #5446
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
base: bb-12.3-MDEV-39368-test-replay-preview-tree
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -3024,6 +3024,23 @@ static uint make_packed_sortkey(Sort_param *param, uchar *to) | |
| return length; | ||
| } | ||
|
|
||
| static bool is_target_cs_superset(CHARSET_INFO *from_cs, CHARSET_INFO *to_cs) | ||
| { | ||
| if (to_cs == &my_charset_bin) | ||
| return true; // binary swallows any bytes | ||
| if (from_cs == &my_charset_bin) | ||
| return false; // arbitrary bytes ⊄ text charset | ||
| if (my_charset_same(from_cs, to_cs)) | ||
| return true; // same repertoire family | ||
| if (from_cs->state & MY_CS_PUREASCII) // ASCII-only source... | ||
| return my_charset_is_ascii_based(to_cs); // ...into any ASCII-based target | ||
| if ((to_cs->state & MY_CS_UNICODE) && // Unicode target covers everything, | ||
| (to_cs->state & | ||
| MY_CS_UNICODE_SUPPLEMENT)) // incl. non-BMP (so utf8mb4/utf16/utf32, | ||
| return true; // but NOT plain utf8mb3/ucs2) | ||
| return false; // unknown → treat as possibly lossy | ||
| } | ||
|
|
||
| /* | ||
| @brief | ||
| Format the row record and store it in the output | ||
|
|
@@ -3148,12 +3165,26 @@ void format_and_store_row(TABLE *table, const uchar *rec, bool print_names, | |
| } | ||
| field->val_str(&tmp); | ||
| } | ||
| if (require_quote) | ||
| output.append('\''); | ||
| output.append_for_single_quote_opt_convert(tmp.ptr(), tmp.length(), | ||
| field->charset()); | ||
| if (require_quote) | ||
| output.append('\''); | ||
| /* | ||
| Emit non-empty values as a hex literal whenever converting field's | ||
| charset to the output charset conversion is lossy; otherwise emit the | ||
| charset-converted value, quoted only when the type requires it. | ||
| */ | ||
| if (require_quote && tmp.length() && | ||
| !is_target_cs_superset(field->charset(), output.charset())) | ||
| { | ||
| output.append(STRING_WITH_LEN("0x")); | ||
| output.append_hex(tmp.ptr(), tmp.length()); | ||
|
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. you know the charset of tmp and charset of output. You must always use hex if tmp cannot be converted to output charset without losses. Otherwise you don't need hex. It's not an external property that you need to pass as an argument.
Contributor
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. May be I should narrow down the scope of change. For Geometry types I can use HEX, and for others, existing string representation should work, as it was already taken care to use charsets.
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. No! This is the whole point. Geometry is just a special case of a binary string that cannot necessarily be converted to the target charset. It is not a property of a type, it's a property of the charset. You must use hex in all cases where you cannot perform a lossless conversion from the source to the target charset.
Contributor
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. updated it now. |
||
| } | ||
| else | ||
| { | ||
| if (require_quote) | ||
| output.append('\''); | ||
| output.append_for_single_quote_opt_convert(tmp.ptr(), tmp.length(), | ||
| field->charset()); | ||
| if (require_quote) | ||
| output.append('\''); | ||
| } | ||
| } | ||
| } | ||
| output.append(')'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25066,6 +25066,63 @@ join_read_const_table(THD *thd, JOIN_TAB *tab, POSITION *pos) | |
| DBUG_RETURN(0); | ||
| } | ||
|
|
||
| /* | ||
| Re-read a const/system table row with ALL columns and record it for the | ||
|
Contributor
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. Worth amending "ALL" because now it's all except virtual |
||
| optimizer context. | ||
|
|
||
| join_read_system()/join_read_const() only fetch the columns present in | ||
| table->read_set. That's sufficient for execution, but when we record the | ||
| const row for replay it yields an incomplete REPLACE INTO -- columns that | ||
| are NOT NULL and have no default then depend on a relaxed sql_mode (see | ||
|
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. just make sure all columns always have defaults and you won't need any of the below!
Contributor
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. @vuvova , do you suggest generating default values for any possible column type? How feasible is this given that there may be something fancy like GEOMETRY? |
||
| Optimizer_context_recorder::record_table_row()). Widen read_set to all | ||
| columns, re-read the single row, record it, then restore read_set so the | ||
| chosen plan is not disturbed. | ||
|
|
||
| The caller must have cached the row the optimizer actually used in | ||
| record[1] before calling this. We re-read the full row into record[0] only | ||
| to record it, then unconditionally restore record[0] from record[1] so that | ||
| recording leaves execution's record[0] byte-for-byte identical to the | ||
| non-recording case -- regardless of whether the re-read found a row. | ||
| */ | ||
| static void record_const_row_full(JOIN_TAB *tab, bool is_system) | ||
| { | ||
| TABLE *table= tab->table; | ||
| Optimizer_context_recorder *rec= tab->join->thd->opt_ctx_recorder; | ||
|
|
||
| MY_BITMAP *saved_read_set= table->read_set; | ||
| uint saved_status= table->status; | ||
| table->column_bitmaps_set(&table->s->all_set, table->write_set); | ||
|
Olernov marked this conversation as resolved.
|
||
| int error; | ||
| for (Field **pfield= table->field; *pfield; pfield++) | ||
| { | ||
| Field *field= *pfield; | ||
| /* virtual columns need not be stored. */ | ||
| if (field->vcol_info) | ||
| bitmap_fast_test_and_clear(table->read_set, field->field_index); | ||
|
Contributor
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. There is a subtle bug here - this in-place bitmap mutation corrupts the shared
|
||
| } | ||
| /* | ||
| Re-read the single const row with the widened read_set, mirroring how the | ||
| optimizer originally fetched it: a system table (join_read_system) has at | ||
| most one row, read via the primary key; a const eq_ref table | ||
| (join_read_const) is fetched by an exact lookup on tab->ref. | ||
| */ | ||
| if (is_system) | ||
|
Olernov marked this conversation as resolved.
|
||
| error= table->file->ha_read_first_row(table->record[0], | ||
| table->s->primary_key); | ||
| else | ||
| error= table->file->ha_index_read_idx_map( | ||
| table->record[0], tab->ref.key, (uchar *) tab->ref.key_buff, | ||
| make_prev_keypart_map(tab->ref.key_parts), HA_READ_KEY_EXACT); | ||
|
|
||
| if (likely(!error)) | ||
| rec->record_current_table_row(table); // records the full record[0] | ||
|
|
||
| /* Recording must not perturb the row execution uses: restore it. */ | ||
| restore_record(table, record[1]); | ||
|
|
||
| table->column_bitmaps_set(saved_read_set, table->write_set); | ||
| table->status= saved_status; | ||
| } | ||
|
|
||
| /** | ||
| Read a constant table when there is at most one matching row, using a table | ||
|
|
@@ -25095,9 +25152,9 @@ join_read_system(JOIN_TAB *tab) | |
| empty_record(table); // Make empty record | ||
| return -1; | ||
| } | ||
| store_record(table,record[1]); | ||
| if (Optimizer_context_recorder *rec= tab->join->thd->opt_ctx_recorder) | ||
| rec->record_const_table_row(table); | ||
| store_record(table, record[1]); // cache the row the optimizer used | ||
| if (tab->join->thd->opt_ctx_recorder) | ||
| record_const_row_full(tab, true); // re-read full row, record, restore | ||
| } | ||
| else if (!table->status) // Only happens with left join | ||
| restore_record(table,record[1]); // restore old record | ||
|
|
@@ -25152,10 +25209,9 @@ join_read_const(JOIN_TAB *tab) | |
| return report_error(table, error); | ||
| return -1; | ||
| } | ||
| store_record(table,record[1]); | ||
|
|
||
| if (Optimizer_context_recorder *rec= tab->join->thd->opt_ctx_recorder) | ||
| rec->record_const_table_row(table); | ||
| store_record(table, record[1]); // cache the row the optimizer used | ||
| if (tab->join->thd->opt_ctx_recorder) | ||
| record_const_row_full(tab, false); // re-read full row, record, restore | ||
| } | ||
| else if (!(table->status & ~STATUS_NULL_ROW)) // Only happens with left join | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is not descriptive, consider renaming to something like:
Or even invert it and rename to
charset_conversion_may_lose_data, so that the code on the call site becomes more readable: