On Sat, 17 Nov 2018 at 07:28, Alvaro Herrera 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
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
Description: PGP signature
Hi,
On Thu, Nov 22, 2018 at 7:25 AM David Rowley
wrote:
>
> On Thu, 22 Nov 2018 at 07:06, Alvaro Herrera 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
On Thu, 22 Nov 2018 at 07:06, Alvaro Herrera 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
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
On Sat, 17 Nov 2018 at 04:14, Alvaro Herrera 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
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
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
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
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
On Fri, Nov 16, 2018 at 11:40 AM Alvaro Herrera
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
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
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
>> wrote:
>>> What's with this comment?
>>>
>>> * Initially we must only set up 1 PartitionDispatch object; the
>>> one for
>>> * the partitioned
On 2018-Nov-15, David Rowley wrote:
> On 15 November 2018 at 07:10, Alvaro Herrera 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
> >
Thanks for picking this up.
On 15 November 2018 at 07:10, Alvaro Herrera 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
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
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
> wrote:
>> +if (dispatchidx >= proute->dispatch_allocsize)
>> +{
>> +/* Expand allocated space. */
>> +
Thanks for looking at this again.
On 14 November 2018 at 13:47, Amit Langote
wrote:
> +if (dispatchidx >= proute->dispatch_allocsize)
> +{
> +/* Expand allocated space. */
> +proute->dispatch_allocsize *= 2;
> +proute->partition_dispatch_info =
On 2018/11/14 0:32, Jesper Pedersen wrote:
> Hi,
>
> On 11/12/18 6:17 PM, David Rowley wrote:
>> On 9 November 2018 at 19:18, Amit Langote
>> wrote:
>>> I have a comment regarding how you chose to make
>>> PartitionTupleRouting private.
>>>
>>> Using the v14_to_v15 diff, I could quickly see that
Hi,
On 11/12/18 6:17 PM, David Rowley wrote:
On 9 November 2018 at 19:18, Amit Langote wrote:
I have a comment regarding how you chose to make
PartitionTupleRouting private.
Using the v14_to_v15 diff, I could quickly see that there are many diffs
changing PartitionTupleRouting to struct
On 9 November 2018 at 19:18, Amit Langote wrote:
> I have a comment regarding how you chose to make
> PartitionTupleRouting private.
>
> Using the v14_to_v15 diff, I could quickly see that there are many diffs
> changing PartitionTupleRouting to struct PartitionTupleRouting, but they
> would be
On 2018/11/08 20:28, David Rowley wrote:
> On 8 November 2018 at 20:15, Amit Langote
> wrote:
>> Actually, as I also proposed upthread, we should move root_tuple_slot from
>> PartitionTupleRouting to ModifyTableState as mt_root_tuple_slot, because
>> it's part of the first step described above
On 9 November 2018 at 00:28, David Rowley wrote:
> I've attached v15 and a delta from v14 to ease re-review.
I just revived the 0002 patch, which is still just for testing at this
stage. There was mention over on [1] about removing the
find_all_inheritors() call.
Also some benchmarks from v14
On Thu, Nov 8, 2018 at 6:28 AM David Rowley
wrote:
> I've attached v15 and a delta from v14 to ease re-review.
>
> I also ran pgindent on this version. That's not part of the delta but
> is in the main patch.
Did you notice
On 8 November 2018 at 20:15, Amit Langote wrote:
> Actually, as I also proposed upthread, we should move root_tuple_slot from
> PartitionTupleRouting to ModifyTableState as mt_root_tuple_slot, because
> it's part of the first step described above that has nothing to do with
> partition tuple
On 2018/11/07 20:46, David Rowley wrote:
> On 5 November 2018 at 20:17, Amit Langote
> wrote:
>> On 2018/11/04 19:07, David Rowley wrote:
>>> Perhaps a better design would be to instead of having random special
>>> partitioned-table-only fields in ResultRelInfo, just have an extra
>>> struct
Hi David,
On 11/7/18 6:46 AM, David Rowley wrote:
I've attached a patch which does this. It adds a new struct named
PartitionRoutingInfo into ResultRelInfo and pulls 3 of the 4 arrays
out of PartitionTupleRouting. There are fields for each of what these
arrays used to store inside the
On 5 November 2018 at 20:17, Amit Langote wrote:
> On 2018/11/04 19:07, David Rowley wrote:
>> Perhaps a better design would be to instead of having random special
>> partitioned-table-only fields in ResultRelInfo, just have an extra
>> struct there that contains the extra partition information
On 11/4/18 5:07 AM, David Rowley wrote:
I've attached v13 which hopefully addresses these.
I ran a test against the INSERT case using a 64 hash partition.
Non-partitioned table: ~73k TPS
Master: ~25k TPS
0001: ~26k TPS
0001 + 0002: ~68k TPS
The profile of 0001 shows that almost all of
On 2018/11/04 19:07, David Rowley wrote:
> On 1 November 2018 at 22:39, Amit Langote
> wrote:
> I've attached v13 which hopefully addresses these.
Thank you for updating the patch.
>> The macro naming discussion got me thinking today about the macro itself.
>> It encapsulates access to the
On 1 November 2018 at 22:39, Amit Langote wrote:
> On 2018/11/01 10:30, David Rowley wrote:
>> It's great to know the patch is now so perfect that we've only the
>> macro naming left to debate ;-)
>
> I looked over v12 again and noticed a couple minor issues.
>
> + * table then we
On 2018/11/01 10:30, David Rowley wrote:
> It's great to know the patch is now so perfect that we've only the
> macro naming left to debate ;-)
I looked over v12 again and noticed a couple minor issues.
+ * table then we store the index into parenting
+ *
On 01/11/2018 14:30, David Rowley wrote:
On 1 November 2018 at 13:35, Amit Langote wrote:
On 2018/11/01 8:58, David Rowley wrote:
[...]
I agree. I don't think "TupRouting" really needs to be in the name.
Probably "To" can also just become "2" and we can put back the
Parent/Child before
On 1 November 2018 at 13:35, Amit Langote wrote:
> On 2018/11/01 8:58, David Rowley wrote:
>> On 1 November 2018 at 06:45, Robert Haas wrote:
>>> I think a better way to shorten the name would be to truncate the
>>> PartitionTupRouting() prefix in some way, e.g. dropping TupRouting.
>>
>> Thanks
On 2018/11/01 8:58, David Rowley wrote:
> On 1 November 2018 at 06:45, Robert Haas wrote:
>> On Wed, Aug 22, 2018 at 8:30 AM David Rowley
>> wrote:
>>> On 22 August 2018 at 19:08, Amit Langote
>>> wrote:
+#define PartitionTupRoutingGetToParentMap(p, i) \
+#define
On 1 November 2018 at 06:45, Robert Haas wrote:
> On Wed, Aug 22, 2018 at 8:30 AM David Rowley
> wrote:
>> On 22 August 2018 at 19:08, Amit Langote
>> wrote:
>> > +#define PartitionTupRoutingGetToParentMap(p, i) \
>> > +#define PartitionTupRoutingGetToChildMap(p, i) \
>> >
>> > If the "Get"
On Wed, Aug 22, 2018 at 8:30 AM David Rowley
wrote:
> On 22 August 2018 at 19:08, Amit Langote
> wrote:
> > +#define PartitionTupRoutingGetToParentMap(p, i) \
> > +#define PartitionTupRoutingGetToChildMap(p, i) \
> >
> > If the "Get" could be replaced by "Child" and "Parent", respectively,
> >
Thanks for both clarifications!
I skimmed through the commits related to Inserts with partitioning
since 10 and indeed - while not impossible it seems like quite some
work to merge them into PG 10 codebase.
We might consider preparing the patch in-house as otherwise PG 10
based partitioning is a
On 2018/10/30 8:41, Krzysztof Nienartowicz wrote:
> On Thu, Oct 25, 2018 at 5:58 PM Krzysztof Nienartowicz
> wrote:
>> On Tue, Oct 23, 2018 at 4:02 AM David Rowley
>> wrote:
>>>
>>> I more meant that it might be 0002 that fixes the issue for you. I
>>> just wanted to check if you'd tried 0001
To complement the info: number of columns varies from 20 to 100 but
some of the columns are composite types or arrays of composite types.
The flamegraph after applying changes from patch 0002 can be seen
here: https://gaiaowncloud.isdc.unige.ch/index.php/s/W3DLecAWAfkesiP
shows most of the time
In the end we hacked the code to re-enable triggers on partitioned
tables and switch off native insert code on partitioned tables. Quite
hackish and would be nice to have it fixed in a more natural manner.
Yes, it looked like locking but not only -
ExecSetupPartitionTupleRouting:
On 15 October 2018 at 23:04, Krzysztof Nienartowicz
wrote:
> We see quite prohibitive 5-6x slowdown with native partitioning on in
> comparison to trigger based in PG9.5.
> This is clearly visible with highly parallel inserts (Can share
> flamediagrams comparing the two).
Does the 0001 patch
Hi,
On 2018/10/15 19:04, Krzysztof Nienartowicz wrote:
>
> We see quite prohibitive 5-6x slowdown with native partitioning on in
> comparison to trigger based in PG9.5.
> This is clearly visible with highly parallel inserts (Can share
> flamediagrams comparing the two).
>
> This basically
We see quite prohibitive 5-6x slowdown with native partitioning on in
comparison to trigger based in PG9.5.
This is clearly visible with highly parallel inserts (Can share
flamediagrams comparing the two).
This basically excludes native partitioning from being used by us. Do you
think your
On 9 October 2018 at 15:49, Amit Langote wrote:
> On 2018/10/05 21:55, David Rowley wrote:
>> I'm not so sure we need to zero the partition_tuple_slots[] array at
>> all since we always set a value there is there's a corresponding map
>> stored. I considered pulling that out, but in the end, I
Hi David,
On 2018/10/05 21:55, David Rowley wrote:
> On 17 September 2018 at 21:15, David Rowley
> wrote:
>> v9 patch attached. Fixes conflict with 6b78231d.
>
> v10 patch attached. Fixes conflict with cc2905e9.
Thanks for rebasing.
> I'm not so sure we need to zero the
On 17 September 2018 at 21:15, David Rowley
wrote:
> v9 patch attached. Fixes conflict with 6b78231d.
v10 patch attached. Fixes conflict with cc2905e9.
I'm not so sure we need to zero the partition_tuple_slots[] array at
all since we always set a value there is there's a corresponding map
On 23 August 2018 at 00:30, David Rowley
wrote:
> I've attached a v8. The only change from your v7 is in the "go making"
> comment.
>
v9 patch attached. Fixes conflict with 6b78231d.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training
On 2018/08/22 21:30, David Rowley wrote:
> On 22 August 2018 at 19:08, Amit Langote
> wrote:
>> +#define PartitionTupRoutingGetToParentMap(p, i) \
>> +#define PartitionTupRoutingGetToChildMap(p, i) \
>>
>> If the "Get" could be replaced by "Child" and "Parent", respectively,
>> they'd sound more
On 22 August 2018 at 19:08, Amit Langote wrote:
> +#define PartitionTupRoutingGetToParentMap(p, i) \
> +#define PartitionTupRoutingGetToChildMap(p, i) \
>
> If the "Get" could be replaced by "Child" and "Parent", respectively,
> they'd sound more meaningful, imho.
I did that to save 3 chars. I
On 2018/08/21 14:44, David Rowley wrote:
> On 3 August 2018 at 17:58, Amit Langote wrote:
>> On 2018/07/31 16:03, David Rowley wrote:
>>> Maybe we can do that as a follow-on patch.
>>
>> We probably could, but I think it would be a good idea get rid of *all*
>> redundant allocations due to tuple
On 3 August 2018 at 17:58, Amit Langote wrote:
> On 2018/07/31 16:03, David Rowley wrote:
>> Maybe we can do that as a follow-on patch.
>
> We probably could, but I think it would be a good idea get rid of *all*
> redundant allocations due to tuple routing in one patch, if that's the
> mission of
(looking at the v5 patch but replying to an older email)
On 2018/07/31 16:03, David Rowley wrote:
> I've attached a complete v4 patch.
>
>> By the way, when going over the updated code, I noticed that the code
>> around child_parent_tupconv_maps could use some refactoring too.
>> Especially, I
On 31 July 2018 at 19:03, David Rowley wrote:
> I've attached a complete v4 patch.
I've attached v5 of the patch which is based on top of today's master
(@ 579b985b22)
A couple of recent patches conflict with v4. I've also made another
tidy up pass, which was mostly just rewording comments in
On 30 July 2018 at 20:26, Amit Langote wrote:
> I couldn't find much to complain about in the latest v3, except I noticed
> a few instances of the word "setup" where I think what's really meant is
> "set up".
>
> + * must be setup, but any sub-partitioned tables can be setup lazily as
>
> +
On 2018/07/28 10:54, David Rowley wrote:
> On 27 July 2018 at 19:11, Amit Langote wrote:
>> I've attached a delta patch to make the above changes. I'm leaving the
>> hash table rename up to you though.
>
> Thanks for the delta patch. I took all of it, just rewrote a comment slightly.
>
> I
On 27 July 2018 at 19:11, Amit Langote wrote:
> I've attached a delta patch to make the above changes. I'm leaving the
> hash table rename up to you though.
Thanks for the delta patch. I took all of it, just rewrote a comment slightly.
I also renamed the hash table to your suggestion and
On 2018/07/27 1:19, David Rowley wrote:
> On 18 July 2018 at 20:29, Amit Langote wrote:
>> Let me know what you think of the code in the updated patch.
>
> Thanks for sending the updated patch.
>
> I looked over it tonight and made a number of changes:
>
> 1) Got rid of
On 27 July 2018 at 04:19, David Rowley wrote:
> I've attached a delta of the changes I made since your v2 delta and
> also a complete updated patch.
I did a very quick performance test of this patch on an AWS m5d.large
instance with fsync=off.
The test setup is the same as is described in my
David Rowley writes:
> 1) Got rid of PARTITION_ROUTING_MAXSIZE. The code using this was
> useless since the int would have wrapped long before it reached
> UINT_MAX. There's no shortage of other code doubling the size of an
> array by multiplying it by 2 unconditionally without considering
>
On 18 July 2018 at 20:29, Amit Langote wrote:
> Let me know what you think of the code in the updated patch.
Thanks for sending the updated patch.
I looked over it tonight and made a number of changes:
1) Got rid of PARTITION_ROUTING_MAXSIZE. The code using this was
useless since the int would
On 18 July 2018 at 21:44, Kato, Sho wrote:
> part_num | latency_avg | tps_ex | update_latency | select_latency |
> insert_latency
> --+-++++
> 100 |2.09 | 478.379516 | 1.407 |
Alvaro Herrera [mailto:alvhe...@2ndquadrant.com]
Sent: Wednesday, July 11, 2018 10:30 PM
To: David Rowley
Cc: Kato, Sho/加藤 翔 ; PostgreSQL Hackers
Subject: Re: Speeding up INSERTs and UPDATEs to partitioned tables
On 2018-Jul-11, David Rowley wrote:
> On 6 July 2018 at 21:25, Kato, Sho wro
Hi David,
Thanks for taking a look.
On 2018/07/15 17:34, David Rowley wrote:
> I've looked over the code and the ExecUseUpdateResultRelForRouting()
> function is broken. Your while loop only skips partitions for the
> current partitioned table, it does not skip ModifyTable subnodes that
>
On 13 July 2018 at 20:20, Amit Langote wrote:
> Why don't we abandon the notion altogether that
> ExecSetupPartitionTupleRouting *has to* process the whole partition tree?
[...]
> I implemented that idea in the attached patch, which applies on top of
> your 0001 patch, but I'd say it's too big
Hi David.
On 2018/06/22 15:28, David Rowley wrote:
> Hi,
>
> As part of my efforts to make partitioning scale better for larger
> numbers of partitions, I've been looking at primarily INSERT VALUES
> performance. Here the overheads are almost completely in the
> executor. Planning of this type
On 2018-Jul-11, David Rowley wrote:
> On 6 July 2018 at 21:25, Kato, Sho wrote:
> > 2. 11beta2 + patch1 + patch2
> >
> > patch1: Allow direct lookups of AppendRelInfo by child relid
> > commit 7d872c91a3f9d49b56117557cdbb0c3d4c620687
> > patch2:
On 6 July 2018 at 21:25, Kato, Sho wrote:
> 2. 11beta2 + patch1 + patch2
>
> patch1: Allow direct lookups of AppendRelInfo by child relid
> commit 7d872c91a3f9d49b56117557cdbb0c3d4c620687
> patch2: 0001-Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch
>
> part_num | tps_ex|
[mailto:david.row...@2ndquadrant.com]
Sent: Thursday, July 05, 2018 6:19 PM
To: Kato, Sho/加藤 翔
Cc: PostgreSQL Hackers
Subject: Re: Speeding up INSERTs and UPDATEs to partitioned tables
On 5 July 2018 at 18:39, Kato, Sho wrote:
> postgres=# create table a(i int) partition by range(i); CREATE TA
On 6 July 2018 at 01:18, Jesper Pedersen wrote:
> With 0002 INSERTs get close to the TPS of the non-partitioned case. However,
> UPDATEs doesn't see the same speedup. But, as you said, a discussion for
> another thread.
Hi Jesper,
Thanks for testing this out. It was only really the locking I
Hi David,
On 06/22/2018 02:28 AM, David Rowley wrote:
I've attached 2 patches:
0001: implements items 1-6
0002: Is not intended for commit. It just a demo of where we could get
the performance if we were smarter about locking partitions. I've just
included this to show 0001's worth.
I did
On 5 July 2018 at 18:39, Kato, Sho wrote:
> postgres=# create table a(i int) partition by range(i);
> CREATE TABLE
> postgres=# create table a_1 partition of a for values from(1) to (200);
> CREATE TABLE
> postgres=# create table a_2 partition of a for values from(200) to (400);
> server closed
rtition_bound_has_default(partdesc->boundinfo))
269 return
partdesc->oids[partdesc->boundinfo->default_index];
270
271 return InvalidOid;
272 }
273
regards,
-Original Message-
From: David Rowley [mailto:david.row...@2ndquadrant.com]
Sent:
On 22 June 2018 at 18:28, David Rowley wrote:
> I've written fixes for items 1-6 above.
>
> I did:
>
> 1. Use an array instead of a List.
> 2. Don't do this loop. palloc0() the partitions array instead. Let
> UPDATE add whatever subplans exist to the zeroed array.
> 3. Track what we initialize in
74 matches
Mail list logo
