ModifyTable overheads in generic plans

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
47 messages Options
123
Reply | Threaded
Open this post in threaded view
|

ModifyTable overheads in generic plans

Amit Langote
Hi,

I would like to discuss a refactoring patch that builds on top of the
patches at [1] to address $subject.  To get an idea for what
eliminating these overheads looks like, take a look at the following
benchmarking results.

Note 1: I've forced the use of generic plan by setting plan_cache_mode
to 'force_generic_plan'

Note 2: The individual TPS figures as measured are quite noisy, though
I just want to show the rough trend with increasing number of
partitions.

pgbench -i -s 10 --partitions={0, 10, 100, 1000}
pgbench -T120 -f test.sql -M prepared

test.sql:
\set aid random(1, 1000000)
update pgbench_accounts set abalance = abalance + 1 where aid = :aid;

Without any of the patches:

0       tps = 13045.485121 (excluding connections establishing)
10      tps = 9358.157433 (excluding connections establishing)
100     tps = 1878.274500 (excluding connections establishing)
1000    tps = 84.684695 (excluding connections establishing)

The slowdown as the partition count increases can be explained by the
fact that UPDATE and DELETE can't currently use runtime partition
pruning.  So, even if any given transaction is only updating a single
tuple in a single partition, the plans for *all* partitions are being
initialized and also the ResultRelInfos.  That is, a lot of useless
work being done in InitPlan() and ExecInitModifyTable().

With the patches at [1] (latest 0001+0002 posted there), whereby the
generic plan for UPDATE can now perform runtime pruning, numbers can
be seen to improve, slightly:

0       tps = 12743.487196 (excluding connections establishing)
10      tps = 12644.240748 (excluding connections establishing)
100     tps = 4158.123345 (excluding connections establishing)
1000    tps = 391.248067 (excluding connections establishing)

So even though runtime pruning enabled by those patches ensures that
the useless plans are left untouched by the executor, the
ResultRelInfos are still being made assuming *all* result relations
will be processed.  With the attached patches (0001+0002+0003) that I
want to discuss here in this thread, numbers are further improved:

0       tps = 13419.283168 (excluding connections establishing)
10      tps = 12588.016095 (excluding connections establishing)
100     tps = 8560.824225 (excluding connections establishing)
1000    tps = 1926.553901 (excluding connections establishing)

0001 and 0002 are preparatory patches.  0003 teaches nodeModifyTable.c
to make the ResultRelInfo for a given result relation lazily, that is,
when the plan producing tuples to be updated/deleted actually produces
one that belongs to that relation.  So, if a transaction only updates
one tuple, then only one ResultRelInfo would be made.  For larger
partition counts, that saves significant amount of work.

However, there's one new loop in ExecInitModifyTable() added by the
patches at [1] that loops over all partitions, which I haven't been
able to eliminate so far and I'm seeing it cause significant
bottleneck at higher partition counts.  The loop is meant to create a
hash table that maps result relation OIDs to their offsets in the
PlannedStmt.resultRelations list.  We need this mapping, because the
ResultRelInfos are accessed from the query-global array using that
offset.  One approach that was mentioned by David Rowley at [1] to not
have do this mapping is to make the result relation's scan node's
targetlist emit the relation's RT index or ordinal position to begin
with, instead of the table OID, but I haven't figured out a way to do
that.

Having taken care of the ModifyTable overheads (except the one
mentioned in the last paragraph), a few more bottlenecks are seen to
pop up at higher partition counts.  Basically, they result from doing
some pre-execution actions on relations contained in the plan by
traversing the flat range table in whole.

1. AcquireExecutorLocks(): locks *all* partitions before executing the
plan tree but runtime pruning allows to skip scanning all but one

2. ExecCheckRTPerms(): checks permissions of *all* partitions before
executing the plan tree, but maybe it's okay to check only the ones
that will be accessed

Problem 1 has been discussed before and David Rowley even developed a
patch that was discussed at [2].  The approach taken in the patch was
to delay locking of the partitions contained in a generic plan that
are potentially runtime pruneable, although as also described in the
linked thread, that approach has a race condition whereby a concurrent
session may invalidate the generic plan by altering a partition in the
window between when AcquireExecutorLocks() runs on the plan and the
plan is executed.

Another solution suggested to me by Robert Haas in an off-list
discussion is to teach AcquireExecutorLocks() or the nearby code to
perform EXTERN parameter based pruning before passing the plan tree to
the executor and lock partitions that survive that pruning.  It's
perhaps doable if we refactor the ExecFindInitialMatchingSubPlans() to
not require a full-blown execution context.  Or maybe we could do
something more invasive by rewriting AcquireExecutorLocks() to walk
the plan tree instead of the flat range table, looking for scan nodes
and nodes that support runtime pruning to lock the appropriate
relations.

Regarding problem 2, I wonder if we shouldn't simply move the
permission check to ExecGetRangeTableRelation(), which will be
performed the first time a given relation is accessed during
execution.

