(2018/08/21 11:01), Kyotaro HORIGUCHI wrote:
At Tue, 14 Aug 2018 20:49:02 +0900, Etsuro Fujita<fujita.ets...@lab.ntt.co.jp> wrote
in<5b72c1ae.8010...@lab.ntt.co.jp>
(2018/08/09 22:04), Etsuro Fujita wrote:
(2018/08/08 17:30), Kyotaro HORIGUCHI wrote:
I spent more time looking at the patch. ISTM that the patch well
suppresses the effect of the tuple-descriptor expansion by making
changes to code in the planner and executor (and ruleutils.c), but I'm
still not sure that the patch is the right direction to go in, because
ISTM that expanding the tuple descriptor on the fly might be a wart.
The exapansion should be safe if the expanded descriptor has the
same defitions for base columns and all the extended coulumns are
junks. The junk columns should be ignored by unrelated nodes and
they are passed safely as far as ForeignModify passes tuples as
is from underlying ForeignScan to ForeignUpdate/Delete.
I'm not sure that would be really safe. Does that work well when
EvalPlanQual, for example?
You wrote:
Several places seems to be assuming that fdw_scan_tlist may be
used foreign scan on simple relation but I didn't find that
actually happens.
Yeah, currently, postgres_fdw and file_fdw don't use that list for
simple foreign table scans, but it could be used to improve the
efficiency for those scans, as explained in fdwhandler.sgml:
Another<structname>ForeignScan</structname> field that can be filled
by FDWs
is<structfield>fdw_scan_tlist</structfield>, which describes the
tuples returned by
the FDW for this plan node. For simple foreign table scans this can
be
set to<literal>NIL</literal>, implying that the returned tuples have
the
row type declared for the foreign table. A non-<symbol>NIL</symbol>
value must be a
target list (list of<structname>TargetEntry</structname>s) containing
Vars and/or
expressions representing the returned columns. This might be used,
for
example, to show that the FDW has omitted some columns that it noticed
won't be needed for the query. Also, if the FDW can compute
expressions
used by the query more cheaply than can be done locally, it could add
those expressions to<structfield>fdw_scan_tlist</structfield>. Note
that join
plans (created from paths made by
<function>GetForeignJoinPaths</function>) must
always supply<structfield>fdw_scan_tlist</structfield> to describe
the set of
columns they will return.
https://www.postgresql.org/docs/devel/static/fdw-planning.html
Hmm. Thanks for the pointer, it seems to need rewrite. However,
it doesn't seem to work for non-join foreign scans, since the
core igonres it and uses local table definition.
Really?
You wrote:
I'm not sure whether the following ponits are valid.
- If fdw_scan_tlist is used for simple relation scans, this would
break the case. (ExecInitForeignScan, set_foreignscan_references)
Some FDWs might already use that list for the improved efficiency for
simple foreign table scans as explained above, so we should avoid
breaking that.
I considered to use fdw_scan_tlist in that way but the core is
assuming that foreign scans with scanrelid> 0 uses the relation
descriptor.
Could you elaborate a bit more on this?
Do you have any example for that?
I don't know such an example, but in my understanding, the core allows
the FDW to do that.
If we take the Param-based approach suggested by Tom, I suspect there
would be no need to worry about at least those things, so I'll try to
update your patch as such, if there are no objections from you (or
anyone else).
PARAM_EXEC is single storage side channel that can work as far as
it is set and read while each tuple is handled. In this case
postgresExecForeignUpdate/Delete must be called before
postgresIterateForeignScan returns the next tuple. An apparent
failure case for this usage is the join-update case below.
https://www.postgresql.org/message-id/20180605.191032.256535589.horiguchi.kyot...@lab.ntt.co.jp
What I have in mind would be to 1) create a tlist that contains not only
Vars/PHVs but Params, for each join rel involving the target rel so we
ensure that the Params will propagate up through all join plan steps,
and 2) convert a join rel's tlist Params into Vars referencing the same
Params in the tlists for the outer/inner rels, by setrefs.c. I think
that would probably work well even for the case you mentioned above.
Maybe I'm missing something, though.
Sorry for the delay.
Best regards,
Etsuro Fujita
