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

Hello.

At Fri, 24 Aug 2018 21:45:35 +0900, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <5B7FFDEF(dot)6020302(at)lab(dot)ntt(dot)co(dot)jp>
> (2018/08/21 11:01), Kyotaro HORIGUCHI wrote:
> > 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 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?

Nothing. The reason was that core just doesn't know about the
extended portion. So only problematic case was
ExprEvalWholeRowVar, where explicit sanity check is
perfomed. But, I think it is a ugly wart as you said. So the
latest patch generates full fdw_scan_tlist.

> > 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?

No, I was wrong here. The core doesn't consider the case where
fdw_scan_tlist has attributes that is not a part of base relation
but it doesn't affect the description.

> >> 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?

After all I found that core uses fdw_scan_tlist if any and the
attached patch doen't modify the "affected" part. Sorry, it's
still hot here:p

> > 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.

As above, I agreed. Sorry for the bogosity.

> >> 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.kyotaro@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.

As I wrote above, the problem was not param id propagation but
the per-query storage for a parameter holded in econtext.

PARAM_EXEC is assumed to be used between outer and inner
relations of a nestloop or retrieval from sub-query retrieval as
commented in primnodes.h.

> PARAM_EXEC: The parameter is an internal executor parameter, used
> for passing values into and out of sub-queries or from
> nestloop joins to their inner scans.
> For historical reasons, such parameters are numbered from 0.
> These numbers are independent of PARAM_EXTERN numbers.

Anyway the odds are high that I'm missing far more than you.

> Sorry for the delay.

Nope. Thank you for the comment and I'm waiting for the patch.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center