Anyway, applying David's aforementioned patch gives the following numbers:

0       tps = 12325.890487 (excluding connections establishing)
10      tps = 12146.420443 (excluding connections establishing)
100     tps = 12807.850709 (excluding connections establishing)
1000    tps = 7578.652893 (excluding connections establishing)

which suggests that it might be worthwhile try to find a solution for this.

Finally, there's one more place that shows up in perf profiles at
higher partition counts and that is LockReleaseAll().  David,
Tsunakawa-san had worked on a patch [3], which still applies and can
be shown to be quite beneficial when generic plans are involved.  I
couldn't get it to show major improvement over the above numbers in
this case (for UPDATE that is), but maybe that's because the loop in
ExecInitModifyTable() mentioned above is still in the way.

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

[1] https://commitfest.postgresql.org/28/2575/
[2] https://www.postgresql.org/message-id/flat/CAKJS1f_kfRQ3ZpjQyHC7%3DPK9vrhxiHBQFZ%2Bhc0JCwwnRKkF3hg%40mail.gmail.com
[3] https://www.postgresql.org/message-id/flat/CAKJS1f-7T9F1xLw5PqgOApcV6YX3WYC4XJHHCpxh8hzcZsA-xA%40mail.gmail.com#c57f2f592484bca76310f4c551d4de15

