Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions - Mailing list pgsql-hackers

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