From: Antonin Houska <ah@cybertec.at>
To: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Unclear regression test for postgres_fdw
In-reply-to: 
 <CAM2+6=UGBx+WhUnX9O+vYEgtSeaLP-SKmKMVBNcSOFRqwDCL=g@mail.gmail.com>
References: <15265.1511985971@localhost>
 <CAM2+6=Um1iMhWhPHqz8v=4aYNS_+d9-fE4dcaLF4dEoXFoyZkA@mail.gmail.com>
 <CAM2+6=UGBx+WhUnX9O+vYEgtSeaLP-SKmKMVBNcSOFRqwDCL=g@mail.gmail.com>
Comments: In-reply-to Jeevan Chalke <jeevan.chalke@enterprisedb.com>
   message dated "Thu, 30 Nov 2017 16:06:40 +0530."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Fri, 01 Dec 2017 10:01:34 +0100
Message-ID: <16549.1512118894@localhost>
Precedence: bulk

Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

> On Thu, Nov 30, 2017 at 3:44 PM, Jeevan Chalke <jeevan.chalke@enterprised=
b.com> wrote:
>=20
>  On Thu, Nov 30, 2017 at 1:36 AM, Antonin Houska <ah@cybertec.at> wrote:
>=20
>  The following test
>=20
>  -- Input relation to aggregate push down hook is not safe to pushdown an=
d thus
>  -- the aggregate cannot be pushed down to foreign server.
>  explain (verbose, costs off)
>  select count(t1.c3) from ft1 t1, ft1 t2 where t1.c1 =3D postgres_fdw_abs=
(t1.c2);
>=20
>  produces the following plan
>=20
>  QUERY PLAN
>  ------------------------------------------------------------------------=
----------------------------------
>  Aggregate
>  Output: count(t1.c3)
>  -> Nested Loop
>  Output: t1.c3
>  -> Foreign Scan on public.ft1 t2
>  Remote SQL: SELECT NULL FROM "S 1"."T 1"
>  -> Materialize
>  Output: t1.c3
>  -> Foreign Scan on public.ft1 t1
>  Output: t1.c3
>  Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1" =3D public.postgres=
_fdw_abs(c2)))
>=20
>  which is not major problem as such, but gdb shows that the comment "aggr=
egate
>  cannot be pushed" is not correct. In fact, postgresGetForeignUpperPaths()
>  *does* create the upper path.
>=20
>  The reason that UPPERREL_GROUP_AGG is eventually not used seems to be th=
at
>  postgresGetForeignJoinPaths() -> add_foreign_grouping_paths() ->
>  estimate_path_cost_size() estimates the join cost in rather generic way.=
 While
>  the remote server can push the join clause down to the inner relation of=
 NL,
>  the postgres_fdw cost computation assumes that the join clause is applie=
d to
>  each pair of output and input tuple.
>=20
>  I don't think that the postgres_fdw's estimate can be fixed easily, but =
if the
>  impact of "shipability" on (not) using the upper relation should be test=
ed, we
>  need a different test.
>=20
>  Oops. My bad.
>  Agree with your analysis.
>  Will send a patch fixing this testcase.
>=20
> Attached patch to fix the test case. In new test case I am using a JOIN
> query where JOIN condition is not safe to push down and hence the JOIN
> itself is unsafe.

I've just verified that postgresGetForeignUpperPaths() does return here:

	/*
	 * If input rel is not safe to pushdown, then simply return as we cannot
	 * perform any post-join operations on the foreign server.
	 */
	if (!input_rel->fdw_private ||
		!((PgFdwRelationInfo *) input_rel->fdw_private)->pushdown_safe)
		return;

I see no other problems here. You may need to add the patch to the next CF =
so
it does not get lost.

--=20
Antonin Houska
Cybertec Sch=C3=B6nig & Sch=C3=B6nig GmbH
Gr=C3=B6hrm=C3=BChlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at