Re: Problems with plan estimates in postgres_fdw

From: Antonin Houska <ah(at)cybertec(dot)at>
To: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Problems with plan estimates in postgres_fdw
Date: 2019-02-12 11:43:34
Message-ID: 26618.1549971814@localhost
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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.
>
> Here are my review comments:
>
> root->upper_targets[UPPERREL_FINAL] = final_target;
> + root->upper_targets[UPPERREL_ORDERED] = final_target;
> root->upper_targets[UPPERREL_WINDOW] = sort_input_target;
> root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target;
>
> 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?
>
> root->upper_targets[UPPERREL_FINAL] = final_target;
> + root->upper_targets[UPPERREL_ORDERED] = final_target;
> + root->upper_targets[UPPERREL_DISTINCT] = sort_input_target;
> root->upper_targets[UPPERREL_WINDOW] = sort_input_target;
> root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target;

I see nothing wrong about this.

> Another is about this:
>
> /*
> + * 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 = target;
>
> I think this is correct if there are no SRFs in the targetlist, but otherwise
> I'm not sure this is really correct because the relation's reltarget would not
> match the target of paths added to the relation after adjust_paths_for_srfs().

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.

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexey Kondratov 2019-02-12 12:22:04 Re: Logical replication and restore from pg_basebackup
Previous Message Kyotaro HORIGUCHI 2019-02-12 11:36:28 Re: Protect syscache from bloating with negative cache entries