Re: log_newpage header comment

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