Need more robust read/write bit tests for Field and derivatives

Registered by Jay Pipes

At issue is the prior macros, previously defined in field.cc:

#define ASSERT_COLUMN_MARKED_FOR_READ DBUG_ASSERT(!table || (!table->read_set || bitmap_is_set(table->read_set, field_index)))
#define ASSERT_COLUMN_MARKED_FOR_WRITE DBUG_ASSERT(!table || (!table->write_set || bitmap_is_set(table->write_set, field_index)))

When replacing DBUG_ASSERT calls with standard assert, tests failed spectacularly for some unknown reason (core dumps in 10/14 tests)

The macros are ostensibly designed to ensure that when certain Field:: methods are called, that the storage engine certifies the field in a table is writeable or readable. For instance, in Field::store(), the ASSERT_COLUMN_MARKED_FOR_WRITE() macro is called, which does a bare-metal test to see if the table is indeed instantiated and the field in question is writeable.

However, the logic in the macros is weird -- it's essentially a triple-negative logical test.

Jay thinks the test should instead be along these lines:

assert(table && table->write_set && bitmap_is_set(table->write_set, field_index)))

But, for now, these macro tests have been removed to finalize the remove-dbug tree and push forward.

Blueprint information

Status:
Not started
Approver:
None
Priority:
Low
Drafter:
None
Direction:
Needs approval
Assignee:
Patrick Crews
Definition:
Pending Approval
Series goal:
Accepted for trunk
Implementation:
Unknown
Milestone target:
milestone icon future

Related branches

Sprints

Whiteboard

> assert(table && table->write_set && bitmap_is_set(table->write_set, field_index)))

In the original assert, table being NULL is fine. In yours, it's not.
I think the original one is good.

Olaf

(?)

Work Items

This blueprint contains Public information 
Everyone can see this information.

Subscribers

No subscribers.