From: Antonin Houska <ah@cybertec.at>
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
cc: Tom Lane <tgl@sss.pgh.pa.us>,
    Andrew Gierth <andrew@tao11.riddles.org.uk>,
    pgsql-hackers@postgresql.org
Subject: Re: Problems with plan estimates in postgres_fdw
In-reply-to: <5C5D778F.3080501@lab.ntt.co.jp>
References: <87pnz1aby9.fsf@news-spur.riddles.org.uk>
 <1175.1533220908@sss.pgh.pa.us> <5BB739CD.6040002@lab.ntt.co.jp>
 <5BBC4140.50403@lab.ntt.co.jp> <5C17A00E.5000005@lab.ntt.co.jp>
 <5C232F39.9060509@lab.ntt.co.jp> <5C25C7B5.2010802@lab.ntt.co.jp>
 <5C518631.5080401@lab.ntt.co.jp> <10994.1549559088@localhost>
 <5C5D778F.3080501@lab.ntt.co.jp>
Comments: In-reply-to Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
   message dated "Fri, 08 Feb 2019 21:35:27 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Tue, 12 Feb 2019 10:03:30 +0100
Message-ID: <19433.1549962210@localhost>
Precedence: bulk

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:

> (2019/02/08 2:04), Antonin Houska wrote:
>=20
> > First, I wonder if the information on LIMIT needs to be passed to the
> > FDW. Cannot the FDW routines use root->tuple_fraction?
>=20
> I think it's OK for the FDW to use the tuple_fraction, but I'm not sure t=
he
> tuple_fraction should be enough.  For example, I don't think we can re-co=
mpute
> from the tuple_fraction the LIMIT/OFFSET values.
>=20
> > I think (although have
> > not verified experimentally) that the problem reported in [1] is not li=
mited
> > to the LIMIT clause. I think the same problem can happen if client uses=
 cursor
> > and sets cursor_tuple_fraction very low. Since the remote node is not a=
ware of
> > this setting when running EXPLAIN, the low fraction can also make postg=
res_fdw
> > think that retrieval of the whole set is cheaper than it will appear to=
 be
> > during the actual execution.
>=20
> I think it would be better to get a fast-start plan on the remote side in=
 such
> a situation, but I'm not sure we can do that as well with this patch, bec=
ause
> in the cursor case, the FDW couldn't know in advance exactly how may rows=
 will
> be fetched from the remote side using that cursor, so the FDW couldn't pu=
sh
> down LIMIT.  So I'd like to leave that for another patch.

root->tuple_fraction (possibly derived from cursor_tuple_fraction) should be
enough to decide whether fast-startup plan should be considered. I just fai=
led
to realize that your patch primarily aims at the support of UPPERREL_ORDERED
and UPPERREL_FINAL relations by postgres_fdw. Yes, another patch would be
needed to completely resolve the problem reported by Andrew Gierth upthread.

> > Some comments on coding:
> >
> > 0001-postgres_fdw-Perform-UPPERREL_ORDERED-step-remotely-v3.patch
> > -----------------------------------------------------------------
> >
> > * I'm not sure if UPPERREL_ORDERD should to deal with LIMIT at all. Not=
e that
> > grouping_planner() does not call create_limit_path() until it's done wi=
th
> > create_ordered_paths(), and create_ordered_paths() is where the FDW is
> > requested to add its paths to UPPERREL_ORDERED relation.
>=20
> Maybe I'm missing something, but the 0001 patch doesn't deal with LIMIT at
> all.  I added the parameter limit_tuples to PgFdwPathExtraData so that we=
 can
> pass limit_tuples=3D-1, which means no LIMIT, to cost_sort(), though.

Yes, the limit_tuples field made me confused. But if cost_sort() is the only
reason, why not simply pass -1 in the 0001 patch, and use the limit_tuples
argument only in 0002? I see several other calls of cost_sort() in the core
where -1 is hard-coded.

> > Both patches:
> > ------------
> >
> > One thing that makes me confused when I try to understand details is th=
at the
> > new functions add_foreign_ordered_paths() and add_foreign_final_paths()=
 both
> > pass "input_rel" for the "foreignrel" argument of estimate_path_cost_si=
ze():
> >
> > estimate_path_cost_size(root, input_rel, ...)
> >
> > whereas the existing add_foreign_grouping_paths() passes "grouped_rel":
> >
> > estimate_path_cost_size(root, grouped_rel, ...)
> >
> > Can you please make this consistent? If you do, you'll probably need to
> > reconsider your changes to estimate_path_cost_size().
>=20
> This is intended and I think I should add more comments on that.  Let me
> explain.  These patches modified estimate_path_cost_size() so that we can
> estimate the costs of a foreign path for the UPPERREL_ORDERED or
> UPPERREL_FINAL rel, by adding the costs of the upper-relation processing =
(ie,
> ORDER BY and/or LIMIT/OFFSET, specified by PgFdwPathExtraData) to the cos=
ts of
> implementing the *underlying* relation for the upper relation (ie, scan, =
join
> or grouping rel, specified by the input_rel). So those functions call
> estimate_path_cost_size() with the input_rel, not the ordered_rel/final_r=
el,
> along with PgFdwPathExtraData.

I think the same already happens for the UPPERREL_GROUP_AGG relation:
estimate_path_cost_size()

	...
	else if (IS_UPPER_REL(foreignrel))
	{
		...
		ofpinfo =3D (PgFdwRelationInfo *) fpinfo->outerrel->fdw_private;

Here "outerrel" actually means input relation from the grouping perspective.
The input relation (i.e. result of scan / join) estimates are retrieved from
"ofpinfo" and the costs of aggregation are added. Why should this be differ=
ent
for other kinds of upper relations?

> > And maybe related problem: why should FDW care about the effect of
> > apply_scanjoin_target_to_paths() like you claim to do here?
> >
> > 	/*
> > 	 * If this is UPPERREL_ORDERED and/or UPPERREL_FINAL steps on the
> > 	 * final scan/join relation, the costs obtained from the cache
> > 	 * wouldn't yet contain the eval cost for the final scan/join target,
> > 	 * which would have been updated by apply_scanjoin_target_to_paths();
> > 	 * add the eval cost now.
> > 	 */
> > 	if (fpextra&&  !IS_UPPER_REL(foreignrel))
> > 	{
> > 		/* The costs should have been obtained from the cache. */
> > 		Assert(fpinfo->rel_startup_cost>=3D 0&&
> > 			fpinfo->rel_total_cost>=3D 0);
> >
> > 		startup_cost +=3D foreignrel->reltarget->cost.startup;
> > 		run_cost +=3D foreignrel->reltarget->cost.per_tuple * rows;
> > 	}
> >
> > I think it should not, whether "foreignrel" means "input_rel" or "outpu=
t_rel"
> > from the perspective of postgresGetForeignUpperPaths().
>=20
> I added this before costing the sort operation below, because 1) the cost=
 of
> evaluating the scan/join target might not be zero (consider the case where
> sort columns are not simple Vars, for example) and 2) the cost of sorting
> takes into account the underlying path's startup/total costs.  Maybe I'm
> missing something, though.

My understanding is that the "input_rel" argument of
postgresGetForeignUpperPaths() should already have been processed by
postgresGetForeignPaths() or postgresGetForeignJoinPaths() (or actually by
postgresGetForeignUpperPaths() called with different "stage" too). Therefore
it should already have the "fdw_private" field initialized. The estimates in
the corresponding PgFdwRelationInfo should already include the reltarget
costs, as set previously by estimate_path_cost_size().

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