MySQL Bugs: #104574: condition seems not correct

Bug #104574	condition seems not correct
Submitted:	9 Aug 2021 17:56	Modified:	23 Aug 2021 19:35
Reporter:	Weidong Yu	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Optimizer	Severity:	S2 (Serious)
Version:	8.0	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	condition check

Description:
Recently downloaded 8.0.26, the last commit
commit beb865a960b9a8a16cf999c323e46c5b0c67f21f
Author: mysql-builder@oracle.com <>
Date:   Thu Jul 1 07:15:50 2021 +0200

Just read sql_derived.cc
1141 bool Condition_pushdown::replace_columns_in_cond(Item **cond, bool is_having) {
1142   // For a view reference, the underlying expression could be shared if the
1143   // expression is referenced elsewhere in the query. So we clone the expression
1144   // before replacing it with derived table expression.
1145   bool view_ref = false;
1146   WalkItem((*cond), enum_walk::PREFIX, [&view_ref](Item *inner_item) {
1147     if (inner_item->type() == Item::REF_ITEM &&
1148         down_cast<Item_ref *>(inner_item)->ref_type() == Item_ref::VIEW_REF) {
1149       view_ref = true;
1150       return true;
1151     }
1152     return false;
1153   });
1154   if (view_ref) {
1155     (*cond) = (*cond)->transform(&Item::replace_view_refs_with_clone,
1156                                  pointer_cast<uchar *>(m_derived_table));
1157     if (cond == nullptr) return true;
1158   }
1159   Item *new_cond =
1160       is_having ? (*cond)->transform(&Item::replace_with_derived_expr_ref,
1161                                      pointer_cast<uchar *>(m_derived_table))
1162                 : (*cond)->transform(&Item::replace_with_derived_expr,
1163                                      pointer_cast<uchar *>(m_derived_table));
1164   if (new_cond == nullptr) return true;
1165   new_cond->update_used_tables();
1166   (*cond) = new_cond;
1167   return false;
1168 }

Line 1157, whether is it "if (*cond == nullptr) return true;" instead of cond == nullptr

How to repeat:
Just read code, no test case

Hi Mr. Yu,

Thank you very much for your bug report.

We do have a small problem with your proposal.

In the case that the pointer is not NULL, your proposition would leave the method with the value of false. However, this should not be done, since there is still some code to be executed.

If, on the other hand, you would propose a new block when the pointer is not NULL, then there would be no gain in speed, what so ever.

Hence, we do not see any SIGNIFICANT improvement in what you propose, but since we could be wrong, we are waiting on your feedback.

Please note LL1155 return is *cond, and 1157 is checking cond. It doesn't make sense not checking return. In fact, we know cond is never null, since it is address of an Item from caller function.

Currently we are using 8.0.22 to test TPCDS, and TPCDS39.2 got wrong result. We found the problem is fixed in 8.0.26, and I tried to apply the change of  bug32324234 and bug32905044. I may apply wrong, *cond is NULL, and it is not checking, and it causes crash at LL1160

Hi,

Please, clear up something for us. 

If this is fixed in 8.0.26, then this is not a bug.

The fact that line 1155 is returning the `cond` and that line 1157 is checking , that is a perfect solution. Nothing to improve. We can not assume that it will NEVER be NULL !!! Simply because code is changed constantly in so many different places that it is irresponsible to remove the check.

If, however, you are experiencing a crash in 8.0.26, on our unchanged code, we would very much like to have a test case.

Thanks in advance .....

RE: The fact that line 1155 is returning the `cond` and that line 1157 is checking , that is a perfect solution.

Please note: Line 1155 is returning the `*cond`, it is not `cond`. so line 1157 need to checking `*cond`, not `cond`

Hi Mr. Yu,

It is only now that I saw it.

Yes, you are quite right. 

It should be: 

if (*cond == nullptr) 

and not :

if (cond == nullptr)

Verified as reported !!!!!!

Thanks for your contribution.

Happy to know that you agree with me. Hopefully, I didn't make trouble for you

Hi,

Thanks for your concern, but bug reports, like this one, are truly welcome in our company.

Documented fix as follows in the MySQL 8.0.27 changelog:

    In certain cases, the view reference cloned when pushing a
    condition down to a derived table was not always resolved in the
    desired context. In addition, a check for a null condition was
    not performed correctly.

Closed.

Thank you, Jon.