Re: Changing WAL Header to reduce contention during ReserveXLogInsertLocation()

On Tue, Mar 27, 2018 at 6:35 AM, Andrew Dunstan
<andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
> Leaving aside the arguments about process, the patch is pretty small
> and fairly straightforward. Given the claimed performance gains that's
> a nice bang for the buck.
>
> I haven't seen any obvious holes, but this is surely a case for as
> many eyeballs as possible.

Taking a look at this version, I think the key thing we have to decide
is whether we're comfortable with this:

--- a/src/include/access/xlogrecord.h
+++ b/src/include/access/xlogrecord.h
@@ -42,7 +42,7 @@ typedef struct XLogRecord
{
uint32 xl_tot_len; /* total len of entire record */
TransactionId xl_xid; /* xact id */
- XLogRecPtr xl_prev; /* ptr to previous record in log */
+ XLogRecPtr xl_curr; /* ptr to this record in log */
uint8 xl_info; /* flag bits, see below */
RmgrId xl_rmid; /* resource manager for this record */
/* 2 bytes of padding here, initialize to zero */

I don't see any comments in the patch explaining why this substitution
is just as safe as what we had before, and I think it has only very
briefly been commented upon by Pavan, who remarked that it provided
similar protection to what we have today. That's fair enough, but I
think a little more analysis of this point would be good. Can we
think of any possible downsides to making this change? I think there
are basically two issues:

1. Does it materially increase the chance of a bogus checksum match in
any plausible situation?

2. Does the new logic in pg_rewind to search backward for a checkpoint
work reliably, and will it be slow?

I don't know of a problem in either regard, but I wonder if anyone
else can think of anything.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company