Speeding up INSERTs and UPDATEs to partitioned tables

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

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Amit Langote-2
Thanks for updating the patch.

On 2018/11/14 13:16, David Rowley wrote:

> Thanks for looking at this again.
>
> On 14 November 2018 at 13:47, Amit Langote
> <[hidden email]> wrote:
>> +    if (dispatchidx >= proute->dispatch_allocsize)
>> +    {
>> +        /* Expand allocated space. */
>> +        proute->dispatch_allocsize *= 2;
>> +        proute->partition_dispatch_info = (PartitionDispatchData **)
>> +            repalloc(proute->partition_dispatch_info,
>> +                     sizeof(PartitionDispatchData *) *
>> +                     proute->dispatch_allocsize);
>> +    }
>>
>> Sorry, I forgot to point this out before, but can this code in
>> ExecInitPartitionDispatchInfo be accommodated in
>> ExecCheckPartitionArraySpace() for consistency?
>
> I don't really want to put that code in ExecCheckPartitionArraySpace()
> as the way the function is now, it makes quite a lot of sense for the
> compiler to inline it. If we add redundant work in there, then it
> makes less sense.  There's never any need to check both arrays at once
> as we're only adding the new item to one array at a time.
>
> Instead, I've written a new function named
> ExecCheckDispatchArraySpace() and put the resize code inside that.

Okay, seems fine.

> I've fixed the typos you mentioned. The only other thing I changed was
> to only allocate the PartitionDispatch->tupslot if a conversion is
> required.  The previous code allocated this regardless if it was going
> to be used or not.  This saves both the redundant allocation and also
> very slightly reduces the cost of the if test in ExecFindPartition().
> There's now no need to check if the map ! NULL as if the slot is there

Also makes sense.

Although it seems that Alvaro has already started at looking at this, I'll
mark the CF entry as Ready for Committer anyway, because I don't have any
more comments. :)

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Alvaro Herrera-9
In reply to this post by David Rowley-3
What's with this comment?

         * Initially we must only set up 1 PartitionDispatch object; the one for
         * the partitioned table that's the target of the command.  If we must
         * route a tuple via some sub-partitioned table, then its
         * PartitionDispatch is only built the first time it's required.

You're setting the allocsize to PARTITION_ROUTING_INITSIZE, which is at
odds with the '1' mentioned in the comment.  Which is wrong?

(I have a few edits on the patch, so please don't send a full v18 -- a
delta patch would be welcome, if you have further changes to propose.)

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

David Rowley-3
Thanks for picking this up.

On 15 November 2018 at 07:10, Alvaro Herrera <[hidden email]> wrote:
> What's with this comment?
>
>          * Initially we must only set up 1 PartitionDispatch object; the one for
>          * the partitioned table that's the target of the command.  If we must
>          * route a tuple via some sub-partitioned table, then its
>          * PartitionDispatch is only built the first time it's required.
>
> You're setting the allocsize to PARTITION_ROUTING_INITSIZE, which is at
> odds with the '1' mentioned in the comment.  Which is wrong?

I don't think either is wrong, but I guess something must be
misleading, so could perhaps be improved.

We're simply allocating enough space for PARTITION_ROUTING_INITSIZE
but we're only initialising 1 item. That leaves space for
PARTITION_ROUTING_INITSIZE - 1 more items before we'd need to
reallocate the array.

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Alvaro Herrera-9
On 2018-Nov-15, David Rowley wrote:

> On 15 November 2018 at 07:10, Alvaro Herrera <[hidden email]> wrote:
> > What's with this comment?
> >
> >          * Initially we must only set up 1 PartitionDispatch object; the one for
> >          * the partitioned table that's the target of the command.  If we must
> >          * route a tuple via some sub-partitioned table, then its
> >          * PartitionDispatch is only built the first time it's required.
> >
> > You're setting the allocsize to PARTITION_ROUTING_INITSIZE, which is at
> > odds with the '1' mentioned in the comment.  Which is wrong?
>
> I don't think either is wrong, but I guess something must be
> misleading, so could perhaps be improved.

Ah, that makes sense.  Yeah, it seems a bit misleading to me.  No
worries.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Amit Langote-2
On 2018/11/15 8:58, Alvaro Herrera wrote:

> On 2018-Nov-15, David Rowley wrote:
>
>> On 15 November 2018 at 07:10, Alvaro Herrera <[hidden email]> wrote:
>>> What's with this comment?
>>>
>>>          * Initially we must only set up 1 PartitionDispatch object; the one for
>>>          * the partitioned table that's the target of the command.  If we must
>>>          * route a tuple via some sub-partitioned table, then its
>>>          * PartitionDispatch is only built the first time it's required.
>>>
>>> You're setting the allocsize to PARTITION_ROUTING_INITSIZE, which is at
>>> odds with the '1' mentioned in the comment.  Which is wrong?
>>
>> I don't think either is wrong, but I guess something must be
>> misleading, so could perhaps be improved.
>
> Ah, that makes sense.  Yeah, it seems a bit misleading to me.  No
> worries.

