Re: Add semi-join pushdown to postgres_fdw

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(dot)pyhalov(at)postgrespro(dot)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