Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

I have attempted to test the performance of (Stream + Spill) vs
(Stream + BGW pool) and I can see the similar gain what Alexey had
shown[1].

In addition to this, I have rebased the latest patchset [2] without
the two-phase logical decoding patch set.

Test results:
I have repeated the same test as Alexy[1] for 1kk and 1kk data and
here is my result
Stream + Spill
N time on master(sec) Total xact time (sec)
1kk 6 21
3kk 18 55

Stream + BGW pool
N time on master(sec) Total xact time (sec)
1kk 6 13
3kk 19 35

Patch details:
All the patches are the same as posted on [2] except
1. 0006-Gracefully-handle-concurrent-aborts-of-uncommitted -> I have
removed the handling of error which is specific for 2PC
2. 0007-Implement-streaming-mode-in-ReorderBuffer -> Rebased without 2PC
3. 0009-Extend-the-concurrent-abort-handling-for-in-progress -> New
patch to handle concurrent abort error for the in-progress transaction
and also add handling for the sub transaction's abort.
4. v3-0014-BGWorkers-pool-for-streamed-transactions-apply -> Rebased
Alexey's patch

[1] https://www.postgresql.org/message-id/8eda5118-2dd0-79a1-4fe9-eec7e334de17%40postgrespro.ru
[2] https://www.postgresql.org/message-id/20190928190917.hrpknmq76v3ts3lj%40development

On Thu, Oct 3, 2019 at 4:03 AM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On Wed, Oct 02, 2019 at 04:27:30AM +0530, Amit Kapila wrote:
> >On Tue, Oct 1, 2019 at 7:21 PM Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
> >wrote:
> >
> >> On Tue, Oct 01, 2019 at 06:55:52PM +0530, Amit Kapila wrote:
> >> >
> >> >On further testing, I found that the patch seems to have problems with
> >> >toast. Consider below scenario:
> >> >Session-1
> >> >Create table large_text(t1 text);
> >> >INSERT INTO large_text
> >> >SELECT (SELECT string_agg('x', ',')
> >> >FROM generate_series(1, 1000000)) FROM generate_series(1, 1000);
> >> >
> >> >Session-2
> >> >SELECT * FROM pg_create_logical_replication_slot('regression_slot',
> >> >'test_decoding');
> >> >SELECT * FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL);
> >> >*--kaboom*
> >> >
> >> >The second statement in Session-2 leads to a crash.
> >> >
> >>
> >> OK, thanks for the report - will investigate.
> >>
> >
> >It was an assertion failure in ReorderBufferCleanupTXN at below line:
> >+ /* Check we're not mixing changes from different transactions. */
> >+ Assert(change->txn == txn);
> >
>
> Can you still reproduce this issue with the patch I sent on 28/9? I have
> been unable to trigger the failure, and it seems pretty similar to the
> failure you reported (and I fixed) on 28/9.
>
> >> >Other than that, I am not sure if the changes related to spill to disk
> >> >after logical_decoding_work_mem works for toast table as I couldn't hit
> >> >that code for toast table case, but I might be missing something. As
> >> >mentioned previously, I feel there should be some way to test whether this
> >> >patch works for the cases it claims to work. As of now, I have to check
> >> >via debugging. Let me know if there is any way, I can test this.
> >> >
> >>
> >> That's one of the reasons why I proposed to move the statistics (which
> >> say how many transactions / bytes were spilled to disk) from a later
> >> patch in the series. I don't think there's a better way.
> >>
> >>
> >I like that idea, but I think you need to split that patch to only get the
> >stats related to the spill. It would be easier to review if you can
> >prepare that atop of
> >0001-Add-logical_decoding_work_mem-to-limit-ReorderBuffer.
> >
>
> Sure, I wasn't really proposing to adding all stats from that patch,
> including those related to streaming. We need to extract just those
> related to spilling. And yes, it needs to be moved right after 0001.
>
> regards
>
> --
> Tomas Vondra http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com