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

On Tue, Jun 30, 2020 at 5:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jun 30, 2020 at 10:13 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Tue, Jun 30, 2020 at 9:20 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > Can't we name the last parameter as 'commit_lsn' as that is how
> > > documentation in the patch spells it and it sounds more appropriate?
> >
> > You are right commit_lsn seems more appropriate here.
> >
> > > Also, is there a reason for assigning report_location and
> > > write_location differently than what we do in commit_cb_wrapper?
> > > Basically, assign those as txn->final_lsn and txn->end_lsn
> > > respectively.
> >
> > Yes, I think it should be handled in same way as commit_cb_wrapper.
> > Because before calling ReorderBufferStreamCommit in
> > ReorderBufferCommit, we are properly updating the final_lsn as well as
> > the end_lsn.
> >
>
> Okay, I have made these changes in the attached patch and there are
> few more changes in
> 0003-Extend-the-output-plugin-API-with-stream-methods.
> 1. In pg_decode_stream_message, for transactional messages, we were
> displaying message contents which is different from other streaming
> APIs. I have changed it so that streaming API doesn't display message
> contents for transactional messages.

Ok, make sense.

> 2.
> + /* in streaming mode, stream_change_cb is required */
> + if (ctx->callbacks.stream_change_cb == NULL)
> + ereport(ERROR,
> + (errmsg("Output plugin supports streaming, but has not registered "
> + "stream_change_cb callback.")));
>
> The error messages seem a bit weird. (a) doesn't include error code,
> (b) not in PG style. I have changed all the error messages to fix
> these two issues and change the message as well

ok

> 3. Rearranged the functions stream_* so that the optional functions
> are at the end and also arranged other functions in a way that looks
> more logical to me.

Make sense to me.

> 4. Updated comments, commit message, and edited docs in the patch.
>
> I have made a few changes in
> 0004-Gracefully-handle-concurrent-aborts-of-transacti as well.
> 1. The variable bsysscan was not being reset in case of error. I have
> introduced a new function to reset both bsysscan and CheckXidAlive
> during transaction abort. Also, snapmgr.c doesn't seem right place
> for these variables, so I moved them to xact.c. I think this will
> make the initialization of CheckXidAlive during catch in
> ReorderBufferProcessTXN redundant.

That looks better.

> 2. Updated comments and commit message.
>
> Let me know what you think about the above changes.

All the above changes look good to me and I will include in the next version.

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