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: <5C6251A7.40204@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> <5C6251A7.40204@lab.ntt.co.jp>
Comments: In-reply-to Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
   message dated "Tue, 12 Feb 2019 13:55:03 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Tue, 12 Feb 2019 12:43:34 +0100
Message-ID: <26618.1549971814@localhost>
Precedence: bulk

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

> (2019/02/08 21:35), Etsuro Fujita wrote:
> > (2019/02/08 2:04), Antonin Houska wrote:
> >> * add_foreign_ordered_paths()
> >>
> >> Exprssion root->upper_targets[UPPERREL_FINAL] is used to access the
> >> target.
> >>
> >> I think create_ordered_paths() should actually set the target so that
> >> postgresGetForeignUpperPaths() can simply find it in
> >> output_rel->reltarget. This is how postgres_fdw already retrieves the
> >> target
> >> for grouped paths. Small patch is attached that makes this possible.
> >
> > Seems like a good idea. Thanks for the patch! Will review.
>=20
> Here are my review comments:
>=20
>  		root->upper_targets[UPPERREL_FINAL] =3D final_target;
> +		root->upper_targets[UPPERREL_ORDERED] =3D final_target;
>  		root->upper_targets[UPPERREL_WINDOW] =3D sort_input_target;
>  		root->upper_targets[UPPERREL_GROUP_AGG] =3D grouping_target;
>=20
> I think that's a good idea.  I think we'll soon need the PathTarget for
> UPPERREL_DISTINCT, so how about saving that as well like this?
>=20
>                 root->upper_targets[UPPERREL_FINAL] =3D final_target;
> +               root->upper_targets[UPPERREL_ORDERED] =3D final_target;
> +               root->upper_targets[UPPERREL_DISTINCT] =3D sort_input_tar=
get;
>                 root->upper_targets[UPPERREL_WINDOW] =3D sort_input_targe=
t;
>                 root->upper_targets[UPPERREL_GROUP_AGG] =3D grouping_targ=
et;

I see nothing wrong about this.

> Another is about this:
>=20
>  	/*
> +	 * Set reltarget so that it's consistent with the paths. Also it's more
> +	 * convenient for FDW to find the target here.
> +	 */
> +	ordered_rel->reltarget =3D target;
>=20
> I think this is correct if there are no SRFs in the targetlist, but other=
wise
> I'm not sure this is really correct because the relation's reltarget woul=
d not
> match the target of paths added to the relation after adjust_paths_for_sr=
fs().

I wouldn't expect problem here. create_grouping_paths() also sets the
reltarget although adjust_paths_for_srfs() can be alled on grouped_rel
afterwards.

--=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