On Mon, Mar 29, 2021 at 11:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Sun, Mar 28, 2021 at 1:30 AM Tom Lane <tgl@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.
Oh, that is exactly what I have proposed in:
https://commitfest.postgresql.org/32/2621/
> >> 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.
Sure, thanks again.
--
Amit Langote
EDB: http://www.enterprisedb.com
