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

From: Dilip Kumar <dilipbalaut(at)gmail(dot)com>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Erik Rijkers <er(at)xs4all(dot)nl>, Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
Date: 2020-07-05 11:05:08
Message-ID: CAFiTN-sq2dgBB2S6s0D6LcJKqwbkE9iihnSEvZNoq-RVzCtTKw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2020-07-05 11:17:03 Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
Previous Message Peter Eisentraut 2020-07-05 10:20:21 Re: suggest to rename enable_incrementalsort