Re: making update/delete of inheritance trees scale better

On Sun, Mar 28, 2021 at 1:30 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > Attached updated version of the patch. I have forgotten to mention in
> > my recent posts on this thread one thing about 0001 that I had
> > mentioned upthread back in June. That it currently fails a test in
> > postgres_fdw's suite due to a bug of cross-partition updates that I
> > decided at the time to pursue in another thread:
> > https://www.postgresql.org/message-id/CA%2BHiwqE_UK1jTSNrjb8mpTdivzd3dum6mK--xqKq0Y9VmfwWQA%40mail.gmail.com
>
> Yeah, I ran into that too. I think we need not try to fix it in HEAD;
> we aren't likely to commit 0001 and 0002 separately. We need some fix
> for the back branches, but that would better be discussed in the other
> thread. (Note that the version of 0001 I attach below shows the actual
> output of the postgres_fdw test, including a failure from said bug.)

Okay, makes sense.

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

> (Note that 0002 below contains some cosmetic fixes, such as comments,
> that logically belong in 0001, but I didn't bother to tidy that up
> since I'm not seeing these as separate commits anyway.)
>
> The planner, however, still needs a lot of work. There's a serious
> functional problem, in that UPDATEs across partition trees having
> more than one foreign table fail with
>
> ERROR: junk column "wholerow" of child relation 5 conflicts with parent junk column with same name
>
> (cf. multiupdate.sql test case attached).

Oops, thanks for noticing that.

> I think we could get around
> that by requiring "wholerow" junk attrs to have vartype RECORDOID instead
> of the particular table's rowtype, which might also remove the need for
> some of the vartype translation hacking in 0002. But I haven't tried yet.

Sounds like that might work.

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

--
Amit Langote
EDB: http://www.enterprisedb.com