MySQL Bugs: #106203: SQL_delete_cmd::delete_from_single_table problem with ECP causes deletes to fail

Bug #106203	SQL_delete_cmd::delete_from_single_table problem with ECP causes deletes to fail
Submitted:	18 Jan 2022 21:56	Modified:	1 Feb 2022 12:27
Reporter:	Justin Swanhart	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: DML	Severity:	S2 (Serious)
Version:		OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
The problem code is here:
https://github.com/mysql/mysql-server/blob/8.0/sql/sql_delete.cc#L630

This block of code checks if an attached condition exists that it evaluates to something other than non-zero.  WARP attaches conditions that do evaluate to zero through this check, and the storage engine skips the rows that are supposed to be deleted.  

-- with the unaltered server code
mysql> insert into ontime select * from sample;
Query OK, 1 row affected (0.04 sec)
Records: 1  Duplicates: 0  Warnings: 0

-- as you can see no rows are deleted
mysql> delete from ontime where year=1987 and OriginAirportId=12892;
Query OK, 0 rows affected (0.04 sec)

-- after recompiling and removing this code so that skip_record=false
mysql> delete from ontime where year=1987 and OriginAirportId=12892;
ERROR 2013 (HY000): Lost connection to MySQL server during query
No connection. Trying to reconnect...
Connection id:    8
Current database: ontime

Query OK, 1 rows affected (0.07 sec)

How to repeat:
Use a storage engine that implements ECP other than NDB.

Suggested fix:
Make this code somehow non-NDB specific or tell me what to do so that the conditions I have attached via ECP don't evaluate to zero.  I think this is happening because all of the conditions are pushed down and thus the conditions evaluates to zero (I assume it returns the number of unpushed operations)?

It has to evaluate to something non-zero.  

->val_int() returns 0 in WARP

This only happens when filter conditions are placed on the table.  
If I do "delete from table ontime" it works fine, but as soon as I add conditions that can be satisfied via ECP this block of code makes it so that no rows are deleted.  This code was added somewhat recently I think, perhaps added in 8.0.25 because WARP did not experience this problem prior to that version, at least I do not think it did.  I just added some new tests that discovered this issue.

In my storage engine I only attach conditions that could not be pushed down:
  table_aqp->set_condition(const_cast<Item *>(remainder));

so the conditions are a NULL pointer I guess when all the conditions have been pushed down?  I assume this is where the conditions comes from that is being checked to be non-zero.

The code change in WarpSQL is here.  This isn't a suggested patch, just showing what I did to fix the problem temporarily:
https://github.com/greenlion/warp/blob/stability/sql/sql_delete.cc#L519

I added code to print out the conditions (leaving the block commented)
mysql> delete from ontime where year=1987 and OriginAirportId=12892;
Query OK, 3 rows affected (0.05 sec)

-- server output
AND
Arg0: (`ontime`.`ontime`.`OriginAirportID` = 12892)
Arg1: (`ontime`.`ontime`.`Year` = 1987)
Arg0 used_tables:1
Arg1 used_tables:1
Attached condition:
((ontime.OriginAirportID = 12892) and (ontime.`Year` = 1987))
Attached condition:
((ontime.OriginAirportID = 12892) and (ontime.`Year` = 1987))
Attached condition:
((ontime.OriginAirportID = 12892) and (ontime.`Year` = 1987))

so ->val_int on that complex condition returns 0

Hi,

Just to notify you that this issue is still under analysis.

Hi Mr. Swanhart,

We do need some additional explanations from you.

It is true that currently only NDB SE supports ECP (Engine Condition Pushdown), but it is not clear to us how would you like us to change that particular method so that it accommodates ECP for your storage engine, NDB SE and any future SE that will support that algorithm, like for example, InnoDB SE.

In other words, what is it EXACTLY that you want us to change.

Your feedback is eagerly expected.

It would be also nice to know HOW exactly do you envisage those changes ????

Hello, first, thank you for analyzing this report and getting back to me.

I am not sure what I am doing wrong in my storage engine that makes it fail this check.  I attach conditions in engine_push and those attached conditions cause this check to fail.  

I do not handle conditions like NDB handles conditions.  My storage engine speaks a dialect of SQL "under the hood" and it removes any conditions from the WHERE clause that it can satisfy without the SQL layer handling them.  I think this is correct.

So when my code gets to this check, it has a complex attached condition, it is usually a ITEM_COND.  I don't see how a complex expression like an AND can evaluate to 1 through ->val_int() but I will have to look at the AQP code, perhaps I am not doing something I am supposed to be doing.  My ECP code is complex but it works for SELECT statements but not DELETE statements.  

I don't have a suggestion on how to change YOUR code, I really need a suggestion about how to change mine I guess because my usage of the ECP interface is apparently not conforming to the MySQL expectations but unfortunately this interface is not documented anywhere.

There are no comments in this code.  Why does ->val_int() on the condition have to evaluate to zero, otherwise the record is skipped?  Does NDB somehow modify the attached conditions after engine_push or does my engine work completely differently?  I just don't know.  Any assistance you can provide in understanding what this check means would be helpful.

Hi,

We do need certain time in order to provide you a proper answer .......

A short answer on one of your question.

The line that you are referring to is this one:

const bool skip_record = conds->val_int() == 0;

Hence, that particular record should not be deleted if val_int() results in zero. That is quite logical .....

