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

On Tue, Oct 01, 2019 at 06:55:52PM +0530, Amit Kapila wrote:
>On Sun, Sep 29, 2019 at 11:24 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>wrote:
>> On Sun, Sep 29, 2019 at 12:39 AM Tomas Vondra
>> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>> >
>>
>> Yeah, it is better to deal it separately as I am also not entirely
>> convinced at this stage about this parameter. I have mentioned the
>> same in the previous email as well.
>>
>> While glancing through the changes, I noticed a small thing:
>> +#logical_decoding_work_mem = 64MB # min 1MB, or -1 to use
>maintenance_work_mem
>>
>> I guess this need to be updated.
>>
>
>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.

>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 am reluctant to say, but I think this patch still needs some more work
>(review, test, rework) before we can commit it.
>

I agreee.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services