On Wed, 27 Mar 2019 at 18:49, Andres Freund <andres@anarazel.de> wrote:
> Here's a version that applies onto HEAD. It needs a bit more cleanup
> (I'm not sure I like the addition of ri_batchInsertSlots to store slots
> for each partition, there's some duplicated code around slot array and
> slot creation, docs for the multi insert callback).
>
> When measuring inserting into unlogged partitions (to remove the
> overhead of WAL logging, which makes it harder to see the raw
> performance difference), I see a few percent gains of the slotified
> version over the current code. Same with insertions that have fewer
> partition switches.
>
> Sorry for posting this here - David, it'd be cool if you could take a
> look anyway. You've played a lot with this code.
Hi,
I had a look at this and performance has improved again, thanks.
However, I'm not sure if the patch is exactly what we need, let me
explain.
When I was working on 0d5f05cde01, I think my original idea was just
to use the bufferedTuples[] array and always multi insert into
partitioned tables unless something like on insert triggers stopped
that. The reason I ended up with all this CIM_MULTI and
CIM_MULTI_CONDITIONAL stuff is due to Melanie finding a regression
when the target partition changed on each tuple. At the time I
wondered if it might be worth trying to have multiple buffers or to
partition the existing buffer and store tuples belonging to different
partitions, then just flush them when they're full. Looking at the
patch it seems that you have added an array of slots per partition, so
does that not kinda make all that CIM_MULTI / CIM_MULTI_CONDITIONAL
code redundant? ... we could just flush each partition's buffer when
it gets full. There's not much of a need to flush the buffer when the
partition changes since we're using a different buffer for the next
partition anyway.
I also wonder about memory usage here. If we're copying to a
partitioned table with 10k partitions and we get over
MAX_BUFFERED_TUPLES in a row for each partition, we'll end up with
quite a lot of slots.
So, in short. I think the patch either is going to cause possible
memory problems during COPY FROM, or it leaves code that is pretty
much redundant. ... Or I've just misunderstood the patch :)
Does this make sense?
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
