Date: Tue, 1 Oct 2019 15:51:51 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: PATCH: logical_work_mem and logical streaming of large
 in-progress transactions
Message-ID: <20191001135151.swlspdnkxxnhszhn@development>
References: <688b0b7f-2f6c-d827-c27b-216a8e3ea700@2ndquadrant.com>
 <3530384f-f73a-186a-a728-8eeeb5533a10@2ndquadrant.com>
 <262cc9df-0d19-3fca-f899-0890bd2cd4be@2ndquadrant.com>
 <47e505c1-9a21-9edf-9a97-e13acce5aa21@2ndquadrant.com>
 <CAA4eK1Jz_8Sa8rHmUfJZgNpUUWqu9uqiyCmwAzqmGbRxyObgSg@mail.gmail.com>
 <20190927112520.3kco6tqbhcvlk6cd@development>
 <CAA4eK1J7NajKPE58Vnf5pBtwR3upxATUCRFoR04BPKbKBes5hw@mail.gmail.com>
 <20190928190917.hrpknmq76v3ts3lj@development>
 <CAA4eK1LCk4aAeTv8SofwgmKLaFakj+Sn4Y7MLEJORX0BjY2hGw@mail.gmail.com>
 <CAA4eK1+-F_ELz+n3S9bBN8kOgZ6ww67tOoy99f8Ug39RWXH20w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1+-F_ELz+n3S9bBN8kOgZ6ww67tOoy99f8Ug39RWXH20w@mail.gmail.com>
User-Agent: NeoMutt/20180716
Precedence: bulk

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.kapila16@gmail.com>
>wrote:
>> On Sun, Sep 29, 2019 at 12:39 AM Tomas Vondra
>> <tomas.vondra@2ndquadrant.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