Subject: Re: PATCH: logical_work_mem and logical streaming of large
 in-progress transactions
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <688b0b7f-2f6c-d827-c27b-216a8e3ea700@2ndquadrant.com>
 <bbe1333f-17ba-1156-0713-7cc0bfe2d842@2ndquadrant.com>
 <bc83b847-2fc7-6561-4ef1-a3c9d154cc31@2ndquadrant.com>
 <47223121-222d-8e04-4928-a28ecdc5327d@2ndquadrant.com>
 <CAD21AoBa2mhbhZW6hnDRKs1ggycJOBE69eHk3KgCJ5Cwj9-ZkA@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <b9b306a3-c0a2-7eb9-92eb-a3ca111bd1bc@2ndquadrant.com>
Date: Thu, 1 Feb 2018 23:50:25 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: 
 <CAD21AoBa2mhbhZW6hnDRKs1ggycJOBE69eHk3KgCJ5Cwj9-ZkA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 01/31/2018 07:53 AM, Masahiko Sawada wrote:
> On Sat, Jan 20, 2018 at 7:08 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> On 01/19/2018 03:34 PM, Tomas Vondra wrote:
>>> Attached is v5, fixing a silly bug in part 0006, causing segfault when
>>> creating a subscription.
>>>
>>
>> Meh, there was a bug in the sgml docs (<variable> vs. <varname>),
>> causing another failure. Hopefully v6 will pass the CI build, it does
>> pass a build with the same parameters on my system.
> 
> Thank you for working on this. This patch would be helpful for
> synchronous replication.
> 
> I haven't looked at the code deeply yet, but I've reviewed the v6
> patch set especially on subscriber side. All of the patches can be
> applied to current HEAD cleanly. Here is review comment.
> 
> ----
> CREATE SUBSCRIPTION commands accept work_mem < 64 but it leads ERROR
> on publisher side when starting replication. Probably we should check
> the value on the subscriber side as well.
> 
> ----
> When streaming = on, if we drop subscription in the middle of
> receiving stream changes, DROP SUBSCRIPTION could leak tmp files
> (.chages file and .subxacts file). Also it also happens when a
> transaction on upstream aborted without abort record.
> 
> ----
> Since we can change both streaming option and work_mem option by ALTER
> SUBSCRIPTION, documentation of ALTER SUBSCRIPTION needs to be updated.
> 
> ----
> If we create a subscription without any options, both
> pg_subscription.substream and pg_subscription.subworkmem are set to
> null. However, since GetSubscription are't aware of NULL we start the
> replication with invalid options like follows.
> LOG:  received replication command: START_REPLICATION SLOT "hoge_sub"
> LOGICAL 0/0 (proto_version '2', work_mem '893219954', streaming 'on',
> publication_names '"hoge_pub"')
> 
> I think we can set substream to false and subworkmem to -1 instead of
> null, and then makes libpqrcv_startstreaming not set streaming option
> if stream is -1.
> 
> ----
> Some WARNING messages appeared. Maybe these are for debug purpose?
> 
> WARNING:  updating stream stats 0x1c12ef8 4 3 65604
> WARNING: UpdateSpillStats: updating stats 0x1c12ef8 0 0 0 39 41 2632080
> 
> Regards,
> 

Thanks for the review! I'll address the issues in the next version of
the patch.

regards

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