Maybe name it PARTITION_INIT_ALLOCSIZE (dropping the ROUTING from it), or
PROUTE_INIT_ALLOCSIZE, to make it clear that it's only allocation size?

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Alvaro Herrera-9
On 2018-Nov-15, Amit Langote wrote:

> Maybe name it PARTITION_INIT_ALLOCSIZE (dropping the ROUTING from it), or
> PROUTE_INIT_ALLOCSIZE, to make it clear that it's only allocation size?

Here's a proposed delta on v17 0001.  Most importantly, I noticed that
the hashed subplans stuff didn't actually work, because the hash API was
not being used correctly.  So the search in the hash would never return
a hit, and we'd create RRIs for those partitions again.  To fix this, I
added a new struct to hold hash entries.

I think this merits that the performance tests be redone.  (Unless I
misunderstand, this shouldn't change the performance of INSERT, only
that of UPDATE.)

On the subject of the ArraySpace routines, I decided to drop them and
instead do the re-allocations on the places where they were needed.
In the original code there were two places for the partitions array, but
both did the same thing so it made sense to create a separate routine to
do it instead (ExecRememberPartitionRel), and do the allocation there.
Just out of custom I moved the palloc to appear at the same place as the
repalloc, and after doing so it became obvious that we were
over-allocating memory for the PartitionDispatchData pointer --
allocating the size for the whole struct instead of just the pointer.

(I renamed the "allocsize" struct members to "max", as is customary.)

I added CHECK_FOR_INTERRUPTS to the ExecFindPartition loop.  It
shouldn't be useful if the code is correct, but if there are bugs it's
better to be able to interrupt infinite loops :-)

I reindented the comment atop PartitionTupleRouting.  The other way was
just too unwieldy.

Let me know what you think.  Regression tests still pass for me.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v18-delta.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Amit Langote
On Fri, Nov 16, 2018 at 11:40 AM Alvaro Herrera
<[hidden email]> wrote:

> On 2018-Nov-15, Amit Langote wrote:
>
> > Maybe name it PARTITION_INIT_ALLOCSIZE (dropping the ROUTING from it), or
> > PROUTE_INIT_ALLOCSIZE, to make it clear that it's only allocation size?
>
> Here's a proposed delta on v17 0001.  Most importantly, I noticed that
> the hashed subplans stuff didn't actually work, because the hash API was
> not being used correctly.  So the search in the hash would never return
> a hit, and we'd create RRIs for those partitions again.  To fix this, I
> added a new struct to hold hash entries.

I'm a bit surprised that you found that the hash table didn't work,
because I remember having checked by attaching gdb that it works when
I was hacking on my own delta patch, but I may have been looking at
too many things.

> I think this merits that the performance tests be redone.  (Unless I
> misunderstand, this shouldn't change the performance of INSERT, only
> that of UPDATE.)

Actually, I don't remember seeing performance tests done with UPDATEs
on this thread.

Since we don't needlessly scan *all* subplan result rels anymore,
maybe this removes a good deal of overhead for UPDATEs that update
partition key.

> On the subject of the ArraySpace routines, I decided to drop them and
> instead do the re-allocations on the places where they were needed.
> In the original code there were two places for the partitions array, but
> both did the same thing so it made sense to create a separate routine to
> do it instead (ExecRememberPartitionRel), and do the allocation there.
> Just out of custom I moved the palloc to appear at the same place as the
> repalloc, and after doing so it became obvious that we were
> over-allocating memory for the PartitionDispatchData pointer --
> allocating the size for the whole struct instead of just the pointer.
>
> (I renamed the "allocsize" struct members to "max", as is customary.)

These changes look good to me.

> I added CHECK_FOR_INTERRUPTS to the ExecFindPartition loop.  It
> shouldn't be useful if the code is correct, but if there are bugs it's
> better to be able to interrupt infinite loops :-)

Good measure. :)

> I reindented the comment atop PartitionTupleRouting.  The other way was
> just too unwieldy.
>
> Let me know what you think.  Regression tests still pass for me.

Overall, it looks good to me.

Thanks,
Amit

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Alvaro Herrera-9
One thing I don't quite like is the inconsistency in handling memory
context switches in the various function allocating stuff.  It seems
rather haphazard.  I'd rather have a memcxt member in
PartitionTupleRouting, which is set when the struct is created, and then
have all the other functions allocating stuff use that one.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Alvaro Herrera-9
On 2018-Nov-16, Alvaro Herrera wrote:

