Re: log_newpage header comment

On Sat, Jun 9, 2012 at 10:49 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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.

That's a good thing, because if they were, it would be wrong.
RelationNeedsWAL() tells you whether the main fork is WAL-logged, but
the buildempty routines exist for the purpose of constructing the init
fork, which should always be WAL-logged.

> 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.)

XLogIsNeeded() is, indeed, not a very good name: it's telling us
whether wal_level > minimal, and thus whether there might be a
standby. When log_newpage() is used to rebuild a relation, we use WAL
bypass whenever possible, since the whole relation will be removed if
the transaction rolls back, but we must still log it if a standby
exists. In contrast, the init forks need to make it to the standby
regardless. I'm not sure that I agree that it's the job of
log_newpage() to explain to people calling it on what things they must
conditionalize WAL generation: after all, none of this is unique to
new-page records. If we emit some other record while creating an
index on the primary, we can skip that when !XLogIsNeeded(), too. Any
operation we perform on any relation fork other than the init fork can
be conditionalized on RelationNeedsWAL(), not just new-page records.
The charter of the function is just to avoid duplicating the
mumbo-jumbo that's required to emit the record.

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