Amit Langote <amitlangot...@gmail.com> writes:
> On Sun, Mar 28, 2021 at 1:30 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
>> I wanted to give a data dump of where I am. I've reviewed and
>> nontrivially modified 0001 and the executor parts of 0002, and
>> I'm fairly happy with the state of that much of the code now.
> Thanks a lot for that work. I have looked at the changes and I agree
> that updateColnosLists + ExecBuildUpdateProjection() looks much better
> than updateTargetLists in the original patch. Looking at
> ExecBuildUpdateProjection(), I take back my comment upthread regarding
> the performance characteristics of your approach, that the prepared
> statements would suffer from having to build the update-new-tuple
> projection(s) from scratch on every execution.
Yeah, I don't see any reason why the custom projection-build code
would be any slower than the regular path. Related to this, though,
I was wondering whether we could get a useful win by having
nodeModifyTable.c be lazier about doing the per-target-table
initialization steps. I think we have to open and lock all the
tables at start for semantic reasons, so maybe that swamps everything
else. But we could avoid purely-internal setup steps, such as
building the slots and projection expressions, until the first time
a particular target is actually updated into. This'd help if we've
failed to prune a lot of partitions that the update/delete won't
actually affect.
>> More abstractly, I really dislike the "fake variable" design, primarily
>> the aspect that you made the fake variables look like real columns of
>> the parent table with attnums just beyond the last real one. I think
>> this is just a recipe for obscuring bugs, since it means you have to
>> lobotomize a lot of bad-attnum error checks. The alternative I'm
>> considering is to invent a separate RTE that holds all the junk columns.
>> Haven't tried that yet either.
> Hmm, I did expect to hear a strong critique of that piece of code. I
> look forward to reviewing your alternative implementation.
I got one version working over the weekend, but I didn't like the amount
of churn it forced in postgres_fdw (and, presumably, other FDWs). Gimme
a day or so to try something else.
regards, tom lane
