Horton

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

Dilip Kumar <dilipbalaut@gmail.com>

View thread
From: Dilip Kumar <dilipbalaut@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: 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-05-22T12:52:08Z
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, May 19, 2020 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, May 19, 2020 at 3:31 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, May 19, 2020 at 2:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, May 18, 2020 at 5:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > >
> > > > 3.
> > > > + /*
> > > > + * If streaming is enable and we have serialized this transaction because
> > > > + * it had incomplete tuple.  So if now we have got the complete tuple we
> > > > + * can stream it.
> > > > + */
> > > > + if (ReorderBufferCanStream(rb) && can_stream && rbtxn_is_serialized(toptxn)
> > > > + && !(rbtxn_has_toast_insert(txn)) && !(rbtxn_has_spec_insert(txn)))
> > > > + {
> > > >
> > > > This comment is just saying what you are doing in the if-check.  I
> > > > think you need to explain the rationale behind it. I don't like the
> > > > variable name 'can_stream' because it matches ReorderBufferCanStream
> > > > whereas it is for a different purpose, how about naming it as
> > > > 'change_complete' or something like that.  The check has many
> > > > conditions, can we move it to a separate function to make the code
> > > > here look clean?
> > > >
> > >
> > > Do we really need this?  Immediately after this check, we are calling
> > > ReorderBufferCheckMemoryLimit which will anyway stream the changes if
> > > required.
> >
> > Actually, ReorderBufferCheckMemoryLimit is only meant for checking
> > whether we need to stream the changes due to the memory limit.  But
> > suppose when memory limit exceeds that time we could not stream the
> > transaction because there was only incomplete toast insert so we
> > serialized.  Now,  when we get the tuple which makes the changes
> > complete but now it is not crossing the memory limit as changes were
> > already serialized.  So I am not sure whether it is a good idea to
> > stream the transaction as soon as we get the complete changes or we
> > shall wait till next time memory limit exceed and that time we select
> > the suitable candidate.
> >
>
> I think it is better to wait till next time we exceed the memory threshold.

Okay, done this way.


> >  Ideally, we were are in streaming more and
> > the transaction is serialized means it was already a candidate for
> > streaming but could not stream due to the incomplete changes so
> > shouldn't we stream it immediately as soon as its changes are complete
> > even though now we are in memory limit.
> >
>
> The only time we need to stream or spill is when we exceed memory
> threshold.  In the above case, it is possible that next time there is
> some other candidate transaction that we can stream.
>
> > >
> > > Another comments on v20-0010-Bugfix-handling-of-incomplete-toast-tuple:
> > >
> > > + else if (rbtxn_has_toast_insert(txn) &&
> > > + ChangeIsInsertOrUpdate(change->action))
> > > + {
> > > + toptxn->txn_flags &= ~RBTXN_HAS_TOAST_INSERT;
> > > + can_stream = true;
> > > + }
> > > ..
> > > +#define ChangeIsInsertOrUpdate(action) \
> > > + (((action) == REORDER_BUFFER_CHANGE_INSERT) || \
> > > + ((action) == REORDER_BUFFER_CHANGE_UPDATE) || \
> > > + ((action) == REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT))
> > >
> > > How can we clear the RBTXN_HAS_TOAST_INSERT flag on
> > > REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT action?
> >
> > Partial toast insert means we have inserted in the toast but not in
> > the main table.  So even if it is spec insert we can form the complete
> > tuple, however, we can still not stream it because we haven't got
> > spec_confirm but for that, we are marking another flag.  So if the
> > insert is aspect insert the toast insert will also be spec insert and
> > as part of that toast, spec inserts we are marking partial tuple so
> > cleaning that flag should happen when the spec insert is done for the
> > main table right?
> >
>
> Sounds reasonable.

ok




--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com