the condition is:
year = X and flightid = Y

how can that evaluate to 0 or 1 - it is just a set of item conditions.  this is equivalent to WHERE 1

I am not going to have just a 1 constant int in my expressions, and it will never just be that expression.  That is why this is unclear.  I'm not a moron :)

This is my condition:
FIELD = CONST AND FIELD       = CONST
  |       |      |        
 Year = 1234   OriginFlightId = 1234

It is an array of Item* - I don't see how that evaluates to non-zero or zero.  Since it is just an Item* I don't really have much code to inspect and why I am very confused.

FWIW this is a snippet tcode I have for ha::engine_push.  Note that I still have cond_push implemented and I just use it as the engine would have used it.  I call cond_push and it returns the remaining items that could not be pushed down.

 
  THD const *thd = table->in_use;
  if(thd->optimizer_switch_flag(OPTIMIZER_SWITCH_ENGINE_CONDITION_PUSHDOWN)) {
    const Item *cond = table_aqp->get_condition();
    if(cond == nullptr) return 0;

    remainder = cond_push(cond, true);
    
    table_aqp->set_condition(const_cast<Item *>(remainder));
   }

So (remainder) in this case is the array of Item* that represent unpushed conditions as explained above.

To be clear, it is evaluating the first item on the *Item stack that is assigned with set_condition.  Since this is an array of items, this is checking the first item to see if ->val_int !=0

Since the first item in my stack is an AND of course an AND does not evaluate with ->val_int() != 0  

I hope I have made my problem clear enough and you can hopefully tell me what I am doing wrong with set_condition.  I can't just add a 1 as the first item in the stack, it would always be an AND or an OR that is the first item in the stack.

I looked at ::engine_push in NDB and it is not terribly different from mine, so I am not sure exactly how the code is supposed to work with NDB either.  Anyway, please let me know what you find, no rush.

Hi Justin, just to be sure we are on the same page: The condition attached to the table to be deleted from is either a predicate, or a set of predicates combined with AND and OR. We call val_int() on it, but it should rather be val_bool(), which would return false (0) if the condition is rejected or true (1) if the condition is accepted.

Thus, if val_int() returns 0, the row is skipped because it does not qualify to user-supplied conditions.

I don't recall working on the ECP pushdown feature myself, as this feature is only used with NDB in our product (to my knowledge), thus I am not sure how to advice you on this.

If there was a regression on our side, do you have an idea on what it was?

Any row found by ::rnd_next is supposed to be deleted, correct?  Am I expected to set this condition to true or false in ::rnd_next? I will check to see if NDB does that in ::rnd_next.

This is what really confuses me, because the contract is that whatever row is current in ::rnd_next (i call find_current_row() here like CSV does) gets deleted, at least without ECP.  This is why my SE broke, because that contract is no longer met.

This code would likely have been added at the same time as ::engine_push was added to the SE interface.

Hi Justin, still a bit of guesswork from my side, but imagine that we have this
condition:

  "a = 1 and b = 2 and c = 3 and d = 4".

In MySQL, this would be an Item_cond_and with four predicates.
Now, imagine that the two first predicates can be fulfilled by your engine.
push_to_engine would then have to identify these predicates, probably by building
a new Item_cond_and with these predicates. We also need the remainder (the last two predicates) in another Item_cond_and. This one will replace the original condition in MySQL.

To fulfill the pushed predicate in your engine, you will then have to do something like this in your driver:

  int rnd_next(...)
  while (true) {
    get next row from table.
    int result = pushded_down_condition->val_int();
    if (error) exit with error code;
    if (result) break; // Row matches the pushed down condition, so give it to MySQL
    // val_int() returned 0 (false) so repeat with next row;
  }
  return <proper return code>.

On the MySQL side, we get the row from your engine. If there is a remainder condition, it will be evaluated using val_int(), and again the row will qualify if the return value from val_int() is non-zero, and the row should be deleted.

Hi,

First, I have no access to the condition in ::rnd_next
Second, I find a row just like you suggest, I skip any rows that do not match my condition, rows that are deleted, rows that are not visible to the trx, and the matching row returns 0.  I only ever return either 0 or HA_ERR_END_OF_FILE at the end of ::rnd_next.  

I just do not evaluate the condition using any MySQL functions.  
My conditions are evaluated by passing them in as a WHERE clause to my engine.  Ie:
select count(*) from mysql_table where x= 1 and y=2 becomes
select 0 from internal_table where c0=1 or c0=2

I expect MySQL to match any non-pushed portion at the SE layer (it does).

This works for SELECT, it just does not work for DELETE.  It appears to work for UPDATE too.

I will see what I can do with the info.  Thanks.

sorry :)
select count(*) from mysql_table where x= 1 and y=2 becomes
select 0 from internal_table where c0=1 AND c1=2

My storage engine identifies columns by ordinal position.  So I am just really pushing down SQL to my engine in ECP.  Any row that matches rnd_next should get deleted.

I think I solved this problem for my storage engine.  The condition evaluated here is not the same (necessarily) as the condition set in ::engine_push.  I believe that this ->val_int() is testing the actual DELETE conditions when they are not pushed, and it was failing in my engine because I was not populating the row for the DELETE statement.  I am going to close this bug now, and if I continue to experience confusion or problems, I may open it back up.

Thank you for the feedback, Justin .....