On 02/08/2018 01:36, David Rowley wrote:
> On 31 July 2018 at 11:51, David Rowley wrote:
>> The attached v6 delta replaces the v5 delta and should be applied on
>> top of the full v4 patch.
>
> (now committed)
>
> Many thanks for committing this Peter and many thanks to Melanie and
> Karen for r
On 31 July 2018 at 11:51, David Rowley wrote:
> The attached v6 delta replaces the v5 delta and should be applied on
> top of the full v4 patch.
(now committed)
Many thanks for committing this Peter and many thanks to Melanie and
Karen for reviewing it!
--
David Rowley http:
On 31 July 2018 at 06:21, Peter Eisentraut
wrote:
> On 30/07/2018 15:26, David Rowley wrote:
>>> - Add some tests. The if (nBufferedTuples > 0) that flushes the tuples
>>> when the partition changes is not currently exercised.
>>
>> That seems like a good idea. In fact, it uncovered a bug around
On Mon, Jul 30, 2018 at 11:21 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
> On 30/07/2018 15:26, David Rowley wrote:
> >> - Add some tests. The if (nBufferedTuples > 0) that flushes the tuples
> >> when the partition changes is not currently exercised.
> >
> > That seems like
On 30/07/2018 15:26, David Rowley wrote:
>> - Add some tests. The if (nBufferedTuples > 0) that flushes the tuples
>> when the partition changes is not currently exercised.
>
> That seems like a good idea. In fact, it uncovered a bug around
> ConvertPartitionTupleSlot() freeing the previously sto
On 30 July 2018 at 20:33, Peter Eisentraut
wrote:
> Two more thoughts:
>
> - Add some tests. The if (nBufferedTuples > 0) that flushes the tuples
> when the partition changes is not currently exercised.
That seems like a good idea. In fact, it uncovered a bug around
ConvertPartitionTupleSlot() f
On 2018/07/30 17:33, Peter Eisentraut wrote:
> - With proute becoming a function-level variable,
> cstate->partition_tuple_routing is obsolete and could be removed. (No
> point in saving this in cstate if it's only used in one function anyway.)
+1. Also seems to apply to transition_capture, whic
Two more thoughts:
- Add some tests. The if (nBufferedTuples > 0) that flushes the tuples
when the partition changes is not currently exercised.
- With proute becoming a function-level variable,
cstate->partition_tuple_routing is obsolete and could be removed. (No
point in saving this in cstate
On Sat, Jul 28, 2018 at 6:00 PM, David Rowley
wrote:
> Oops. Must've fallen off in transit :) Hopefully, it's more firmly
> attached this time.
>
LGTM. Status changed to "ready for committer"
On 29 July 2018 at 05:24, Melanie Plageman wrote:
> On Thu, Jul 26, 2018 at 7:26 PM, David Rowley
> wrote:
>>
>> Fixed in the attached v4. That's the only change.
>
>
> I don't see an attachment.
Oops. Must've fallen off in transit :) Hopefully, it's more firmly
attached this time.
> We are pr
On Thu, Jul 26, 2018 at 7:26 PM, David Rowley
wrote:
> Fixed in the attached v4. That's the only change.
>
I don't see an attachment.
> So, I thinking I'm missing something. Which of the changes would cause the
> > performance improvement from patch v2 to v3? My understanding is:
>
> You could
On Wed, Jul 25, 2018 at 10:30 PM, David Rowley
wrote:
> I agree RANGE partition is probably the most likely case to benefit
> from this optimisation, but I just don't think that HASH could never
> benefit and LIST probably sits somewhere in the middle.
>
> HASH partitioning might be useful in case
On 27 July 2018 at 05:30, Melanie Plageman wrote:
> On patched code line 2564, there is a missing parenthesis. It might be
> better to remove the parentheses entirely and, while you're at it, there is
> a missing comma.
Thanks for noticing that.
Fixed in the attached v4. That's the only change.
On Wed, Jul 25, 2018 at 7:24 PM, David Rowley
wrote:
On patched code line 2564, there is a missing parenthesis. It might be
better to remove the parentheses entirely and, while you're at it, there is
a missing comma.
/*
* For partitioned tables we can't support multi-inserts when there
* are a
On 26 July 2018 at 03:30, David Rowley wrote:
> The v3 version of the patch also fixes the very small performance
> regression for the (probably quite likely) worst-case situation. New
> performance is about 3.5% faster instead of 0.5-1% slower. So likely
> there's no longer any need to consider
On 25 July 2018 at 04:37, Simon Riggs wrote:
> I don't see any need here for another GUC, nor even a command option.
> The user has already indicated their use case to us:
I agree.
> We know that the common case for RANGE partitioned tables is to load
> into the one current partition. We also kn
Hi Melanie,
Many thanks for looking over this again.
On 25 July 2018 at 03:32, Melanie Plageman wrote:
> One small additional typo I noticed:
>
> In the patched code on line 2555, there appears to be a typo:
> /* ...
> * inserting into and act differently if the tuples that have already
> * b
On 24 July 2018 at 16:32, Melanie Plageman wrote:
>
> On Fri, Jul 20, 2018 at 7:57 AM, David Rowley
> wrote:
>> One final note: I'm not entirely convinced we need this adaptive
>> code, but it seems easy enough to rip it back out if it's more trouble
>> than it's worth. But if the other option
On Fri, Jul 20, 2018 at 7:57 AM, David Rowley
wrote:
> > So, overall, we feel that the code from lines 2689 until 2691 and from
> 2740 to 2766 could use further clarification with regard to switching from
> parent to child partition and sibling to sibling partition as well as
> regarding saving r
On 24 July 2018 at 06:38, Peter Eisentraut
wrote:
> On 20.07.18 16:57, David Rowley wrote:
>> One final note: I'm not entirely convinced we need this adaptive
>> code, but it seems easy enough to rip it back out if it's more trouble
>> than it's worth. But if the other option is a GUC, then I'd r
On 20.07.18 16:57, David Rowley wrote:
> One final note: I'm not entirely convinced we need this adaptive
> code, but it seems easy enough to rip it back out if it's more trouble
> than it's worth. But if the other option is a GUC, then I'd rather
> stick with the adaptive code, it's likely going
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation:not tested
This patch applied cleanly and worked as expected.
Patch description
(This email seemed to only come to me and somehow missed the mailing
list. Including the message here in its entirety)
On 20 July 2018 at 13:26, Karen Huddleston wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Impleme
I was looking at the COPY FROM performance gap between bulk loads with
partitioned tables vs non-partitioned tables. There's quite a gap!
Almost twice as slow in my test.
It seems to be mostly down to lack of usage of heap_multi_insert() for
the partitioned table case, which I guess is because we
24 matches
Mail list logo
