Horton

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

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Erik Rijkers <er@xs4all.nl>, Kuntal Ghosh <kuntalghosh.2007@gmail.com>, Tomas Vondra <tomas.vondra@2ndquadrant.com>, Michael Paquier <michael@paquier.xyz>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2020-06-26T05:51:17Z
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 Fri, Jun 26, 2020 at 10:39 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jun 25, 2020 at 7:10 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Wed, Jun 24, 2020 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > Comments on other patches:
> > > =========================
> > > 5.
> > > > 3. On concurrent abort we are truncating all the changes including
> > > > some incomplete changes,  so later when we get the complete changes we
> > > > don't have the previous changes,  e.g, if we had specinsert in the
> > > > last stream and due to concurrent abort detection if we delete that
> > > > changes later we will get spec_confirm without spec insert.  We could
> > > > have simply avoided deleting all the changes, but I think the better
> > > > fix is once we detect the concurrent abort for any transaction, then
> > > > why do we need to collect the changes for that, we can simply avoid
> > > > that.  So I have put that fix. (0006)
> > > >
> > >
> > > On similar lines, I think we need to skip processing message, see else
> > > part of code in ReorderBufferQueueMessage.
> >
> > Basically, ReorderBufferQueueMessage also calls the
> > ReorderBufferQueueChange internally for transactional changes.

Yes, that is correct but I was thinking about the non-transactional
part due to the below code there.

else
{
ReorderBufferTXN *txn = NULL;
volatile Snapshot snapshot_now = snapshot;

if (xid != InvalidTransactionId)
txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);

Even though we are using txn here but I think we don't need to skip it
for aborted xacts because without patch as well such messages get
decoded irrespective of transaction status.  What do you think?

> >  But,
> > having said that, I realize the idea of skipping the changes in
> > ReorderBufferQueueChange is not good,  because by then we have already
> > allocated the memory for the change and the tuple and it's not a
> > correct to ReturnChanges because it will update the memory accounting.
> > So I think we can do it at a more centralized place and before we
> > process the change,  maybe in LogicalDecodingProcessRecord, before
> > going to the switch we can call a function from the reorderbuffer.c
> > layer to see whether this transaction is detected as aborted or not.
> > But I have to think more on this line that can we skip all the
> > processing of that record or not.
> >
> > Your other comments look fine to me so I will send in the next patch
> > set and reply on them individually.
>
> I think we can not put this check, in the higher-level functions like
> LogicalDecodingProcessRecord or DecodeXXXOp because we need to process
> that xid at least for abort,  so I think it is good to keep the check,
> inside ReorderBufferQueueChange only and we can free the memory of the
> change if the abort is detected.  Also, if just skip those changes in
> ReorderBufferQueueChange then the effect will be localized to that
> particular transaction which is already aborted.
>

Fair enough and for cases like non-transactional part of
ReorderBufferQueueMessage, I think we anyway need to process the
message irrespective of transaction status.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com