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: Jeff Janes <jeff.janes@gmail.com>, 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-08-30T09:13:20Z
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.

Attachments

On Sat, Aug 29, 2020 at 5:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 28, 2020 at 2:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > As discussed, I have added a another test case for covering the out of
> > order subtransaction rollback scenario.
> >
>
> +# large (streamed) transaction with out of order subtransaction ROLLBACKs
> +$node_publisher->safe_psql('postgres', q{
>
> How about writing a comment as: "large (streamed) transaction with
> subscriber receiving out of order subtransaction ROLLBACKs"?

I have fixed and merged with 0002.

> I have reviewed and modified the number of things in the attached patch:
> 1. In apply_handle_origin, improved the check streamed xacts.
> 2. In apply_handle_stream_commit() while applying changes in the loop,
> added CHECK_FOR_INTERRUPTS.
> 3. In DEBUG messages, print the path with double-quotes as we are
> doing in all other places.
> 4.
> + /*
> + * Exit if streaming option is changed. The launcher will start new
> + * worker.
> + */
> + if (newsub->stream != MySubscription->stream)
> + {
> + ereport(LOG,
> + (errmsg("logical replication apply worker for subscription \"%s\" will "
> + "restart because subscription's streaming option were changed",
> + MySubscription->name)));
> +
> + proc_exit(0);
> + }
> +
> We don't need a separate check like this. I have merged this into one
> of the existing checks.
> 5.
> subxact_info_write()
> {
> ..
> + if (subxact_data.nsubxacts == 0)
> + {
> + if (ent->subxact_fileset)
> + {
> + cleanup_subxact_info();
> + BufFileDeleteShared(ent->subxact_fileset, path);
> + pfree(ent->subxact_fileset);
> + ent->subxact_fileset = NULL;
> + }
>
> I don't think it is right to use BufFileDeleteShared interface here
> because it won't perform SharedFileSetUnregister which means if after
> above code execution is the server exits it will crash in
> SharedFileSetDeleteOnProcExit which will try to access already deleted
> fileset entry. Fixed this by calling SharedFileSetDeleteAll() instead.
> The another related problem is that in function
> SharedFileSetDeleteOnProcExit, it tries to delete the list element
> while traversing the list with 'foreach' construct which makes the
> behavior of list traversal unpredictable. I have fixed this in a
> separate patch v54-0001-Fix-the-SharedFileSetUnregister-API, if you
> are fine with this, I would like to commit this as this fixes a
> problem in the existing commit 808e13b282.
> 6. Function stream_cleanup_files() contains a missing_ok argument
> which is not used so removed it.
> 7. In pgoutput.c, change the ordering of functions to make them
> consistent with their declaration.
> 8.
> typedef struct RelationSyncEntry
>  {
>   Oid relid; /* relation oid */
> + TransactionId xid; /* transaction that created the record */
>
> Removed above parameter as this doesn't seem to be required as per the
> new design in the patch.
>
> Apart from above, I have added/changed quite a few comments and a few
> other cosmetic changes. Kindly review and let me know what do you
> think about the changes?

I have reviewed your changes and look fine to me.  And the bug fix in
0001 also looks fine.

> One more comment for which I haven't done anything yet.
> +static void
> +set_schema_sent_in_streamed_txn(RelationSyncEntry *entry, TransactionId xid)
> +{
> + MemoryContext oldctx;
> +
> + oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> +
> + entry->streamed_txns = lappend_int(entry->streamed_txns, xid);

> Is it a good idea to append xid with lappend_int? Won't we need
> something equivalent for uint32? If so, I think we have a couple of
> options (a) use lcons method and accordingly append the pointer to
> xid, I think we need to allocate memory for xid if we want to use this
> idea or (b) use an array instead. What do you think?

BTW, OID is internally mapped to uint32,  but using lappend_oid might
not look good.  So maybe we can provide an option for lappend_uint32?
Using an array is also not a bad idea.  Providing lappend_uint32
option looks more appealing to me.

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