v1-0002-Do-not-set-rootResultRelIndex-unnecessarily.patch (1K) Download Attachment
v1-0001-Revise-how-some-FDW-executor-APIs-obtain-ResultRe.patch (14K) Download Attachment
v1-0003-Delay-initializing-UPDATE-DELETE-ResultRelInfos.patch (91K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
On Fri, Jun 26, 2020 at 9:36 PM Amit Langote <[hidden email]> wrote:
> I would like to discuss a refactoring patch that builds on top of the
> patches at [1] to address $subject.

I've added this to the next CF: https://commitfest.postgresql.org/28/2621/

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


Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

David Rowley
In reply to this post by Amit Langote
On Sat, 27 Jun 2020 at 00:36, Amit Langote <[hidden email]> wrote:
> 2. ExecCheckRTPerms(): checks permissions of *all* partitions before
> executing the plan tree, but maybe it's okay to check only the ones
> that will be accessed

I don't think it needs to be quite as complex as that.
expand_single_inheritance_child will set the
RangeTblEntry.requiredPerms to 0, so we never need to check
permissions on a partition.  The overhead of permission checking when
there are many partitions is just down to the fact that
ExecCheckRTPerms() loops over the entire rangetable and calls
ExecCheckRTEPerms for each one.  ExecCheckRTEPerms() does have very
little work to do when requiredPerms is 0, but the loop itself and the
function call overhead show up when you remove the other bottlenecks.

I have a patch somewhere that just had the planner add the RTindexes
with a non-zero requiredPerms and set that in the plan so that
ExecCheckRTPerms could just look at the ones that actually needed
something checked.   There's a slight disadvantage there that for
queries to non-partitioned tables that we need to build a Bitmapset
that has all items from the rangetable.  That's likely a small
overhead, but not free, so perhaps there is a better way.

David


Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
On Mon, Jun 29, 2020 at 10:39 AM David Rowley <[hidden email]> wrote:

>
> On Sat, 27 Jun 2020 at 00:36, Amit Langote <[hidden email]> wrote:
> > 2. ExecCheckRTPerms(): checks permissions of *all* partitions before
> > executing the plan tree, but maybe it's okay to check only the ones
> > that will be accessed
>
> I don't think it needs to be quite as complex as that.
> expand_single_inheritance_child will set the
> RangeTblEntry.requiredPerms to 0, so we never need to check
> permissions on a partition.  The overhead of permission checking when
> there are many partitions is just down to the fact that
> ExecCheckRTPerms() loops over the entire rangetable and calls
> ExecCheckRTEPerms for each one.  ExecCheckRTEPerms() does have very
> little work to do when requiredPerms is 0, but the loop itself and the
> function call overhead show up when you remove the other bottlenecks.

I had forgotten that we set requiredPerms to 0 for the inheritance child tables.

> I have a patch somewhere that just had the planner add the RTindexes
> with a non-zero requiredPerms and set that in the plan so that
> ExecCheckRTPerms could just look at the ones that actually needed
> something checked.   There's a slight disadvantage there that for
> queries to non-partitioned tables that we need to build a Bitmapset
> that has all items from the rangetable.  That's likely a small
> overhead, but not free, so perhaps there is a better way.

I can't think of anything for this that doesn't involve having one
more list of RTEs or bitmapset of RT indexes in PlannedStmt.



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


Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
In reply to this post by Amit Langote
On Fri, Jun 26, 2020 at 9:36 PM Amit Langote <[hidden email]> wrote:
> I would like to discuss a refactoring patch that builds on top of the
> patches at [1] to address $subject.

I forgot to update a place in postgres_fdw causing one of its tests to crash.

Fixed in the attached updated version.

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

v2-0003-Delay-initializing-UPDATE-DELETE-ResultRelInfos.patch (91K) Download Attachment
v2-0002-Do-not-set-rootResultRelIndex-unnecessarily.patch (1K) Download Attachment
v2-0001-Revise-how-some-FDW-executor-APIs-obtain-ResultRe.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Daniel Gustafsson
> On 1 Jul 2020, at 08:30, Amit Langote <[hidden email]> wrote:
>
> On Fri, Jun 26, 2020 at 9:36 PM Amit Langote <[hidden email]> wrote:
>> I would like to discuss a refactoring patch that builds on top of the
>> patches at [1] to address $subject.
>
> I forgot to update a place in postgres_fdw causing one of its tests to crash.
>
> Fixed in the attached updated version.

The attached 0003 fails to apply to current HEAD, please submit another rebased
version.  Marking the entry as Waiting on Author in the meantime.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
Hi Daniel,

On Wed, Jul 1, 2020 at 6:50 PM Daniel Gustafsson <[hidden email]> wrote:

> > On 1 Jul 2020, at 08:30, Amit Langote <[hidden email]> wrote:
> >
> > On Fri, Jun 26, 2020 at 9:36 PM Amit Langote <[hidden email]> wrote:
> >> I would like to discuss a refactoring patch that builds on top of the
> >> patches at [1] to address $subject.
> >
> > I forgot to update a place in postgres_fdw causing one of its tests to crash.
> >
> > Fixed in the attached updated version.
>
> The attached 0003 fails to apply to current HEAD, please submit another rebased
> version.  Marking the entry as Waiting on Author in the meantime.
Thank you for the heads up.

Actually, as I noted in the first email, the patches here are to be
applied on top of patches of another thread that I chose not to post
here.  But I can see how that is inconvenient both for the CF bot and
other humans, so I'm attaching all of the patches.

Another thing I could do is decouple the patches to discuss here from
the patches of the other thread, which should be possible and might be
good to avoid back and forth between the two threads.

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

v2-0005-Delay-initializing-UPDATE-DELETE-ResultRelInfos.patch (91K) Download Attachment
v2-0003-Revise-how-some-FDW-executor-APIs-obtain-ResultRe.patch (14K) Download Attachment
v2-0004-Do-not-set-rootResultRelIndex-unnecessarily.patch (1K) Download Attachment
v2-0001-Overhaul-UPDATE-s-targetlist-processing.patch (101K) Download Attachment
v2-0002-Overhaul-how-inherited-update-delete-are-handled.patch (254K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Daniel Gustafsson
> On 1 Jul 2020, at 15:38, Amit Langote <[hidden email]> wrote:

> Another thing I could do is decouple the patches to discuss here from
> the patches of the other thread, which should be possible and might be
> good to avoid back and forth between the two threads.

It sounds like it would make it easier for reviewers, so if it's possible with
a reasonable effort it might be worth it.  I've moved this entry to the next CF
for now.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Robert Haas
In reply to this post by Amit Langote
On Fri, Jun 26, 2020 at 8:36 AM Amit Langote <[hidden email]> wrote:
> 0001 and 0002 are preparatory patches.

I read through these patches a bit but it's really unclear what the
point of them is. I think they need better commit messages, or better
comments, or both.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
On Sat, Aug 1, 2020 at 4:46 AM Robert Haas <[hidden email]> wrote:
> On Fri, Jun 26, 2020 at 8:36 AM Amit Langote <[hidden email]> wrote:
> > 0001 and 0002 are preparatory patches.
>
> I read through these patches a bit but it's really unclear what the
> point of them is. I think they need better commit messages, or better
> comments, or both.

Thanks for taking a look.  Sorry about the lack of good commentary,
which I have tried to address in the attached updated version. I
extracted one more part as preparatory from the earlier 0003 patch, so
there are 4 patches now.

Also as discussed with Daniel, I have changed the patches so that they
can be applied on plain HEAD instead of having to first apply the
patches at [1].  Without runtime pruning for UPDATE/DELETE proposed in
[1], optimizing ResultRelInfo creation by itself does not improve the
performance/scalability by that much, but the benefit of lazily
creating ResultRelInfos seems clear so I think maybe it's okay to
pursue this independently.

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

[1] https://www.postgresql.org/message-id/CA%2BHiwqHpHdqdDn48yCEhynnniahH78rwcrv1rEX65-fsZGBOLQ%40mail.gmail.com

v3-0003-Revise-child-to-root-tuple-conversion-map-managem.patch (28K) Download Attachment
v3-0004-Initialize-result-relation-information-lazily.patch (80K) Download Attachment
v3-0001-Revise-how-FDWs-obtain-result-relation-informatio.patch (16K) Download Attachment
v3-0002-Don-t-make-root-ResultRelInfo-for-insert-queries.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
On Tue, Aug 4, 2020 at 3:15 PM Amit Langote <[hidden email]> wrote:

> On Sat, Aug 1, 2020 at 4:46 AM Robert Haas <[hidden email]> wrote:
> > On Fri, Jun 26, 2020 at 8:36 AM Amit Langote <[hidden email]> wrote:
> > > 0001 and 0002 are preparatory patches.
> >
> > I read through these patches a bit but it's really unclear what the
> > point of them is. I think they need better commit messages, or better
> > comments, or both.
>
> Thanks for taking a look.  Sorry about the lack of good commentary,
> which I have tried to address in the attached updated version. I
> extracted one more part as preparatory from the earlier 0003 patch, so
> there are 4 patches now.
>
> Also as discussed with Daniel, I have changed the patches so that they
> can be applied on plain HEAD instead of having to first apply the
> patches at [1].  Without runtime pruning for UPDATE/DELETE proposed in
> [1], optimizing ResultRelInfo creation by itself does not improve the
> performance/scalability by that much, but the benefit of lazily
> creating ResultRelInfos seems clear so I think maybe it's okay to
> pursue this independently.
Per cfbot's automatic patch tester, there were some issues in the 0004 patch:

nodeModifyTable.c: In function ‘ExecModifyTable’:
1529nodeModifyTable.c:2484:24: error: ‘junkfilter’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
1530              junkfilter->jf_junkAttNo,
1531                        ^
1532nodeModifyTable.c:2309:14: note: ‘junkfilter’ was declared here
1533  JunkFilter *junkfilter;
1534              ^
1535cc1: all warnings being treated as errors
1536<builtin>: recipe for target 'nodeModifyTable.o' failed
1537make[3]: *** [nodeModifyTable.o] Error 1

Fixed in the attached updated version

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

v4-0003-Revise-child-to-root-tuple-conversion-map-managem.patch (28K) Download Attachment
v4-0002-Don-t-make-root-ResultRelInfo-for-insert-queries.patch (10K) Download Attachment
v4-0004-Initialize-result-relation-information-lazily.patch (80K) Download Attachment
v4-0001-Revise-how-FDWs-obtain-result-relation-informatio.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
Hello,

On Fri, Aug 7, 2020 at 9:26 PM Amit Langote <[hidden email]> wrote:

> On Tue, Aug 4, 2020 at 3:15 PM Amit Langote <[hidden email]> wrote:
> > On Sat, Aug 1, 2020 at 4:46 AM Robert Haas <[hidden email]> wrote:
> > > On Fri, Jun 26, 2020 at 8:36 AM Amit Langote <[hidden email]> wrote:
> > > > 0001 and 0002 are preparatory patches.
> > >
> > > I read through these patches a bit but it's really unclear what the
> > > point of them is. I think they need better commit messages, or better
> > > comments, or both.
> >
> > Thanks for taking a look.  Sorry about the lack of good commentary,
> > which I have tried to address in the attached updated version. I
> > extracted one more part as preparatory from the earlier 0003 patch, so
> > there are 4 patches now.
> >
> > Also as discussed with Daniel, I have changed the patches so that they
> > can be applied on plain HEAD instead of having to first apply the
> > patches at [1].  Without runtime pruning for UPDATE/DELETE proposed in
> > [1], optimizing ResultRelInfo creation by itself does not improve the
> > performance/scalability by that much, but the benefit of lazily
> > creating ResultRelInfos seems clear so I think maybe it's okay to
> > pursue this independently.
>
> Per cfbot's automatic patch tester, there were some issues in the 0004 patch:
>
> nodeModifyTable.c: In function ‘ExecModifyTable’:
> 1529nodeModifyTable.c:2484:24: error: ‘junkfilter’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
> 1530              junkfilter->jf_junkAttNo,
> 1531                        ^
> 1532nodeModifyTable.c:2309:14: note: ‘junkfilter’ was declared here
> 1533  JunkFilter *junkfilter;
> 1534              ^
> 1535cc1: all warnings being treated as errors
> 1536<builtin>: recipe for target 'nodeModifyTable.o' failed
> 1537make[3]: *** [nodeModifyTable.o] Error 1
>
> Fixed in the attached updated version
Needed a rebase due to f481d28232.  Attached updated patches.

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

v5-0004-Initialize-result-relation-information-lazily.patch (80K) Download Attachment
v5-0001-Revise-how-FDWs-obtain-result-relation-informatio.patch (16K) Download Attachment
v5-0002-Don-t-make-root-ResultRelInfo-for-insert-queries.patch (10K) Download Attachment
v5-0003-Revise-child-to-root-tuple-conversion-map-managem.patch (28K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
Attached updated patches based on recent the discussion at:

* Re: partition routing layering in nodeModifyTable.c *
https://www.postgresql.org/message-id/CA%2BHiwqHpmMjenQqNpMHrhg3DRhqqQfby2RCT1HWVwMin3_5vMA%40mail.gmail.com

0001 adjusts how ForeignScanState.resultRelInfo is initialized for use
by direct modify operations.

0002 refactors ResultRelInfo initialization do be don lazily on first use

I call these v6, because the last version posted on this thread was
v5, even though it went through a couple of iterations on the above
thread. Sorry about the confusion.

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

v6-0001-Call-BeginDirectModify-from-ExecInitModifyTable.patch (5K) Download Attachment
v6-0002-Initialize-result-relation-information-lazily.patch (89K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Heikki Linnakangas
On 30/10/2020 08:13, Amit Langote wrote:

> /*
>  * Perform WITH CHECK OPTIONS check, if any.
>  */
> static void
> ExecProcessWithCheckOptions(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
> TupleTableSlot *slot, WCOKind wco_kind)
> {
> ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
> EState *estate = mtstate->ps.state;
>
> if (node->withCheckOptionLists == NIL)
> return;
>
> /* Initialize expression state if not already done. */
> if (resultRelInfo->ri_WithCheckOptions == NIL)
> {
> int whichrel = resultRelInfo - mtstate->resultRelInfo;
> List   *wcoList;
> List   *wcoExprs = NIL;
> ListCell   *ll;
>
> Assert(whichrel >= 0 && whichrel < mtstate->mt_nplans);
> wcoList = (List *) list_nth(node->withCheckOptionLists, whichrel);
> foreach(ll, wcoList)
> {
> WithCheckOption *wco = (WithCheckOption *) lfirst(ll);
> ExprState  *wcoExpr = ExecInitQual((List *) wco->qual,
>   &mtstate->ps);
>
> wcoExprs = lappend(wcoExprs, wcoExpr);
> }
>
> resultRelInfo->ri_WithCheckOptions = wcoList;
> resultRelInfo->ri_WithCheckOptionExprs = wcoExprs;
> }
>
> /*
> * ExecWithCheckOptions() will skip any WCOs which are not of the kind
> * we are looking for at this point.
> */
> ExecWithCheckOptions(wco_kind, resultRelInfo, slot, estate);
> }

Can we do this initialization in ExecGetResultRelation()? That would
seem much more straightforward. Is there any advantage to delaying it
here? And same thing with the junk filter and the RETURNING list.

(/me reads patch further) I presume that's what you referred to in the
commit message:

>     Also, extend this lazy initialization approach to some of the
>     individual fields of ResultRelInfo such that even for the result
>     relations that are initialized, those fields are only initialized on
>     first access.  While no performance improvement is to be expected
>     there, it can lead to a simpler initialization logic of the
>     ResultRelInfo itself, because the conditions for whether a given
>     field is needed or not tends to look confusing.  One side-effect
>     of this is that any "SubPlans" referenced in the expressions of
>     those fields are also lazily initialized and hence changes the
>     output of EXPLAIN (without ANALYZE) in some regression tests.


I'm now curious what the initialization logic would look like, if we
initialized those fields in ExecGetResultRelation(). At a quick glance
on the conditions on when those initializations are done in the patch
now, it would seem pretty straightforward. If the target list contains
any junk columns, initialize junk filter, and if
ModifyTable->returningLists is set, initialize RETURNING list. Maybe I'm
missing something.

- Heikki


Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
On Mon, Nov 2, 2020 at 10:19 PM Heikki Linnakangas <[hidden email]> wrote:

> On 30/10/2020 08:13, Amit Langote wrote:
> > /*
> >  * Perform WITH CHECK OPTIONS check, if any.
> >  */
> > static void
> > ExecProcessWithCheckOptions(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
> >                                                       TupleTableSlot *slot, WCOKind wco_kind)
> > {
> >       ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
> >       EState *estate = mtstate->ps.state;
> >
> >       if (node->withCheckOptionLists == NIL)
> >               return;
> >
> >       /* Initialize expression state if not already done. */
> >       if (resultRelInfo->ri_WithCheckOptions == NIL)
> >       {
> >               int             whichrel = resultRelInfo - mtstate->resultRelInfo;
> >               List   *wcoList;
> >               List   *wcoExprs = NIL;
> >               ListCell   *ll;
> >
> >               Assert(whichrel >= 0 && whichrel < mtstate->mt_nplans);
> >               wcoList = (List *) list_nth(node->withCheckOptionLists, whichrel);
> >               foreach(ll, wcoList)
> >               {
> >                       WithCheckOption *wco = (WithCheckOption *) lfirst(ll);
> >                       ExprState  *wcoExpr = ExecInitQual((List *) wco->qual,
> >                                                                                          &mtstate->ps);
> >
> >                       wcoExprs = lappend(wcoExprs, wcoExpr);
> >               }
> >
> >               resultRelInfo->ri_WithCheckOptions = wcoList;
> >               resultRelInfo->ri_WithCheckOptionExprs = wcoExprs;
> >       }
> >
> >       /*
> >        * ExecWithCheckOptions() will skip any WCOs which are not of the kind
> >        * we are looking for at this point.
> >        */
> >       ExecWithCheckOptions(wco_kind, resultRelInfo, slot, estate);
> > }
>
> Can we do this initialization in ExecGetResultRelation()? That would
> seem much more straightforward. Is there any advantage to delaying it
> here? And same thing with the junk filter and the RETURNING list.
>
> (/me reads patch further) I presume that's what you referred to in the
> commit message:
>
> >     Also, extend this lazy initialization approach to some of the
> >     individual fields of ResultRelInfo such that even for the result
> >     relations that are initialized, those fields are only initialized on
> >     first access.  While no performance improvement is to be expected
> >     there, it can lead to a simpler initialization logic of the
> >     ResultRelInfo itself, because the conditions for whether a given
> >     field is needed or not tends to look confusing.  One side-effect
> >     of this is that any "SubPlans" referenced in the expressions of
> >     those fields are also lazily initialized and hence changes the
> >     output of EXPLAIN (without ANALYZE) in some regression tests.
>
>
> I'm now curious what the initialization logic would look like, if we
> initialized those fields in ExecGetResultRelation(). At a quick glance
> on the conditions on when those initializations are done in the patch
> now, it would seem pretty straightforward. If the target list contains
> any junk columns, initialize junk filter, and if
> ModifyTable->returningLists is set, initialize RETURNING list. Maybe I'm
> missing something.

Yeah, it's not that complicated to initialize those things in
ExecGetResultRelation().  In fact, ExecGetResultRelation() (or its
subroutine ExecBuildResultRelation()) housed those initializations in
the earlier versions of this patch, but I changed that after our
discussion about being lazy about initializing as much stuff as we
can.  Maybe I should revert that?

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


Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
On Mon, Nov 2, 2020 at 10:53 PM Amit Langote <[hidden email]> wrote:

> On Mon, Nov 2, 2020 at 10:19 PM Heikki Linnakangas <[hidden email]> wrote:
> > (/me reads patch further) I presume that's what you referred to in the
> > commit message:
> >
> > >     Also, extend this lazy initialization approach to some of the
> > >     individual fields of ResultRelInfo such that even for the result
> > >     relations that are initialized, those fields are only initialized on
> > >     first access.  While no performance improvement is to be expected
> > >     there, it can lead to a simpler initialization logic of the
> > >     ResultRelInfo itself, because the conditions for whether a given
> > >     field is needed or not tends to look confusing.  One side-effect
> > >     of this is that any "SubPlans" referenced in the expressions of
> > >     those fields are also lazily initialized and hence changes the
> > >     output of EXPLAIN (without ANALYZE) in some regression tests.
> >
> >
> > I'm now curious what the initialization logic would look like, if we
> > initialized those fields in ExecGetResultRelation(). At a quick glance
> > on the conditions on when those initializations are done in the patch
> > now, it would seem pretty straightforward. If the target list contains
> > any junk columns, initialize junk filter, and if
> > ModifyTable->returningLists is set, initialize RETURNING list. Maybe I'm
> > missing something.
>
> Yeah, it's not that complicated to initialize those things in
> ExecGetResultRelation().  In fact, ExecGetResultRelation() (or its
> subroutine ExecBuildResultRelation()) housed those initializations in
> the earlier versions of this patch, but I changed that after our
> discussion about being lazy about initializing as much stuff as we
> can.  Maybe I should revert that?
Please check the attached if that looks better.

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

v7-0001-Call-BeginDirectModify-from-ExecInitModifyTable.patch (5K) Download Attachment
v7-0002-Initialize-result-relation-information-lazily.patch (70K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Heikki Linnakangas
On 03/11/2020 10:27, Amit Langote wrote:
> Please check the attached if that looks better.

Great, thanks! Yeah, I like that much better.

This makes me a bit unhappy:

>
> /* Also let FDWs init themselves for foreign-table result rels */
> if (resultRelInfo->ri_FdwRoutine != NULL)
> {
> if (resultRelInfo->ri_usesFdwDirectModify)
> {
> ForeignScanState *fscan = (ForeignScanState *) mtstate->mt_plans[i];
>
> /*
> * For the FDW's convenience, set the ForeignScanState node's
> * ResultRelInfo to let the FDW know which result relation it
> * is going to work with.
> */
> Assert(IsA(fscan, ForeignScanState));
> fscan->resultRelInfo = resultRelInfo;
> resultRelInfo->ri_FdwRoutine->BeginDirectModify(fscan, eflags);
> }
> else if (resultRelInfo->ri_FdwRoutine->BeginForeignModify != NULL)
> {
> List   *fdw_private = (List *) list_nth(node->fdwPrivLists, i);
>
> resultRelInfo->ri_FdwRoutine->BeginForeignModify(mtstate,
> resultRelInfo,
> fdw_private,
> i,
> eflags);
> }
> }

If you remember, I was unhappy with a similar assertion in the earlier
patches [1]. I'm not sure what to do instead though. A few options:

A) We could change FDW API so that BeginDirectModify takes the same
arguments as BeginForeignModify(). That avoids the assumption that it's
a ForeignScan node, because BeginForeignModify() doesn't take
ForeignScanState as argument. That would be consistent, which is nice.
But I think we'd somehow still need to pass the ResultRelInfo to the
corresponding ForeignScan, and I'm not sure how.

B) Look up the ResultRelInfo, and call BeginDirectModify(), on the first
call to ForeignNext().

C) Accept the Assertion. And add an elog() check in the planner for that
with a proper error message.

I'm leaning towards B), but maybe there's some better solution I didn't
think of? Perhaps changing the API would make sense in any case, it is a
bit weird as it is. Backwards-incompatible API changes are not nice, but
I don't think there are many FDWs out there that implement the
DirectModify functions. And those functions are pretty tightly coupled
with the executor and ModifyTable node details anyway, so I don't feel
like we can, or need to, guarantee that they stay unchanged across major
versions.

[1]
https://www.postgresql.org/message-id/19c23dd9-89ce-75a3-9105-5fc05a46f94a%40iki.fi

- Heikki


Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
On Tue, Nov 3, 2020 at 9:05 PM Heikki Linnakangas <[hidden email]> wrote:

> On 03/11/2020 10:27, Amit Langote wrote:
> > Please check the attached if that looks better.
>
> Great, thanks! Yeah, I like that much better.
>
> This makes me a bit unhappy:
>
> >
> >               /* Also let FDWs init themselves for foreign-table result rels */
> >               if (resultRelInfo->ri_FdwRoutine != NULL)
> >               {
> >                       if (resultRelInfo->ri_usesFdwDirectModify)
> >                       {
> >                               ForeignScanState *fscan = (ForeignScanState *) mtstate->mt_plans[i];
> >
> >                               /*
> >                                * For the FDW's convenience, set the ForeignScanState node's
> >                                * ResultRelInfo to let the FDW know which result relation it
> >                                * is going to work with.
> >                                */
> >                               Assert(IsA(fscan, ForeignScanState));
> >                               fscan->resultRelInfo = resultRelInfo;
> >                               resultRelInfo->ri_FdwRoutine->BeginDirectModify(fscan, eflags);
> >                       }
> >                       else if (resultRelInfo->ri_FdwRoutine->BeginForeignModify != NULL)
> >                       {
> >                               List   *fdw_private = (List *) list_nth(node->fdwPrivLists, i);
> >
> >                               resultRelInfo->ri_FdwRoutine->BeginForeignModify(mtstate,
> >                                                                                                                                resultRelInfo,
> >                                                                                                                                fdw_private,
> >                                                                                                                                i,
> >                                                                                                                                eflags);
> >                       }
> >               }
>
> If you remember, I was unhappy with a similar assertion in the earlier
> patches [1]. I'm not sure what to do instead though. A few options:
>
> A) We could change FDW API so that BeginDirectModify takes the same
> arguments as BeginForeignModify(). That avoids the assumption that it's
> a ForeignScan node, because BeginForeignModify() doesn't take
> ForeignScanState as argument. That would be consistent, which is nice.
> But I think we'd somehow still need to pass the ResultRelInfo to the
> corresponding ForeignScan, and I'm not sure how.

Maybe ForeignScan doesn't need to contain any result relation info
then?  ForeignScan.operation != CMD_SELECT is enough to tell it to
call IterateDirectModify() as today.

> B) Look up the ResultRelInfo, and call BeginDirectModify(), on the first
> call to ForeignNext().
>
> C) Accept the Assertion. And add an elog() check in the planner for that
> with a proper error message.
>
> I'm leaning towards B), but maybe there's some better solution I didn't
> think of?   Perhaps changing the API would make sense in any case, it is a
> bit weird as it is. Backwards-incompatible API changes are not nice, but
> I don't think there are many FDWs out there that implement the
> DirectModify functions. And those functions are pretty tightly coupled
> with the executor and ModifyTable node details anyway, so I don't feel
> like we can, or need to, guarantee that they stay unchanged across major
> versions.

