Re: Speedup twophase transactions

On 12 January 2016 at 06:35, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Tue, Jan 12, 2016 at 4:57 AM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> wrote:
> > Performance tests for me show that the patch is effective; my results
> match
> > Jesper's roughly in relative numbers.
> >
> > My robustness review is that the approach and implementation are safe.
> >
> > It's clear there are various additional tuning opportunities, but the
> > objective of the current patch to improve performance is very, very
> clearly
> > met, so I'm aiming to commit *this* patch soon.
>
> - /* initialize LSN to 0 (start of WAL) */
> - gxact->prepare_lsn = 0;
> + /* initialize LSN to InvalidXLogRecPtr */
> + gxact->prepare_start_lsn = InvalidXLogRecPtr;
> + gxact->prepare_end_lsn = InvalidXLogRecPtr;
>

OK

> I think that it would be better to explicitly initialize gxact->ondisk
> to false here.
>
> + xlogreader = XLogReaderAllocate(&logical_read_local_xlog_page,
> NULL);
> + if (!xlogreader)
> + ereport(ERROR,
> + (errcode(ERRCODE_OUT_OF_MEMORY),
> + errmsg("out of memory"),
> + errdetail("Failed while allocating an
> XLog reading processor.")));
> Depending on something that is part of logical decoding to decode WAL
> is not a good idea.

Well, if you put it like that, it sounds wrong, clearly; that's not how I
saw it, when reviewed.

I think any fuss can be avoided simply by renaming
logical_read_local_xlog_page() to read_local_xlog_page()

> If you want to move on with this approach, you
> should have a dedicated function.

The code is exactly what we need, apart from the point that the LSN is
always known flushed by the time we execute it, for 2PC.

> Even better, it would be nice to
> come up with a generic function used by both 2PC and logical decoding.
>

Surely that is exactly what has been done?

A specific function could have been written, which would simply have
duplicated about 160 lines of code. Reusing the existing code makes the
code generic. So lets just rename the function, as mentioned above.

Should we just move the code somewhere just to imply it is generic? Seems
pointless refactoring to me.

The code is clearly due for refactoring once we can share elog with client
programs, as described in comments on the functions.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services