On Sun, 13 Jun 2021 at 21:28, Tomas Vondra <tomas.von...@enterprisedb.com> wrote: > > Here is a slightly updated version of the patch >
Hi, I have looked at this in some more detail, and it all looks pretty good, other than some mostly cosmetic stuff. The new code in statext_is_compatible_clause_internal() is a little hard to follow because some of the comments aren't right (e.g. when checking clause_expr2, it isn't an (Expr op Const) or (Const op Expr) as the comment says). Rather than trying to comment on each conditional branch, it might be simpler to just have a single catch-all comment at the top, and also remove the "return true" in the middle, to make it something like: /* * Check Vars appearing on either side by recursing, and make a note of * any expressions. */ if (IsA(clause_expr, Var)) { if (!statext_is_compatible_clause_internal(...)) return false; } else *exprs = lappend(*exprs, clause_expr); if (clause_expr2) { if (IsA(clause_expr2, Var)) { if (!statext_is_compatible_clause_internal(...)) return false; } else *exprs = lappend(*exprs, clause_expr2); } return true; Is the FIXME comment in examine_opclause_args() necessary? The check for a single relation has already been done in clause[list]_selectivity_ext(), and I'm not sure what examine_opclause_args() would do differently. In mcv_get_match_bitmap(), perhaps do the RESULT_IS_FINAL() checks first in each loop. Also in mcv_get_match_bitmap(), the 2 "First check whether the constant is below the lower boundary ..." comments don't make any sense to me. Were those perhaps copied and pasted from somewhere else? They should perhaps say "Otherwise, compare the MCVItem with the constant" and "Otherwise compare the values from the MCVItem using the clause operator", or something like that. But other than such cosmetic things, I think the patch is good, and gives some nice estimate improvements. Regards, Dean