B is not too bad, but I tend to prefer doing A too.

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


Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
On Wed, Nov 4, 2020 at 11:32 AM Amit Langote <[hidden email]> wrote:

> On Tue, Nov 3, 2020 at 9:05 PM Heikki Linnakangas <[hidden email]> wrote:
> > A) We could change FDW API so that BeginDirectModify takes the same
> > arguments as BeginForeignModify(). That avoids the assumption that it's
> > a ForeignScan node, because BeginForeignModify() doesn't take
> > ForeignScanState as argument. That would be consistent, which is nice.
> > But I think we'd somehow still need to pass the ResultRelInfo to the
> > corresponding ForeignScan, and I'm not sure how.
>
> Maybe ForeignScan doesn't need to contain any result relation info
> then?  ForeignScan.operation != CMD_SELECT is enough to tell it to
> call IterateDirectModify() as today.
>
> > B) Look up the ResultRelInfo, and call BeginDirectModify(), on the first
> > call to ForeignNext().
> >
> > C) Accept the Assertion. And add an elog() check in the planner for that
> > with a proper error message.
> >
> > I'm leaning towards B), but maybe there's some better solution I didn't
> > think of?   Perhaps changing the API would make sense in any case, it is a
> > bit weird as it is. Backwards-incompatible API changes are not nice, but
> > I don't think there are many FDWs out there that implement the
> > DirectModify functions. And those functions are pretty tightly coupled
> > with the executor and ModifyTable node details anyway, so I don't feel
> > like we can, or need to, guarantee that they stay unchanged across major
> > versions.
>
> B is not too bad, but I tend to prefer doing A too.

