Re: Problems with plan estimates in postgres_fdw

(2019/02/25 18:40), Etsuro Fujita wrote:
> (2019/02/23 0:21), Antonin Houska wrote:
>> Etsuro Fujita<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:

>> What about an ORDER BY expression that contains multiple Var nodes? For
>> example
>>
>> SELECT * FROM foo ORDER BY x + y;

> Actually, add_paths_with_pathkeys_for_rel() generates such pre-sorted
> paths for the base relation, as shown in the below example using HEAD
> without the patchset proposed in this thread:
>
> postgres=# explain verbose select a+b from ft1 order by a+b;
> QUERY PLAN
> --------------------------------------------------------------------------
> Foreign Scan on public.ft1 (cost=100.00..200.32 rows=2560 width=4)
> Output: (a + b)
> Remote SQL: SELECT a, b FROM public.t1 ORDER BY (a + b) ASC NULLS LAST
> (3 rows)
>
> I think it is OK for that function to generate such paths, as tlists for
> such paths would be adjusted in apply_scanjoin_target_to_paths(), by
> doing create_projection_path() to them.
>
> Conversely, it appears that add_foreign_ordered_paths() added by the
> patchset would generate such pre-sorted paths *redundantly* when the
> input_rel is the final scan/join relation. Will look into that.

As mentioned above, I noticed that we generated a properly-sorted path
redundantly in add_foreign_ordered_paths(), for the case where 1) the
input_rel is the final scan/join relation and 2) the input_rel already
has a properly-sorted path in its pathlist that was created by
add_paths_with_pathkeys_for_rel(). So, I modified
add_foreign_ordered_paths() to skip creating a path in that case.

(2019/02/25 19:31), Etsuro Fujita wrote:
> + /*
> + * 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 the eval
> + * costs now.
> + */
> + if (fpextra && !IS_UPPER_REL(foreignrel))
> + {
> + /* The costs should have been obtained from the cache. */
> + Assert(fpinfo->rel_startup_cost >= 0 &&
> + fpinfo->rel_total_cost >= 0);
> +
> + startup_cost += foreignrel->reltarget->cost.startup;
> + run_cost += foreignrel->reltarget->cost.per_tuple * rows;
> + }

> but as I said in the nearby thread, this part might be completely
> redundant. Will re-consider about this.

I thought that if it was true that in add_foreign_ordered_paths() we
didn't need to consider pushing down the final sort to the remote in the
case where the input_rel to that function is the final scan/join
relation, the above code could be entirely removed from
estimate_path_cost_size(), but I noticed that that is wrong; as we do
not always have a properly-sorted path in the input_rel's pathlist
already. So, I think we need to keep the above code so that we we can
consider the final sort pushdown for the final scan/join relation in
add_foreign_ordered_paths(). Sorry for the confusion. I moved the
above code to the place we get cached costs, which I hope makes
estimate_path_cost_size() a bit more readable.

Other changes:

* I modified add_foreign_final_paths() to skip creating a path if
possible, in a similar way to add_foreign_ordered_paths().
* I fixed the issue pointed out by Jeff [1].
* I added more comments.

Best regards,
Etsuro Fujita

[1]
https://www.postgresql.org/message-id/CAMkU%3D1z1Ez7fb_P_0Bc1040npE5fCOnu0M1DFyOzCp%3De%3DrBJCw%40mail.gmail.com