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: <5C7928C7.50507@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> <7273.1551438041@localhost>
 <5C7928C7.50507@lab.ntt.co.jp>
Comments: In-reply-to Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
   message dated "Fri, 01 Mar 2019 21:42:47 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <26193.1551685577.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 04 Mar 2019 08:46:17 +0100
Message-ID: <26194.1551685577@localhost>
Precedence: bulk

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

> (2019/03/01 20:00), Antonin Houska wrote:
> > Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp>  wrote:

> > It's still unclear to me why add_foreign_ordered_paths() passes the in=
put
> > relation (input_rel) to estimate_path_cost_size(). If it passed the ou=
tput rel
> > (i.e. ordered_rel in this case) like add_foreign_grouping_paths() does=
, then
> > foreignrel->reltarget should contain the random() function. (What I se=
e now is
> > that create_ordered_paths() leaves ordered_rel->reltarget empty, but t=
hat's
> > another problem to be fixed.)
> >
> > Do you still see a reason to call estimate_path_cost_size() this way?
> =

> Yeah, the reason for that is because we can estimate the costs of sortin=
g for
> the final scan/join relation using the existing code as-is that estimate=
s the
> costs of sorting for base or join relations, except for tlist eval cost
> adjustment.  As you mentioned, we could pass ordered_rel to
> estimate_path_cost_size(), but if so, I think we would need to get input=
_rel
> (ie, the final scan/join relation) from ordered_rel within
> estimate_path_cost_size(), to use that code, which would not be great.

After some more thought I suggest that estimate_path_cost_size() is redesi=
gned
before your patch gets applied. The function is quite hard to read if - wi=
th
your patch applied - the "foreignrel" argument sometimes means the output
relation and other times the input one. I takes some effort to "switch
context" between these two perspectives when I read the code.

Perhaps both input and output relation should be passed to the function no=
w,
and maybe also UpperRelationKind of the output relation. And maybe it's ev=
en
worth moving some code into a subroutine that handles only the upper relat=
ion.

Originally the function could only handle base relations, then join relati=
on
was added, then one kind of upper relation (UPPERREL_GROUP_AGG) was added =
and
eventually the function will need to handle multiple kinds of upper
relation. It should be no surprise if such an amount of changes makes the
original signature insufficient.

-- =

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