Skip to content

MDEV-38315 Optimize NULL only reads for wide tables in core engines#4627

Open
alexaustin007 wants to merge 7 commits intoMariaDB:mainfrom
alexaustin007:MDEV-38315
Open

MDEV-38315 Optimize NULL only reads for wide tables in core engines#4627
alexaustin007 wants to merge 7 commits intoMariaDB:mainfrom
alexaustin007:MDEV-38315

Conversation

@alexaustin007
Copy link
Copy Markdown

@alexaustin007 alexaustin007 commented Feb 6, 2026

MDEV-38315 Implement NULL-only read optimization for InnoDB, Aria, and MyISAM.

SQL layer:

  • Handle WHERE, HAVING, JOIN ON, and subqueries.

  • Clear null-only marks when column value is needed elsewhere.

  • Optimization is enabled only for handlers with HA_CAN_NULL_ONLY.
    Observability:

  • Add session status metric Handler_null_only_columns.

  • InnoDB/Aria/MyISAM consume null_set and skip value materialization
    for null-only columns.

  • Federated is not covered in this patch.
    Tests:

  • Extend targeted tests:

    • maria.maria_null_only
  • Cover WHERE, JOIN ON, EXISTS-subquery, and negative "value needed"
    cases.

  • InnoDB test also validates physical impact via blob-page read metrics.

This PR adds NULL-only read optimization for wide rows where query logic
needs only NULL-ness, not the column value.

  • Aria

Not covered:

  • Federated

  • Scenarios covered

  • JOIN ... ON ... AND col IS NULL

  • EXISTS (subquery ... col IS NULL)

  • Negative cases where the value is needed (for example LENGTH(col),
    CRC32(col)), where optimization must not apply.

Conditions / gating

  • SQL computes a per-table NULL-only bitmap (null_set).
  • Columns are marked only for direct-field NULL predicates.
  • Optimization is used only if handler advertises HA_CAN_NULL_ONLY.

Observability

  • Added session status variable: Handler_null_only_columns.
  • Meaning: number of columns that remain in NULL-only mode for a
    statement after all SQL-layer filtering.
  • Tests assert:
    • > 0 for positive NULL-only scenarios
    • = 0 for value-needed scenarios.

Engine-level validation

  • InnoDB:
    • correctness + Handler_null_only_columns
    • physical effect validation via innodb_metrics blob-page counters.
  • Aria/MyISAM:
    • correctness + Handler_null_only_columns assertions.

Tests updated

  • innodb.innodb_null_only
  • maria.maria_null_only
  • myisam.myisam_null_only

Copy link
Copy Markdown
Contributor

@dr-m dr-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an initial review, focusing on the InnoDB part, and not reviewing the changes in row0sel.cc in detail.

Comment on lines +3 to +26
--disable_query_log
--disable_warnings
SET GLOBAL innodb_monitor_disable='module_buffer_page';
SET GLOBAL innodb_monitor_reset_all='module_buffer_page';
SET GLOBAL innodb_monitor_enable='module_buffer_page';
--enable_warnings

CREATE TABLE t (
id INT PRIMARY KEY,
b MEDIUMBLOB NULL,
c INT
) ENGINE=InnoDB ROW_FORMAT=DYNAMIC;

INSERT INTO t VALUES
(1, REPEAT('a', 100000), 1),
(2, NULL, 2),
(3, REPEAT('b', 100000), 3),
(4, NULL, 4);

--let $restart_noprint=2
--let $restart_parameters=--innodb-buffer-pool-load-at-startup=0 --innodb-buffer-pool-dump-at-shutdown=0
--source include/restart_mysqld.inc
--let $restart_parameters=
--disable_query_log
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the server being restarted after the data load? Restarting the server can significantly increase the execution time.

The ROW_FORMAT=DYNAMIC is redundant; this test should work with any value of innodb_default_row_format.

Hiding the queries from the output will make the .result file harder to read.

It is worth noting that in InnoDB, all of VARCHAR, TEXT, BLOB, MEDIUMBLOB and so on are being treated in the same way. I see that we are writing to the column b values that will never fit in a single InnoDB page. Because the maximum length of an InnoDB index record is 16383 bytes, shorter BLOB columns would work as well. Why does the column c exist and why does it allow NULL even if we are only inserting NOT NULL values?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server restart is intentional for test correctness so physical BLOB page reads from information_schema.innodb_metrics. Without a cold start after every data load, BLOB pages may still be in buffer pool and the assertion can pass falsely. I already removed unnecessary restarts and kept only those needed for I/O validation.

