Re: Getting sorted data from foreign server for merge join

Hi,
In get_useful_ecs_for_relation(), while checking whether to use left or
right argument of a mergejoinable operator, the arguments to
bms_is_subset() are passed in reverse order. bms_is_subset() checks whether
the first argument in subset of the second, but in this function the subset
to be checked is passed as the second argument. Because of this following
query when run in contrib_regression database after "make installcheck" in
contrib/postgres_fdw trips assertion Assert(bms_is_subset(relids,
restrictinfo->left_ec->ec_relids));

EXPLAIN (COSTS false, VERBOSE)
SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on
(t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;

PFA patch to fix it.

Reason why it was not caught earlier: this code is excercised when
expressions in left/right join clauses are considered. For mergejoinable
clauses, the left and right side are not merged into a single EC but appear
as separate ECs. All tests in the postgres_fdw.sql that excercised this
code involved only two relations, thus ec_relids and relids had only a
single member and bms_is_subset() returned true. But in the above query the
left/right EC has relids of t2 and t3, which caused bms_is_subset() to
return false and thus trip the assertion.

I have added above query and the output to the tests. The output of EXPLAIN
shows that ORDER BY clause is pushed down for ft2 but not ft1. This is
because ft2 has use_remote_estimate true and ft1 has that false. So, we
push down ORDER BY corresponding merge join for ft2 but not for ft1.

On Wed, Dec 23, 2015 at 12:24 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Dec 21, 2015 at 6:34 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> >> I went over this patch in some detail today and did a lot of cosmetic
> >> cleanup. The results are attached. I'm fairly happy with this
> >> version, but let me know what you think. Of course, feedback from
> >> others is more than welcome also.
> >>
> >
> > Attached patch with some cosmetic changes (listed here for your quick
> > reference)
> > 1. , was replaced with ; in comment "inner join, expressions in the " at
> one
> > place, which is correct, but missed other place.
> > 2. The comment "First, consider whether any each active EC is
> potentially"
> > should use either "any" or "each". I have reworded it as "First, consider
> > whether any of the active ECs is potentially ...". Or we can use "First,
> > find all of the active ECs which are potentially ....".
> > 3. "having the remote side due the sort generally won't be any worse
> ..." -
> > instead of "due" we should use "do"?
> > 4. Added static prototype of function get_useful_ecs_for_relation().
> > 5. The comment "Extract unique EC for query, if any, so we don't
> consider it
> > again." is too crisp. Phrase "Unique EC for query" is confusing; EC can
> not
> > be associated with a query per say and EC's are always unique because of
> > canonicalisation. May be we should reword it as "Extract single EC for
> > ordering of query, if any, so we don't consider it again." Is that
> cryptic
> > as well?
>
> Thanks. I committed this version with one small tweak.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company