On Fri, Jun 21, 2024 at 1:05 PM Alena Rybakina <a.rybak...@postgrespro.ru> wrote: > I'm confused, I have seen that we have two threads [1] and [2] about this > thread and I haven't found any explanation for how they differ. > > And I don't understand, why am I not listed as the author of the patch? I was > developing the first part of the patch before Andrey came to review it [3] > and his first part hasn't changed much since then.
v25 still lists you as an author (in fact, the first author) but I can't say why we have two CommitFest entries. Surely that's a mistake. On the patch itself, I'm really glad we got to a design where this is part of planning, not parsing. I'm not sure yet whether we're doing it at the right time within the planner, but I think this *might* be right, whereas the old way was definitely wrong. What exactly is the strategy around OR-clauses with type differences? If I'm reading the code correctly, the first loop requires an exact opno match, which presumably implies that the constant-type elements are of the same type. But then why does the second loop need to use coerce_to_common_type? Also, why is the array built with eval_const_expressions instead of something like makeArrayResult? There should be no need for general expression evaluation here if we are just dealing with constants. + foreach(lc2, entry->exprs) + { + RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc2); + + is_pushed_down = is_pushed_down || rinfo->is_pushed_down; + has_clone = has_clone || rinfo->is_pushed_down; + security_level = Max(security_level, rinfo->security_level); + required_relids = bms_union(required_relids, rinfo->required_relids); + incompatible_relids = bms_union(incompatible_relids, rinfo->incompatible_relids); + outer_relids = bms_union(outer_relids, rinfo->outer_relids); + } This seems like an extremely bad idea. Smushing together things with different security levels (or a bunch of these other properties) seems like it will break things. Presumably we wouldn't track these properties on a per-RelOptInfo basis unless we needed an accurate idea of the property value for each RelOptInfo. If the values are guaranteed to match, then it's fine, but then we don't need this code to merge possibly-different values. If they're not guaranteed to match, then presumably we shouldn't merge into a single OR clause unless they do. On a related note, it looks to me like the tests focus too much on simple cases. It seems like it's mostly testing cases where there are no security quals, no weird operator classes, no type mismatches, and few joins. In the cases where there are joins, it's an inner join and there's no distinction between an ON-qual and a WHERE-qual. I strongly suggest adding some test cases for weirder scenarios. + if (!OperatorIsVisible(entry->opno)) + namelist = lappend(namelist, makeString(get_namespace_name(operform->oprnamespace))); + + namelist = lappend(namelist, makeString(pstrdup(NameStr(operform->oprname)))); + ReleaseSysCache(opertup); + + saopexpr = + (ScalarArrayOpExpr *) + make_scalar_array_op(NULL, + namelist, + true, + (Node *) entry->expr, + (Node *) newa, + -1); I do not think this is acceptable. We should find a way to get the right operator into the ScalarArrayOpExpr without translating the OID back into a name and then back into an OID. + /* One more trick: assemble correct clause */ This comment doesn't seem to make much sense. Some other comments contain spelling mistakes. The patch should have comments in more places explaining key design decisions. +extern JumbleState *JumbleExpr(Expr *expr, uint64 *exprId); This is no longer needed. -- Robert Haas EDB: http://www.enterprisedb.com