Re: [COMMITTERS] pgsql: Move WAL continuation record information to WAL page header.

Move WAL continuation record information to WAL page header.

The continuation record only contained one field, xl_rem_len, so it makes
things simpler to just include it in the WAL page header. This wastes four
bytes on pages that don't begin with a continuation from previos page, plus
four bytes on every page, because of padding.

The motivation of this is to make it easier to calculate how much space a
WAL record needs. Before this patch, it depended on how many page boundaries
the record crosses. The motivation of that, in turn, is to separate the
allocation of space in the WAL from the copying of the record data to the
allocated space. Keeping the calculation of space required simple helps to
keep the critical section of allocating the space from WAL short. But that's
not included in this patch yet.

Bump WAL version number again, as this is an incompatible change.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/20ba5ca64c7c858400f845f8ded330604e2c8d61

Modified Files
--------------
src/backend/access/transam/xlog.c | 31 ++++++++++++++-----------------
src/include/access/xlog_internal.h | 35 ++++++++++++++---------------------
2 files changed, 28 insertions(+), 38 deletions(-)

On 27.06.2012 17:53, Andres Freund wrote:
> I had noticed one thing when reviewing the patches before:
>
> @@ -717,6 +719,15 @@ XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
> bool doPageWrites;
> bool isLogSwitch = (rmid == RM_XLOG_ID&& info == XLOG_SWITCH);
> uint8 info_orig = info;
> + static XLogRecord *rechdr;
> +
> + if (rechdr == NULL)
> + {
> + rechdr = malloc(SizeOfXLogRecord);
> + if (rechdr == NULL)
> + elog(ERROR, "out of memory");
> + MemSet(rechdr, 0, SizeOfXLogRecord);
> + }
>
> /* cross-check on whether we should be here or not */
> if (!XLogInsertAllowed())
>
> Why do you allocate this dynamically? XLogRecord is 32bytes, there doesn't
> seem to be much point in this?

On 64-bit architectures, the struct needs padding at the end to make the
size MAXALIGNed to 32 bytes; a simple "XLogRecord rechdr" local variable
would not include that. You could do something like:

union
{
XLogRecord rechdr;
char bytes[SizeOfXLogRecord];
}

but that's quite ugly too.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Wednesday, June 27, 2012 05:09:46 PM Heikki Linnakangas wrote:
> On 27.06.2012 17:53, Andres Freund wrote:
> > I had noticed one thing when reviewing the patches before:
> >
> > @@ -717,6 +719,15 @@ XLogInsert(RmgrId rmid, uint8 info, XLogRecData
> > *rdata)
> >
> > bool doPageWrites;
> > bool isLogSwitch = (rmid == RM_XLOG_ID&& info ==
> > XLOG_SWITCH); uint8 info_orig = info;
> >
> > + static XLogRecord *rechdr;
> > +
> > + if (rechdr == NULL)
> > + {
> > + rechdr = malloc(SizeOfXLogRecord);
> > + if (rechdr == NULL)
> > + elog(ERROR, "out of memory");
> > + MemSet(rechdr, 0, SizeOfXLogRecord);
> > + }
> >
> > /* cross-check on whether we should be here or not */
> > if (!XLogInsertAllowed())
> >
> > Why do you allocate this dynamically? XLogRecord is 32bytes, there
> > doesn't seem to be much point in this?
>
> On 64-bit architectures, the struct needs padding at the end to make the
> size MAXALIGNed to 32 bytes; a simple "XLogRecord rechdr" local variable
> would not include that. You could do something like:
>
> union
> {
> XLogRecord rechdr;
> char bytes[SizeOfXLogRecord];
> }
>
> but that's quite ugly too.
Ah, yes. Makes sense.

Btw, the XLogRecord struct is directly layed out with 32bytes here (64bit,
linux) because xl_prev needs to be 8byte aligned...

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services