From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Surafel Temesgen <surafel3000@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Adam Berlin <berlin.ab@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: COPY FROM WHEN condition
Date: Tue, 29 Jan 2019 10:51:04 +0100
Message-ID: <b5fe1ea4-5007-2183-bd11-a8210cca3cd8@2ndquadrant.com> (raw)
In-Reply-To: <CAKJS1f91By42p=oQHOF+n-dAyVTYGYGjHK2qg74xL=5=6MC9ig@mail.gmail.com>
References: <3b2b1aac-5861-acd8-fda3-054c4e4da888@2ndquadrant.com>
<CALAY4q-6ffQPi+CZMVjddEMXe_QBrqfz6aauc+QPxSLEO9vUQQ@mail.gmail.com>
<b5234b76-9ee0-6a44-6a21-d90c385a27b8@2ndquadrant.com>
<ea03bbf3-c201-5ce1-eee9-9fc4b527fd03@2ndquadrant.com>
<20190121012405.633row4iw7nfxz7h@alap3.anarazel.de>
<6e80b7c1-58cd-4212-41d3-7c519aed0500@2ndquadrant.com>
<20190121020805.3kdscvwzwvijlees@alap3.anarazel.de>
<20190121021220.7tbu3oizvydov5by@alap3.anarazel.de>
<52b10375-aca3-ea9b-10a1-2e4e9a011665@2ndquadrant.com>
<f66a0132-c707-0a3e-fb53-5880ec06bf28@2ndquadrant.com>
<20190121185102.m7keci23l7q6byou@alap3.anarazel.de>
<0a3d8cff-0b9d-ea2e-634b-5dcb8d0d5766@2ndquadrant.com>
<eb58de96-0722-bbff-c08a-758dc818473b@2ndquadrant.com>
<CAKJS1f91By42p=oQHOF+n-dAyVTYGYGjHK2qg74xL=5=6MC9ig@mail.gmail.com>
On 1/29/19 8:18 AM, David Rowley wrote:
> On Wed, 23 Jan 2019 at 06:35, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>> It turned out to be a tad more complex due to partitioning, because when
>> we find the partitions do not match, the tuple is already allocated in
>> the "current" context (be it per-tuple or batch). So we can't just free
>> the whole context at that point. The old code worked around this by
>> alternating two contexts, but that seems a bit too cumbersome to me, so
>> the patch simply copies the tuple to the new context. That allows us to
>> reset the batch context always, right after emptying the buffer. I need
>> to do some benchmarking to see if the extra copy causes any regression.
>
> I agree that fixing the problem mentioned by separating out tuple and
> batch contexts is a good idea, but I disagree with removing the
> alternating batch context idea. FWIW the reason the alternating
> contexts went in wasn't just for fun, it was on performance grounds.
> There's a lengthy discussion in [1] about not adding any new
> performance regressions to COPY. To be more specific, Peter complains
> about a regression of 5% in [2].
>
> It's pretty disheartening to see the work there being partially undone.
>
Whoops :-(
> Here are my performance tests of with and without your change to the
> memory contexts (I missed where you posted your results).
>
> $ cat bench.pl
> for (my $i=0; $i < 8912891; $i++) {
> print "1\n1\n2\n2\n";
> }
> 36a1281f86c: (with your change)
>
> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> COPY 35651564
> Time: 26825.142 ms (00:26.825)
> Time: 27202.117 ms (00:27.202)
> Time: 26266.705 ms (00:26.267)
>
> 4be058fe9ec: (before your change)
>
> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> COPY 35651564
> Time: 25645.460 ms (00:25.645)
> Time: 25698.193 ms (00:25.698)
> Time: 25737.251 ms (00:25.737)
>
> So looks like your change slows this code down 4% for this test case.
> That's about twice as bad as the 2.2% regression that I had to solve
> for the multi-insert partition patch (of which you've removed much of
> the code from)
>
> I'd really like to see the alternating batch context code being put
> back in to fix this. Of course, if you have a better idea, then we can
> look into that, but just removing code that was carefully written and
> benchmarked without any new benchmarks to prove that it's okay to do
> so seems wrong to me.
>
Yeah, that's not very nice. Do you suggest to revert 36a1281f86c0f, or
shall we try to massage it a bit first? ISTM we could simply create two
batch memory contexts and alternate those, just like with the expression
contexts before.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, david.rowley@2ndquadrant.com, andres@anarazel.de, surafel3000@gmail.com, alvherre@2ndquadrant.com, berlin.ab@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: COPY FROM WHEN condition
In-Reply-To: <b5fe1ea4-5007-2183-bd11-a8210cca3cd8@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox