From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Antonin Houska <ah(at)cybertec(dot)at> |
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-03-05 09:19:02 |
Message-ID: | 5C7E3F06.9000507@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
(2019/03/04 16:46), Antonin Houska wrote:
> Etsuro Fujita<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> (2019/03/01 20:00), Antonin Houska wrote:
>>> 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, then
>>> foreignrel->reltarget should contain the random() function. (What I see now 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?
>>
>> Yeah, the reason for that is because we can estimate the costs of sorting for
>> the final scan/join relation using the existing code as-is that estimates 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 redesigned
> before your patch gets applied. The function is quite hard to read if - with
> 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.
I have to admit that my proposal makes estimate_path_cost_size()
complicated to a certain extent, but I don't think it changes the
meaning of the foreignrel; even in my proposal, the foreignrel should be
considered as an output relation rather than an input relation, because
we create a foreign upper path with the foreignrel being the parent
RelOptInfo for the foreign upper path in add_foreign_ordered_paths() or
add_foreign_final_paths().
> Perhaps both input and output relation should be passed to the function now,
> and maybe also UpperRelationKind of the output relation.
My concern is: that would make it inconvenient to call that function.
> And maybe it's even
> worth moving some code into a subroutine that handles only the upper relation.
>
> Originally the function could only handle base relations, then join relation
> 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.
I 100% agree with you on that point.
Best regards,
Etsuro Fujita
From | Date | Subject | |
---|---|---|---|
Next Message | David Rowley | 2019-03-05 09:22:26 | Re: NOT IN subquery optimization |
Previous Message | David Steele | 2019-03-05 09:18:50 | Re: extension patch of CREATE OR REPLACE TRIGGER |