Why does the column c exist and why does it allow NULL even if we are only inserting NOT NULL values?
Good point, fixed in the latest update

Comment on lines +97 to +101
CREATE TABLE t_red (
id INT PRIMARY KEY,
b MEDIUMBLOB NULL,
c INT
) ENGINE=InnoDB ROW_FORMAT=REDUNDANT;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of repeating the test for each ROW_FORMAT, you should make use of the following:

--source include/innodb_default_row_format.inc

Comment on lines +7091 to +7102

templ->null_only = false;
if (!templ->is_virtual
&& bitmap_is_set(&table->null_set, field->field_index)) {
templ->null_only = true;
if (prebuilt->template_type == ROW_MYSQL_WHOLE_ROW
|| prebuilt->select_lock_type == LOCK_X
|| prebuilt->pk_filter
|| prebuilt->in_fts_query) {
templ->null_only = false;
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is introducing a large number of frequently executed conditional branches. The conditions on prebuilt could be initialized to a new bool parameter of the function, which the caller would pass as a local variable that is initialized outside the loop, like this:

const bool maybe_null_only= whole_row
		    || prebuilt->select_lock_type == LOCK_X
		    || prebuilt->pk_filter
		    || prebuilt->in_fts_query;

Side note: It looks like the prebuilt->template_type could have been shrunk to a single bit in ab01901.

Then, in this function, inside the pre-existing if (!templ->is_virtual) block we can use the following simple assignment:

templ->null_only = maybe_null_only && bitmap_is_set(&table->null_set, field->field_index);

Comment on lines +4141 to +4145
if (templ->mysql_null_bit_mask) {
mysql_rec[templ->mysql_null_byte_offset]
&= static_cast<byte>
(~templ->mysql_null_bit_mask);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the condition? We could just unconditionally perform the assignment; it should be smaller and faster.

Which test case in the regression test suite is exercising this code path (index condition pushdown)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines +446 to +450
@@ -447,6 +447,7 @@ struct mysql_row_templ_t {
type and this field is != 0, then
it is an unsigned integer type */
ulint is_virtual; /*!< if a column is a virtual column */
bool null_only; /*!< only NULL status is required */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the data structure with GDB ptype/o and try to rearrange the fields and change some data types so that we get better locality of reference and a smaller footprint. I would assume that null_only and mysql_null_bit_mask will be accessed together. The ulint (an alias of size_t) is an overkill and some old legacy that we could fix. For example, would byte be a more suitable data type of mysql_null_bit_mask?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, Changed mysql_null_bit_mask from ulint to byte

@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Feb 9, 2026
@gkodinov gkodinov self-assigned this Feb 13, 2026
Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a preliminary review.
Marko has already provided some feedback on the innodb part. When done with the preliminary review I'll ask for a reviewer from optimizer too.

Please fill in the commit message according to the coding style.

Also, please add details of your implementation: what SEs are covered, what scenarios, what conditions etc.

sql/handler.h Outdated
@@ -199,6 +192,7 @@ enum chf_create_flags {
#define HA_HAS_OLD_CHECKSUM (1ULL << 24)
/* Table data are stored in separate files (for lower_case_table_names) */
#define HA_FILE_BASED (1ULL << 26)
#define HA_CAN_NULL_ONLY (1ULL << 27)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably left empty for some historical reason. I'd avoid refilling it, unless I can prove that the historical thing doesn't apply anymore.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit 27 was old HA_NO_VARCHAR (unused) and removed in commit 6a6cc8a653a
Let me know if my assumption is wrong.

Copy link
Copy Markdown
Member

@gkodinov gkodinov Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some ABI considerations at play here: old SE binary running in a new server. Let's not reuse to be safe.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed HA_CAN_NULL_ONLY entirely. The gating is now done via db_type check allowing only InnoDB, Aria, and MyISAM. No handler flag bits are used. Let me know if this approach works.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about that. Leave it be for now. We'll expect that the final reviewer will make up his own mind about this.

@alexaustin007
Copy link
Copy Markdown
Author

This is a preliminary review. Marko has already provided some feedback on the innodb part. When done with the preliminary review I'll ask for a reviewer from optimizer too.

Please fill in the commit message according to the coding style.

Also, please add details of your implementation: what SEs are covered, what scenarios, what conditions etc.

Added in the description above

Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for working on this. I'm ok with the change, given that the while-space only changes are addressed and the tests that fail due to the new status var are re-recorded.

@alexaustin007
Copy link
Copy Markdown
Author

given that the while-space only changes are addressed and the tests that fail due to the new status var are re-recorded.

Whitespace-only changes are cleaned up. Some tests are failing again I'll re-record it. Requesting to review other changes. Thank you

Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no further remarks. Thanks again for working on this. Please wait for the final review.

@gkodinov gkodinov requested a review from vuvova February 20, 2026 11:20
Copy link
Copy Markdown
Member

@vuvova vuvova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! See comments inline.

Why did you decide to do a bitmap and not just one boolean property? I'm not sure, but kind of have a feeling that this is too much memory and CPU time spent on rather corner case optimization.

sql/item.cc Outdated

bool Item_field::clear_null_only_fields_processor(void *arg)
{
(void) arg;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is intentionally unused here because this clear pass is unconditional per visited field; (void) arg only suppresses unused parameter warnings

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's C++, you can use

bool Item_field::clear_null_only_fields_processor(void *)
{


static inline Field *null_predicate_field(Item *item);
static inline bool mark_null_only_field(Field *field, void *arg);
static inline Field *null_safe_equal_field(Item *left, Item *right);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not to define functions here at once, why these forward declarations?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the forward declarations i think it is now more easier to read now

{
if ((flags & WALK_SKIP_NULL_PREDICATE_ARGS)
&& null_safe_equal_field(args[0], args[1]))
return (this->*processor)(arg);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? what was wrong with parent's walk() implementation?
same question below, for other walk() overrides

Copy link
Copy Markdown
Author

@alexaustin007 alexaustin007 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parent walk() is too generic for the clear pass (this is just my assumption, i could be wrong here). It would descend into the field inside IS NULL / IS NOT NULL / <=> NULL and clear_null_only_fields_processor() would clear that bit immediately, which defeats the earlier mark. The override is only there for WALK_SKIP_NULL_PREDICATE_ARGS: it skips those predicate arguments during the clear pass, and otherwise falls back to the parent walk() unchanged. Again do let me know if I my approach/assumption is incorrect

static inline bool mark_null_only_field(Field *field, void *arg)
{
if (!field || !field->table || !field->table->file
|| !field->real_maybe_null())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you check for field->table->file and field->real_maybe_null() ?

Copy link
Copy Markdown
Author

@alexaustin007 alexaustin007 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was overly defense on my end so I removed the field->table->file check here. real_maybe_null() is intentional: this optimization only makes sense for actually nullable stored columns, and marking a NOT NULL column would be meaningless
So
!field->table->file removed
!field->real_maybe_null() kept

*static_cast<bool *>(arg)= true;
return true;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to have a comment here, that it's handling conditions like field <=> NULL

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

sql/sql_lex.cc Outdated
while ((tl= ti++))
{
if (tl->table)
bitmap_clear_all(&tl->table->null_set);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that's needed. read_set/write_set etc are all initialized in one place, when a table is being opened (or taken from the cache) for the query. Why null_set needs to be cleaned up separately?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This separate clear is only there because isnull_only_set is recomputed when update_used_tables() runs, and that happens multiple times during optimization. So unlike read_set/write_set, this bitmap is not just initialized once per table open

}

clear_null_only_in_expr(select->join->conds);
clear_null_only_in_expr(select->join->having);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very confused. You set bits in null_set, then you cleared them
all again, what's the point?

Copy link
Copy Markdown
Author

@alexaustin007 alexaustin007 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the code as it is now, the separate clear is needed. isnull_only_set (renamed) is recomputed several times.
Because of that, the extra clear is necessary maybe a better design would compute isnull_only_set once at a later stable point, which would likely make the repeated clear unnecessary.
That might be more efficient, but it seems like a big redesign

for (ORDER *order= select->order_list.first; order; order= order->next)
(*order->item)->walk(&Item::clear_null_only_fields_processor, 0,
WALK_SUBQUERY | WALK_SKIP_NULL_PREDICATE_ARGS);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is all that?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment for it

  Mark fields that appear in NULL predicates, then clear fields that are also
  used in other contexts. During the clear pass,
  WALK_SKIP_NULL_PREDICATE_ARGS keeps the field inside IS NULL / IS NOT NULL /
  <=> NULL from being cleared by that same predicate, so only fields used
  exclusively for NULL checks remain set.

sql/sql_lex.cc Outdated

if (null_only_columns)
status_var_add(current_thd->status_var.ha_null_only_columns,
null_only_columns);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not exactly right, ha_* counters count handler::ha_* calls.
there is no handler call here, it cannot use ha_* naming.

also I'm not quite sure that a sum of null_only_columns is the most
useful data to show, but I don't have a replacement at the moment,
so let's keep it your way. May be I'll be able to suggest something later.

Copy link
Copy Markdown
Author

@alexaustin007 alexaustin007 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it cannot use ha_* naming.

Addressed

include/myisam.h Outdated
*/

#define MI_NO_FIELD_NR 0xFFFFU

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't bother fixing MyISAM and Aria.
MyISAM doesn't even have HA_PARTIAL_COLUMN_READ in table_flags,
meaning it always ignores read_set and returns the complete row.
null_set would be a rather minimal improvement here.

What's important is to fix InnoDB.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it, kept patch focused on InnoDB

@alexaustin007
Copy link
Copy Markdown
Author

alexaustin007 commented Mar 1, 2026

Why did you decide to do a bitmap and not just one boolean property? I'm not sure, but kind of have a feeling that this is too much memory and CPU time spent on rather corner case optimization.

I used it because this decision is per column, not per table. In the same query one column can be used only in IS NULL / <=> NULL, while another column from the same table is still needed normally for select/join/filtering. A single boolean would be too coarse and would force an all or nothing choice, while the bitmap lets InnoDB skip value materialization only for the specific columns that are actually NULL only. Please do let me know if there is a better approach where it's less expensive and my inline comments if any assumption/approach seems incorrect. Thank You so much

@alexaustin007 alexaustin007 requested a review from vuvova March 5, 2026 19:24
@vuvova
Copy link
Copy Markdown
Member

vuvova commented Apr 8, 2026

Why did you decide to do a bitmap and not just one boolean property? I'm not sure, but kind of have a feeling that this is too much memory and CPU time spent on rather corner case optimization.

I used it because this decision is per column, not per table. In the same query one column can be used only in IS NULL / <=> NULL, while another column from the same table is still needed normally for select/join/filtering. A single boolean would be too coarse and would force an all or nothing choice, while the bitmap lets InnoDB skip value materialization only for the specific columns that are actually NULL only. Please do let me know if there is a better approach where it's less expensive and my inline comments if any assumption/approach seems incorrect. Thank You so much

I wouldn't call it incorrect. It's a tradeoff. We've discussed it on Zulip. A single boolean would mean "return NULL bitmap for all columns, in addition to column value from read_set". Most engines cannot return NULL bits partially, so saying "NULL bits only for these three columns" is hardly ever cheaper than "NULL bits for everything". A single boolean is simpler to implement and takes less memory and almost as good as a bitmap.

On the other hand, FEDERATED and SPIDER, indeed, retrieve NULLs per column, so for them a per-colunm bitmap, as you did, would be clearly better. I'm just not sure it makes sense to go for a more complex and memory consuming implementation just for the sake of FEDERATED and SPIDER, they're few orders of magnitude less used than InnoDB or Aria. And every user will pay the complexity/memory price for the sake of those few who use FEDERATED or SPIDER.

But ok, it's a tradeoff, a design choice and you decided to do a bitmap. Let's keep it your way for now.

SET GLOBAL innodb_monitor_disable='module_buffer_page';
SET GLOBAL innodb_monitor_reset_all='module_buffer_page';
SET GLOBAL innodb_monitor_enable='module_buffer_page';
--enable_warnings
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you need to do it here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed it now.

(4, NULL, 4);

--let $restart_noprint=2
--echo # Restart to flush buffer pool state before measuring blob page reads.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

(3, REPEAT('b', 100000), 3),
(4, NULL, 4);

--let $restart_noprint=2
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's intentional because the blob-page-read assertions make sense from a cold buffer-pool state, otherwise setup-time caching can make the metric stay at zero, for the wrong reason. so have kept the restart.


FLUSH STATUS;
SELECT COUNT(*) FROM t WHERE b IS NULL;
SELECT VARIABLE_VALUE > 0 AS null_only_used
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why > 0 ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to prove something was counted, but now it seems it was too weak for these positive cases. I changed those checks to assert the raw Null_only_columns value directly, and the expected count is 1 in each of those single-column cases

FROM information_schema.session_status
WHERE VARIABLE_NAME='Null_only_columns';

SELECT SUM(COUNT) AS blob_reads
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why SUM?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kept because the test needs one total across the three relevant InnoDB blob-page-read counters: buffer_page_read_blob, buffer_page_read_zblob, and buffer_page_read_zblob2. I added a comment in the test to make that explicit.


SELECT SUM(payload)
FROM t_icp FORCE INDEX (idx_cd)
WHERE c > 0 AND d IS NULL;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you must have the same query in EXPLAIN and here, otherwise EXPLAIN makes no sense

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


SELECT VARIABLE_VALUE > 0 AS icp_used
FROM information_schema.session_status
WHERE VARIABLE_NAME='HANDLER_ICP_ATTEMPTS';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where's innodb_metrics query? why do you check for HANDLER_ICP_ATTEMPTS and not for null_only_columns?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it checks for null_only_columns s well

(4, NULL);

--let $restart_parameters=--innodb-buffer-pool-load-at-startup=0 --innodb-buffer-pool-dump-at-shutdown=0
--echo # Restart to validate virtual-column guardrail with cold-page metrics.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why??

SET GLOBAL innodb_monitor_disable='module_buffer_page';
SET GLOBAL innodb_monitor_reset_all='module_buffer_page';
SET GLOBAL innodb_monitor_enable='module_buffer_page';
--enable_warnings
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why???

SELECT SUM(COUNT) > 0 AS blob_reads
FROM information_schema.innodb_metrics
WHERE NAME IN ('buffer_page_read_blob','buffer_page_read_zblob',
'buffer_page_read_zblob2');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where's the status query?

@vuvova
Copy link
Copy Markdown
Member

vuvova commented Apr 8, 2026

On the other hand, FEDERATED and SPIDER, indeed, retrieve NULLs per column, so for them a per-colunm bitmap, as you did, would be clearly better. I'm just not sure it makes sense to go for a more complex and memory consuming implementation just for the sake of FEDERATED and SPIDER, they're few orders of magnitude less used than InnoDB or Aria. And every user will pay the complexity/memory price for the sake of those few who use FEDERATED or SPIDER.

But ok, it's a tradeoff, a design choice and you decided to do a bitmap. Let's keep it your way for now.

On yet another hand, if you chose an approach that specifically helps only FEDERATED and SPIDER and is detrimental for InnoDB/MyISAM/Aria, then your PR must include support for FEDERATED and Spider and not only for InnoDB/MyISAM/Aria.

@alexaustin007
Copy link
Copy Markdown
Author

On the other hand, FEDERATED and SPIDER, indeed, retrieve NULLs per column, so for them a per-colunm bitmap, as you did, would be clearly better. I'm just not sure it makes sense to go for a more complex and memory consuming implementation just for the sake of FEDERATED and SPIDER, they're few orders of magnitude less used than InnoDB or Aria. And every user will pay the complexity/memory price for the sake of those few who use FEDERATED or SPIDER.
But ok, it's a tradeoff, a design choice and you decided to do a bitmap. Let's keep it your way for now.

On yet another hand, if you chose an approach that specifically helps only FEDERATED and SPIDER and is detrimental for InnoDB/MyISAM/Aria, then your PR must include support for FEDERATED and Spider and not only for InnoDB/MyISAM/Aria.

I haven’t added it in this patch yet. I’ll implement support for FEDERATED and SPIDER as well after the current review is complete.

@alexaustin007 alexaustin007 requested a review from vuvova April 15, 2026 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

4 participants