Horton

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

Tomas Vondra <tomas.vondra@2ndquadrant.com>

View thread
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2019-10-22T17:12:26Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Tighten the concurrent abort check during decoding.

  2. Improve hash_create()'s API for some added robustness.

  3. Use HASH_BLOBS for xidhash.

  4. Fix initialization of RelationSyncEntry for streaming transactions.

  5. Remove unused function declaration in logicalproto.h.

  6. Add additional tests to test streaming of in-progress transactions.

  7. Fix inline marking introduced in commit 464824323e.

  8. Add support for streaming to built-in logical replication.

  9. Fix the SharedFileSetUnregister API.

  10. Fix comment in procarray.c

  11. Suppress compiler warning in non-cassert builds.

  12. Extend the BufFile interface.

  13. Mark a few logical decoding related variables with PGDLLIMPORT.

  14. Implement streaming mode in ReorderBuffer.

  15. Extend the logical decoding output plugin API with stream methods.

  16. WAL Log invalidations at command end with wal_level=logical.

  17. Immediately WAL-log subtransaction and top-level XID association.

  18. Allow logical replication to transfer data in binary format.

  19. Only superuser can set sslcert/sslkey in postgres_fdw user mappings

  20. Track statistics for spilling of changes from ReorderBuffer.

  21. Add logical_decoding_work_mem to limit ReorderBuffer memory usage.

  22. logical decoding: process ASSIGNMENT during snapshot build

  23. Emit invalidations to standby for transactions without xid.

On Tue, Oct 22, 2019 at 10:30:16AM +0530, Dilip Kumar wrote:
>On Fri, Oct 18, 2019 at 5:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Mon, Oct 14, 2019 at 3:09 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>> >
>> > On Thu, Oct 3, 2019 at 4:03 AM Tomas Vondra
>> > <tomas.vondra@2ndquadrant.com> wrote:
>> > >
>> > >
>> > > 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.
>> > >
>> > I have extracted the spilling related code to a separate patch on top
>> > of 0001.  I have also fixed some bugs and review comments and attached
>> > as a separate patch.  Later I can merge it to the main patch if you
>> > agree with the changes.
>> >
>>
>> Few comments
>> -------------------------
>> 0001-Add-logical_decoding_work_mem-to-limit-ReorderBuffer
>> 1.
>> + {
>> + {"logical_decoding_work_mem", PGC_USERSET, RESOURCES_MEM,
>> + gettext_noop("Sets the maximum memory to be used for logical decoding."),
>> + gettext_noop("This much memory can be used by each internal "
>> + "reorder buffer before spilling to disk or streaming."),
>> + GUC_UNIT_KB
>> + },
>>
>> I think we can remove 'or streaming' from above sentence for now.  We
>> can add it later with later patch where streaming will be allowed.
>Done
>>
>> 2.
>> @@ -206,6 +206,18 @@ CREATE SUBSCRIPTION <replaceable
>> class="parameter">subscription_name</replaceabl
>>           </para>
>>          </listitem>
>>         </varlistentry>
>> +
>> +       <varlistentry>
>> +        <term><literal>work_mem</literal> (<type>integer</type>)</term>
>> +        <listitem>
>> +         <para>
>> +          Limits the amount of memory used to decode changes on the
>> +          publisher.  If not specified, the publisher will use the default
>> +          specified by <varname>logical_decoding_work_mem</varname>. When
>> +          needed, additional data are spilled to disk.
>> +         </para>
>> +        </listitem>
>> +       </varlistentry>
>>
>> It is not clear why we need this parameter at least with this patch?
>> I have raised this multiple times [1][2].
>
>I have moved it out as a separate patch (0003) so that if we need that
>we need this for the streaming transaction then we can keep this.
>>

I'm OK with moving it to a separate patch. That being said I think
ability to control memory usage for individual subscriptions is very
useful. Saying "We don't need such parameter" is essentially equivalent
to saying "One size fits all" and I think we know that's not true.

Imagine a system with multiple subscriptions, some of them mostly
replicating OLTP changes, but one or two replicating tables that are
updated in batches. What we'd have is to allow higher limit for the
batch subscriptions, but much lower limit for the OLTP ones (which they
should never hit in practice).

With a single global GUC, you'll either have a high value - risking
OOM when the OLTP subscriptions happen to decode a batch update, or a
low value affecting the batch subscriotions.

It's not strictly necessary (and we already have such limit), so I'm OK
with treating it as an enhancement for the future.


regards

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