Re: Add semi-join pushdown to postgres_fdw

From: Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
To: Alexander Korotkov <aekorotkov(at)gmail(dot)com>
Cc: Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, "Fujii(dot)Yuki(at)df(dot)MitsubishiElectric(dot)co(dot)jp" <Fujii(dot)Yuki(at)df(dot)mitsubishielectric(dot)co(dot)jp>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>, Ian Lawrence Barwick <barwick(at)gmail(dot)com>
Subject: Re: Add semi-join pushdown to postgres_fdw
Date: 2023-10-31 11:07:56
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Alexander Korotkov писал 2023-10-30 19:05:
> 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.

Hi. Thanks for reviewing.

> + /*
> + * 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?

Updated comment.

> + 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)?

Made comment a bit more verbose.

> 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.

I've tried to rewrite it as managing lists.. to find out that these are
not lists.
I mean, in deparseFromExprForRel() we replace lists from both side with
one condition.
This allows us to preserve conditions hierarchy. We should merge these
in the end of IS_JOIN_REL(foreignrel) branch, or we'll push them too
high. And if we
deparse them in this place as StringInfo, I see no benefit to convert
them to lists.

> 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?

There are several cases when we can't push down semi-join in current

1) When target list has attributes from inner relation, which are
equivalent to some attributes of outer
relation, we fail to notice this.

2) When we examine A join B and decide that we can't push it down, this
decision is final - we state it in fdw_private of joinrel,
and so if we consider joining these relations in another order, we don't
This means that if later examine B join A, we don't try to push it down.
As semi-join can be executed as JOIN_UNIQUE_INNER or JOIN_UNIQUE_OUTER,
this can be a problem - we look at some of these paths and remember that
we can't push down such join.

Best regards,
Alexander Pyhalov,
Postgres Professional

Attachment Content-Type Size
v5-0001-postgres_fdw-add-support-for-deparsing-semi-joins.patch text/x-diff 51.9 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2023-10-31 11:17:38 Re: A recent message added to pg_upgade
Previous Message Amit Kapila 2023-10-31 10:45:13 Re: Synchronizing slots from primary to standby