From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Antonin Houska <ah(at)cybertec(dot)at> |
Cc: | Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: postgres_fdw: another oddity in costing aggregate pushdown paths |
Date: | 2019-05-08 03:42:51 |
Message-ID: | 5CD2503B.6010108@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
(2019/02/25 19:59), Etsuro Fujita wrote:
> (2019/02/22 23:10), Antonin Houska wrote:
>> Etsuro Fujita<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> As mentioned in the near thread, I think there is another oversight in
>>> the cost estimation for aggregate pushdown paths in postgres_fdw, IIUC.
>>> When costing an aggregate pushdown path using local statistics, we
>>> re-use the estimated costs of implementing the underlying scan/join
>>> relation, cached in the relation's PgFdwRelationInfo (ie,
>>> rel_startup_cost and rel_total_cost). Since these costs wouldn't yet
>>> contain the costs of evaluating the final scan/join target, as tlist
>>> replacement by apply_scanjoin_target_to_paths() is performed afterwards.
>>> So I think we need to adjust these costs so that the tlist eval costs
>>> are included, but ISTM that estimate_path_cost_size() forgot to do so.
>>> Attached is a patch for fixing this issue.
>>
>> I think the following comment in apply_scanjoin_target_to_paths() should
>> mention that FDWs rely on the new value of reltarget.
>>
>> /*
>> * Update the reltarget. This may not be strictly necessary in all cases,
>> * but it is at least necessary when create_append_path() gets called
>> * below directly or indirectly, since that function uses the reltarget as
>> * the pathtarget for the resulting path. It seems like a good idea to do
>> * it unconditionally.
>> */
>> rel->reltarget = llast_node(PathTarget, scanjoin_targets);
>
> Agreed. How about mentioning that like the attached? In addition, I
> added another assertion to estimate_path_cost_size() in that patch.
This doesn't get applied cleanly after commit 1d33858406. Here is a
rebased version of the patch. I also modified the comments a little
bit. If there are no objections from Antonin or anyone else, I'll
commit the patch.
Best regards,
Etsuro Fujita
Attachment | Content-Type | Size |
---|---|---|
another-fix-postgres-fdw-grouping-path-cost-v3.patch | text/x-patch | 3.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2019-05-08 03:53:06 | Re: We're leaking predicate locks in HEAD |
Previous Message | Thomas Munro | 2019-05-08 03:30:32 | Re: We're leaking predicate locks in HEAD |