Re: ModifyTable overheads in generic plans

Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> On Mon, Apr 5, 2021 at 1:43 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> OK. Do you want to pull out the bits of the patch that we can still
>> do without postponing BeginDirectModify?

> I ended up with the attached, whereby ExecInitResultRelation() is now
> performed for all relations before calling ExecInitNode() on the
> subplan. As mentioned, I moved other per-result-rel initializations
> into the same loop that does ExecInitResultRelation(), while moving
> code related to some initializations into initialize-on-first-access
> accessor functions for the concerned fields. I chose to do that for
> ri_WIthCheckOptionExprs, ri_projectReturning, and ri_projectNew.

I pushed the parts of this that I thought were safe and productive.

The business about moving the subplan tree initialization to after
calling FDWs' BeginForeignModify functions seems to me to be a
nonstarter. Existing FDWs are going to expect their scan initializations
to have been done first. I'm surprised that postgres_fdw seemed to
need only a one-line fix; it could have been far worse. The amount of
trouble that could cause is absolutely not worth it to remove one loop
over the result relations.

I also could not get excited about postponing initialization of RETURNING
or WITH CHECK OPTIONS expressions. I grant that that can be helpful
when those features are used, but I doubt that RETURNING is used that
heavily, and WITH CHECK OPTIONS is surely next door to nonexistent
in performance-critical queries. If the feature isn't used, the cost
of the existing code is about zero. So I couldn't see that it was worth
the amount of code thrashing and risk of new bugs involved. The bit you
noted about EXPLAIN missing a subplan is pretty scary in this connection;
I'm not at all sure that that's just cosmetic.

(Having said that, I'm wondering if there are bugs in these cases for
cross-partition updates that target a previously-not-used partition.
So we might have things to fix anyway.)

Anyway, looking at the test case you posted at the very top of this
thread, I was getting this with HEAD on Friday:

nparts TPS
0 12152
10 8672
100 2753
1000 314

and after the two patches I just pushed, it looks like:

0 12105
10 9928
100 5433
1000 938

So while there's certainly work left to do, that's not bad for
some low-hanging-fruit grabbing.

regards, tom lane