Andres Freund wrote:
> On 2014-07-04 18:31:34 +0800, gotoschool6g wrote:
> > Carp the code:
> >
> > const char *
> > clog_identify(uint8 info)
> > {
> > switch (info)
> > {
> > case CLOG_ZEROPAGE:
> > return "ZEROPAGE";
> > case CLOG_TRUNCATE:
> > return "TRUNCATE";
> > break;
> > }
> > return NULL;
> > }
I agree that performance is secondary here. The part that I don't quite
like in all these blocks is the fact that they initialize the return
value to NULL beforehand, and there's no 'default' label. Now, I see
that pg_xlogdump is checking for NULL return, but I'm not sure this is
the best possible API. Shouldn't we perhaps return a different string
that indicates there is no known description?
Also, are we certain that we're never going to need anything other than
the "info" to identify the record? In practice we seem to follow that
rule currently, but it's not totally out of the question that someday
the rm_redo function uses more than just "info" to identify the record.
I wonder if it'd be better to pass the whole record instead and have the
rm_identify function pull out the info if it's all it needs, but leave
the possibility open that it could read, say, some header bytes in the
record data.
Also I didn't check how you handle the "init" bit in RM_HEAP and
RM_HEAP2. Are we just saying that "insert (init)" is a different record
type from "insert"? Maybe that's okay, but I'm not 100% sure.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services