Re: Problem while updating a foreign table pointing to a partitioned table on foreign server

Fujita-san thank you for the comment.

At Tue, 14 Aug 2018 20:49:02 +0900, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <5B72C1AE(dot)8010408(at)lab(dot)ntt(dot)co(dot)jp>
> (2018/08/09 22:04), Etsuro Fujita wrote:
> > (2018/08/08 17:30), Kyotaro HORIGUCHI wrote:
> >> I choosed to expand tuple descriptor for junk column added to
> >> foreign relaions. We might be better to have new member in
> >> ForeignScan but I didn't so that we can backpatch it.
> >
> > I've not looked at the patch closely yet, but I'm not sure that it's a
> > good idea to expand the tuple descriptor of the target relation on the
> > fly so that it contains the remotetableoid as a non-system attribute
> > of
> > the target table. My concern is: is there not any risk in affecting
> > some
> > other part of the planner and/or the executor? (I was a bit surprised
> > that the patch passes the regression tests successfully.)

Yeah, me too.

> 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 non-Var nodes seems to me the same as PARAM_EXEC, which works
imperfectly for this purpose since tableoid must be in one-to-one
correspondence with a tuple but differently from joins the
correspondence is stired up by intermedate executor nodes in some
cases.

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.

> >> What the patch does are:
> >>
> >> - This abuses ForeignScan->fdw_scan_tlist to store the additional
> >> junk columns when foreign simple relation scan (that is, not a
> >> join).
> >
> > I think that this issue was introduced in 9.3, which added
> > postgres_fdw
> > in combination with support for writable foreign tables, but
> > fdw_scan_tlist was added to 9.5 as part of join-pushdown
> > infrastructure,
> > so my concern is that we might not be able to backpatch your patch to
> > 9.3 and 9.4.

Right. So I'm thinking that the older versions just get error for
the failure case instead of get it work anyhow. Or we might be
able to use tableoid in tuple header without emitting the local
oid to users but I haven't find the way to do that.

> Another concern about this:
>
> 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. This "tweak"
won't be needed if it worked.

> 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. Do you have any example for 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).

Feel free to do that since I couldn't find the way. I'll put more
consideration on using fdw_scan_tlist in the documented way.

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.kyotaro@lab.ntt.co.jp

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center