How about I update the 0001 patch to implement A?

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


Reply | Threaded
Open this post in threaded view
|

Re: ModifyTable overheads in generic plans

Amit Langote
In reply to this post by Amit Langote
On Wed, Nov 4, 2020 at 11:32 AM Amit Langote <[hidden email]> wrote:

> On Tue, Nov 3, 2020 at 9:05 PM Heikki Linnakangas <[hidden email]> wrote:
> > On 03/11/2020 10:27, Amit Langote wrote:
> > > Please check the attached if that looks better.
> >
> > Great, thanks! Yeah, I like that much better.
> >
> > This makes me a bit unhappy:
> >
> > >
> > >               /* Also let FDWs init themselves for foreign-table result rels */
> > >               if (resultRelInfo->ri_FdwRoutine != NULL)
> > >               {
> > >                       if (resultRelInfo->ri_usesFdwDirectModify)
> > >                       {
> > >                               ForeignScanState *fscan = (ForeignScanState *) mtstate->mt_plans[i];
> > >
> > >                               /*
> > >                                * For the FDW's convenience, set the ForeignScanState node's
> > >                                * ResultRelInfo to let the FDW know which result relation it
> > >                                * is going to work with.
> > >                                */
> > >                               Assert(IsA(fscan, ForeignScanState));
> > >                               fscan->resultRelInfo = resultRelInfo;
> > >                               resultRelInfo->ri_FdwRoutine->BeginDirectModify(fscan, eflags);
> > >                       }
> > >                       else if (resultRelInfo->ri_FdwRoutine->BeginForeignModify != NULL)
> > >                       {
> > >                               List   *fdw_private = (List *) list_nth(node->fdwPrivLists, i);
> > >
> > >                               resultRelInfo->ri_FdwRoutine->BeginForeignModify(mtstate,
> > >                                                                                                                                resultRelInfo,
> > >                                                                                                                                fdw_private,
> > >                                                                                                                                i,
> > >                                                                                                                                eflags);
> > >                       }
> > >               }
> >
> > If you remember, I was unhappy with a similar assertion in the earlier
> > patches [1]. I'm not sure what to do instead though. A few options:
> >
> > A) We could change FDW API so that BeginDirectModify takes the same
> > arguments as BeginForeignModify(). That avoids the assumption that it's
> > a ForeignScan node, because BeginForeignModify() doesn't take
> > ForeignScanState as argument. That would be consistent, which is nice.
> > But I think we'd somehow still need to pass the ResultRelInfo to the
> > corresponding ForeignScan, and I'm not sure how.
>
> Maybe ForeignScan doesn't need to contain any result relation info
> then?  ForeignScan.operation != CMD_SELECT is enough to tell it to
> call IterateDirectModify() as today.
Hmm, I misspoke.   We do still need ForeignScanState.resultRelInfo,
because the IterateDirectModify() API uses it to return the remotely
inserted/updated/deleted tuple for the RETURNING projection performed
by ExecModifyTable().

> > B) Look up the ResultRelInfo, and call BeginDirectModify(), on the first
> > call to ForeignNext().
> >
> > C) Accept the Assertion. And add an elog() check in the planner for that
> > with a proper error message.
> >
> > I'm leaning towards B), but maybe there's some better solution I didn't
> > think of?   Perhaps changing the API would make sense in any case, it is a
> > bit weird as it is. Backwards-incompatible API changes are not nice, but
> > I don't think there are many FDWs out there that implement the
> > DirectModify functions. And those functions are pretty tightly coupled
> > with the executor and ModifyTable node details anyway, so I don't feel
> > like we can, or need to, guarantee that they stay unchanged across major
> > versions.
>
> B is not too bad, but I tend to prefer doing A too.
On second thought, it seems A would amount to merely a cosmetic
adjustment of the API, nothing more.  B seems to get the job done for
me and also doesn't unnecessarily break compatibility, so I've updated
0001 to implement B.  Please give it a look.

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

v8-0001-Set-ForeignScanState.resultRelInfo-lazily.patch (5K) Download Attachment
v8-0002-Initialize-result-relation-information-lazily.patch (69K) Download Attachment
123