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: <5C6BBD7A.1030702@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> <19433.1549962210@localhost>
 <5C669DFD.30002@lab.ntt.co.jp> <16616.1550234762@localhost>
 <5C6A5FD3.8080102@lab.ntt.co.jp> <30524.1550499671@localhost>
 <5C6BBD7A.1030702@lab.ntt.co.jp>
Comments: In-reply-to Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
   message dated "Tue, 19 Feb 2019 17:25:30 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <29189.1550843669.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 22 Feb 2019 14:54:29 +0100
Message-ID: <29190.1550843669@localhost>
Precedence: bulk

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

> Maybe, my explanation in that thread was not enough, but the changes pro=
posed
> by the patch posted there wouldn't obsolete the above.  Let me explain u=
sing a
> foreign-table variant of the example shown in the comments for
> make_group_input_target():
> =

>     SELECT a+b, sum(c+d) FROM foreign_table GROUP BY a+b;
> =

> When called from postgresGetForeignPaths(), the reltarget for the base
> relation foreign_table would be {a, b, c, d}, for the same reason as the=
 LIMIT
> example in [1], and the foreign_table's rel_startup_cost and rel_total_c=
ost
> would be calculated based on this reltarget in that callback routine.  (=
The
> tlist eval costs would be zeroes, though).  BUT: before called from
> postgresGetForeignUpperPaths() with the UPPERREL_GROUP_AGG stage, the
> reltarget would be replaced with {a+b, c, d}, which is the final scan ta=
rget
> as explained in the comments for make_group_input_target(), and the eval=
 costs
> of the new reltarget wouldn't be zeros, so the costs of scanning the
> foreign_table would need to be adjusted to include the eval costs.  That=
's why
> I propose the assignments in estimate_path_cost_size() shown above.

Yes, apply_scanjoin_target_to_paths() can add some more costs, including t=
hose
introduced by make_group_input_target(), which postgres_fdw needs to accou=
nt
for. This is what you fix in

https://www.postgresql.org/message-id/5C66A056.60007%40lab.ntt.co.jp

> On the other hand, the code bit added by
> 0001-postgres_fdw-Perform-UPPERREL_ORDERED-step-remotely-v3.patch handle=
s the
> case where a post-scan/join processing step other than grouping/aggregat=
ion
> (ie, the final sort or LIMIT/OFFSET) is performed directly on a base or =
join
> relation, as in the LIMIT example.  So, the two changes are handling dif=
ferent
> cases, hence both changes would be required.

Do you mean this part?

+       /*
+        * If this includes an UPPERREL_ORDERED step, the given target, wh=
ich
+        * would be the final target to be applied to the resulting path, =
might
+        * have different expressions from the underlying relation's relta=
rget
+        * (see make_sort_input_target()); adjust tlist eval costs.
+        */
+       if (fpextra && fpextra->target !=3D foreignrel->reltarget)
+       {
+               QualCost        oldcost =3D foreignrel->reltarget->cost;
+               QualCost        newcost =3D fpextra->target->cost;
+
+               startup_cost +=3D newcost.startup - oldcost.startup;
+               total_cost +=3D newcost.startup - oldcost.startup;
+               total_cost +=3D (newcost.per_tuple - oldcost.per_tuple) * =
rows;
+       }

You should not need this. Consider what grouping_planner() does if
(!have_grouping && !activeWindows && parse->sortClause !=3D NIL):

	sort_input_target =3D make_sort_input_target(...);
	...
	grouping_target =3D sort_input_target;
	...
	scanjoin_target =3D grouping_target;
	...
	apply_scanjoin_target_to_paths(...);

So apply_scanjoin_target_to_paths() effectively accounts for the additiona=
l
costs introduced by make_sort_input_target().

Note about the !activeWindows test: It occurs me now that no paths should =
be
added to UPPERREL_ORDERED relation if the query needs WindowAgg node, beca=
use
postgres_fdw currently cannot handle UPPERREL_WINDOW relation. Or rather i=
n
general, the FDW should not skip any stage just because it doesn't know ho=
w to
build the paths.

-- =

Antonin Houska
https://www.cybertec-postgresql.com