On Tue, Jan 12, 2016 at 4:57 AM, Simon Riggs <simon@2ndquadrant.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/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services