Hi, Alexander! Thank you for working on this. I believe this is a very interesting patch, which significantly improves our FDW-based distributed facilities. This is why I decided to review this.
On Fri, Jan 20, 2023 at 11:00 AM Alexander Pyhalov <a.pyha...@postgrespro.ru> wrote: > Tomas Vondra писал 2023-01-19 20:49: > > I took a quick look at the patch. It needs a rebase, although it > > applies > > fine using patch. > > > > A couple minor comments: > > > > 1) addl_conds seems a bit hard to understand, I'd use either the full > > wording (additional_conds) or maybe extra_conds > > Renamed to additional_conds. > > > > > 2) some of the lines got quite long, and need a wrap > Splitted some of them. Not sure if it's enough. > > > > > 3) unknown_subquery_rels name is a bit misleading - AFAIK it's the rels > > that can't be referenced from upper rels (per what the .h says). So > > they > > are known, but hidden. Is there a better name? > > Renamed to hidden_subquery_rels. These are rels, which can't be referred > to from upper join levels. > > > > > 4) joinrel_target_ok() needs a better comment, explaining *when* the > > reltarget is safe for pushdown. The conditions are on the same row, but > > the project style is to break after '&&'. > > Added comment. It seems to be a rephrasing of lower comment in > joinrel_target_ok(). + /* + * We can't push down join if its reltarget is not safe + */ + if (!joinrel_target_ok(root, joinrel, jointype, outerrel, innerrel)) return false; As I get joinrel_target_ok() function do meaningful checks only for semi join and always return false for all other kinds of joins. I think we should call this only for semi join and name the function accordingly. + fpinfo->unknown_subquery_rels = bms_union(fpinfo_o->unknown_subquery_rels, + fpinfo_i->unknown_subquery_rels); Should the comment before this code block be revised? + case JOIN_SEMI: + fpinfo->joinclauses = list_concat(fpinfo->joinclauses, + fpinfo_i->remote_conds); + fpinfo->joinclauses = list_concat(fpinfo->joinclauses, + fpinfo->remote_conds); + fpinfo->remote_conds = list_copy(fpinfo_o->remote_conds); + fpinfo->unknown_subquery_rels = bms_union(fpinfo->unknown_subquery_rels, + innerrel->relids); + break; I think that comment before switch() should be definitely revised. + Relids hidden_subquery_rels; /* relids, which can't be referred to + * from upper relations */ Could this definition contain the positive part? Can't be referred to from upper relations, but used internally for semi joins (or something like that)? Also, I think the machinery around the append_conds could be somewhat simpler if we turn them into a list (list of strings). I think that should make code clearer and also save us some memory allocations. In [1] you've referenced the cases, when your patch can't push down semi-joins. It doesn't seem impossible to handle these cases, but that would make the patch much more complicated. I'm OK to continue with a simpler patch to handle the majority of cases. Could you please add the cases, which can't be pushed down with the current patch, to the test suite? Links 1. https://www.postgresql.org/message-id/816fa8b1bc2da09a87484d1ef239a332%40postgrespro.ru ------ Regards, Alexander Korotkov