> One thing I don't quite like is the inconsistency in handling memory
> context switches in the various function allocating stuff.  It seems
> rather haphazard.  I'd rather have a memcxt member in
> PartitionTupleRouting, which is set when the struct is created, and then
> have all the other functions allocating stuff use that one.

So while researching this I finally realized that there was a "lexical
disconnect" between setting a ResultRelInfo's ri_PartitionInfo
struct/pointer and adding it to the PartitionTupleRoute arrays.
However, if you think about it, these things are one and the same, so we
don't need to do them separately; just merge the new function I wrote
into the existing ExecInitRoutingInfo().  Patch attached.

(This version also rebases across Andres' recent conflicting
TupleTableSlot changes.)

I'll now see about the commit message and push shortly.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v19-Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch (71K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Alvaro Herrera-9
I repeated David's original tests not terribly rigorously[*] and got
these numbers:

* Unpatched:    72.396196
* 0001     :    77.279404
* 0001+0002: 20409.415094
*      0002:   816.606613
* control  : 22969.140596 (insertion into unpartitioned table)

So while this patch by itself gives a pretty lame increase in tps, it
removes bottlenecks that will appear once we change the locking scheme.

[*] On my laptop, running each test only once for 60s.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Alvaro Herrera-9
In reply to this post by David Rowley-3
On 2018-Nov-13, David Rowley wrote:

> The 0002 patch is included again, this time with a new proposed commit
> message.  There was some discussion over on [1] where nobody seemed to
> have any concerns about delaying the locking until we route the first
> tuple to the partition.

Please get a new commitfest entry for this patch.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

David Rowley-3
In reply to this post by Alvaro Herrera-9
On Sat, 17 Nov 2018 at 04:14, Alvaro Herrera <[hidden email]> wrote:
> I'll now see about the commit message and push shortly.

Many thanks for making the required adjustments and pushing this.

If I wasn't on leave late last week and early this week then the only
thing I'd have mentioned was the lack of empty comment line in the
header comment for PartitionDispatchData. It looks a bit messy
without.

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Alvaro Herrera-9
On 2018-Nov-21, David Rowley wrote:

> If I wasn't on leave late last week and early this week then the only
> thing I'd have mentioned was the lack of empty comment line in the
> header comment for PartitionDispatchData. It looks a bit messy
> without.

Absolutely.  Pushed a few newlines -- I hope I understood you correctly.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

David Rowley-3
On Thu, 22 Nov 2018 at 07:06, Alvaro Herrera <[hidden email]> wrote:
> On 2018-Nov-21, David Rowley wrote:
> > If I wasn't on leave late last week and early this week then the only
> > thing I'd have mentioned was the lack of empty comment line in the
> > header comment for PartitionDispatchData. It looks a bit messy
> > without.
>
> Absolutely.  Pushed a few newlines -- I hope I understood you correctly.

Thanks, you did. That looks better now.

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Amit Langote
Hi,

On Thu, Nov 22, 2018 at 7:25 AM David Rowley
<[hidden email]> wrote:

>
> On Thu, 22 Nov 2018 at 07:06, Alvaro Herrera <[hidden email]> wrote:
> > On 2018-Nov-21, David Rowley wrote:
> > > If I wasn't on leave late last week and early this week then the only
> > > thing I'd have mentioned was the lack of empty comment line in the
> > > header comment for PartitionDispatchData. It looks a bit messy
> > > without.
> >
> > Absolutely.  Pushed a few newlines -- I hope I understood you correctly.
>
> Thanks, you did. That looks better now.
I noticed that there's a "be" missing in the comment above
ExecFindPartition.  Fixed in the attached.

Thanks,
Amit

ExecFindPartition-typo.patch (972 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

Michael Paquier-2
On Thu, Nov 22, 2018 at 11:32:04AM +0900, Amit Langote wrote:
> I noticed that there's a "be" missing in the comment above
> ExecFindPartition.  Fixed in the attached.

Thanks Amit, I have committed this one.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Speeding up INSERTs and UPDATEs to partitioned tables

David Rowley-3
In reply to this post by Alvaro Herrera-9
On Sat, 17 Nov 2018 at 07:28, Alvaro Herrera <[hidden email]> wrote:
> > The 0002 patch is included again, this time with a new proposed commit
> > message.  There was some discussion over on [1] where nobody seemed to
> > have any concerns about delaying the locking until we route the first
> > tuple to the partition.
>
> Please get a new commitfest entry for this patch.

Added to Jan-fest in: https://commitfest.postgresql.org/21/1887/

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

1234