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: <5C73C403.7080109@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> <29190.1550843669@localhost>
 <5C73C403.7080109@lab.ntt.co.jp>
Comments: In-reply-to Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
   message dated "Mon, 25 Feb 2019 19:31:31 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <7272.1551438041.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 01 Mar 2019 12:00:41 +0100
Message-ID: <7273.1551438041@localhost>
Precedence: bulk

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

> (2019/02/22 22:54), Antonin Houska wrote:
> > Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp>  wrote:
> >> On the other hand, the code bit added by
> >> 0001-postgres_fdw-Perform-UPPERREL_ORDERED-step-remotely-v3.patch han=
dles the
> >> case where a post-scan/join processing step other than grouping/aggre=
gation
> >> (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 =
different
> >> cases, hence both changes would be required.
> >
> > Do you mean this part?
> =

> No, I don't.  What I meant was this part:
> =

> +       /*
> +        * If this is an UPPERREL_ORDERED step performed on the final
> +        * scan/join relation, the costs obtained from the cache wouldn'=
t yet
> +        * contain the eval costs for the final scan/join target, which =
would
> +        * have been updated by apply_scanjoin_target_to_paths(); add th=
e eval
> +        * costs 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;
> +       }

> Yeah, but I think the code bit cited above is needed.  Let me explain us=
ing
> yet another example with grouping and ordering:
> =

>     SELECT a+b, random() FROM foreign_table GROUP BY a+b ORDER BY a+b;
> =

> For this query, the sort_input_target would be {a+b}, (to which the
> foreignrel->reltarget would have been set when called from
> estimate_path_cost_size() called from postgresGetForeignUpperPaths() wit=
h the
> UPPERREL_ORDERED stage), but the final target would be {a+b, random()}, =
so the
> sort_input_target isn't the same as the final target in that case; the c=
osts
> needs to be adjusted so that the costs include the ones of generating th=
e
> final target.  That's the reason why I added the code bit you cited.

I used gdb to help me understand, however the condition

	if (fpextra && !IS_UPPER_REL(foreignrel))

never evaluated to true with the query above. fpextra was only !=3DNULL wh=
en
estimate_path_cost_size() was called from add_foreign_ordered_paths(), but=
 at
the same time foreignrel->reloptkind was RELOPT_UPPER_REL.

It's still unclear to me why add_foreign_ordered_paths() passes the input
relation (input_rel) to estimate_path_cost_size(). If it passed the output=
 rel
(i.e. ordered_rel in this case) like add_foreign_grouping_paths() does, th=
en
foreignrel->reltarget should contain the random() function. (What I see no=
w is
that create_ordered_paths() leaves ordered_rel->reltarget empty, but that'=
s
another problem to be fixed.)

Do you still see a reason to call estimate_path_cost_size() this way?

> > Note about the !activeWindows test: It occurs me now that no paths sho=
uld be
> > added to UPPERREL_ORDERED relation if the query needs WindowAgg node, =
because
> > postgres_fdw currently cannot handle UPPERREL_WINDOW relation. Or rath=
er in
> > general, the FDW should not skip any stage just because it doesn't kno=
w how to
> > build the paths.
> =

> That's right.  In postgres_fdw, by the following code in
> postgresGetForeignUpperPaths(), we will give up on doing the UPPERREL_OR=
DERED
> step remotely, if the query has the UPPERREL_WINDOW (and/or UPPERREL_DIS=
TINCT)
> steps, because in that case we would have input_rel->fdw_private=3DNULL =
for a
> call to postgresGetForeignUpperPaths() with the UPPERREL_ORDERED stage:
> =

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

I understand now: if FDW did not handle the previous stage, then
input_rel->fdw_private is NULL.

-- =

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