Re: log_newpage header comment

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: log_newpage header comment
Date: 2012-06-09 14:49:44
Message-ID: 28146.1339253384@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Whee, testing is fun. Second try.

I'm concerned by the fact that neither the original nor the new code
bother to test whether the relation is WAL-loggable. It may be that
ginbuildempty cannot be invoked for temp tables, but it still seems
like an oversight waiting to bite you. I'd be happier if there were
a RelationNeedsWAL test here. Come to think of it, the other
foobuildempty functions aren't checking this either.

A related point is that most of the other existing calls to log_newpage
are covered by "if (XLogIsNeeded())" tests. It's not immediately
obvious why these two shouldn't be. After some reflection I think
that's correct, but probably the comments for log_newpage and
log_newpage_buffer need to explain the different WAL-is-needed tests
that apply to the two usages.

(I'm also thinking that the XLogIsNeeded macro is very poorly named,
as the situations it should be used in are far more narrow than the
macro name suggests. Haven't consumed enough caffeine to think of
a better name, though.)

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Simon Riggs 2012-06-09 15:36:38 pgsql: Add ERROR msg for GLOBAL/LOCAL TEMP is not yet implemented
Previous Message Tom Lane 2012-06-09 12:51:10 Re: pg_basebackup blocking all queries with horrible performance