From 604f7956b9460192222dd37bd3baea24cb669a47 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 22 Sep 2014 16:48:14 +0200 Subject: [PATCH] Improve code around the recently added rm_identify rmgr callback. There are four weaknesses in728f152e07f998d2cb4fe5f24ec8da2c3bda98f2: * append_init() in heapdesc.c was ugly and required that rm_identify return values are only valid till the next call. Instead just add a couple more switch() cases for the INIT_PAGE cases. Now the returned value will always be valid. * a couple rm_identify() callbacks missed masking xl_info with ~XLR_INFO_MASK. * pg_xlogdump didn't map a NULL rm_identify to UNKNOWN or a similar string. * append_init() was called when id=NULL - which should never actually happen. But it's better to be careful. --- contrib/pg_xlogdump/pg_xlogdump.c | 7 ++++- src/backend/access/rmgrdesc/clogdesc.c | 2 +- src/backend/access/rmgrdesc/dbasedesc.c | 2 +- src/backend/access/rmgrdesc/gindesc.c | 2 +- src/backend/access/rmgrdesc/gistdesc.c | 2 +- src/backend/access/rmgrdesc/heapdesc.c | 33 ++++++++++------------- src/backend/access/rmgrdesc/mxactdesc.c | 2 +- src/backend/access/rmgrdesc/nbtdesc.c | 2 +- src/backend/access/rmgrdesc/relmapdesc.c | 2 +- src/backend/access/rmgrdesc/seqdesc.c | 2 +- src/backend/access/rmgrdesc/smgrdesc.c | 2 +- src/backend/access/rmgrdesc/spgdesc.c | 2 +- src/backend/access/rmgrdesc/standbydesc.c | 2 +- src/backend/access/rmgrdesc/tblspcdesc.c | 2 +- src/backend/access/rmgrdesc/xactdesc.c | 2 +- src/backend/access/rmgrdesc/xlogdesc.c | 2 +- src/backend/access/transam/xlog.c | 3 ++- src/include/access/xlog_internal.h | 3 --- 18 files changed, 36 insertions(+), 38 deletions(-) diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c index 1cd554ac4f..adc9087a1d 100644 --- a/contrib/pg_xlogdump/pg_xlogdump.c +++ b/contrib/pg_xlogdump/pg_xlogdump.c @@ -382,8 +382,13 @@ XLogDumpCountRecord(XLogDumpConfig *config, XLogDumpStats *stats, XLogRecPtr Rea static void XLogDumpDisplayRecord(XLogDumpConfig *config, XLogRecPtr ReadRecPtr, XLogRecord *record) { + const char *id; const RmgrDescData *desc = &RmgrDescTable[record->xl_rmid]; + id = desc->rm_identify(record->xl_info); + if (id == NULL) + id = psprintf("UNKNOWN (%x)", record->xl_info & ~XLR_INFO_MASK); + printf("rmgr: %-11s len (rec/tot): %6u/%6u, tx: %10u, lsn: %X/%08X, prev %X/%08X, bkp: %u%u%u%u, desc: %s ", desc->rm_name, record->xl_len, record->xl_tot_len, @@ -394,7 +399,7 @@ XLogDumpDisplayRecord(XLogDumpConfig *config, XLogRecPtr ReadRecPtr, XLogRecord !!(XLR_BKP_BLOCK(1) & record->xl_info), !!(XLR_BKP_BLOCK(2) & record->xl_info), !!(XLR_BKP_BLOCK(3) & record->xl_info), - desc->rm_identify(record->xl_info)); + id); /* the desc routine will printf the description directly to stdout */ desc->rm_desc(NULL, record); diff --git a/src/backend/access/rmgrdesc/clogdesc.c b/src/backend/access/rmgrdesc/clogdesc.c index 8beb6d027a..4a12e286e4 100644 --- a/src/backend/access/rmgrdesc/clogdesc.c +++ b/src/backend/access/rmgrdesc/clogdesc.c @@ -37,7 +37,7 @@ clog_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case CLOG_ZEROPAGE: id = "ZEROPAGE"; diff --git a/src/backend/access/rmgrdesc/dbasedesc.c b/src/backend/access/rmgrdesc/dbasedesc.c index e36988a736..446e5f97f4 100644 --- a/src/backend/access/rmgrdesc/dbasedesc.c +++ b/src/backend/access/rmgrdesc/dbasedesc.c @@ -46,7 +46,7 @@ dbase_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_DBASE_CREATE: id = "CREATE"; diff --git a/src/backend/access/rmgrdesc/gindesc.c b/src/backend/access/rmgrdesc/gindesc.c index dd4c791864..2f783cee2b 100644 --- a/src/backend/access/rmgrdesc/gindesc.c +++ b/src/backend/access/rmgrdesc/gindesc.c @@ -181,7 +181,7 @@ gin_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_GIN_CREATE_INDEX: id = "CREATE_INDEX"; diff --git a/src/backend/access/rmgrdesc/gistdesc.c b/src/backend/access/rmgrdesc/gistdesc.c index f2ed1f672b..db3ba13ccd 100644 --- a/src/backend/access/rmgrdesc/gistdesc.c +++ b/src/backend/access/rmgrdesc/gistdesc.c @@ -69,7 +69,7 @@ gist_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_GIST_PAGE_UPDATE: id = "PAGE_UPDATE"; diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c index cd9d59d6af..ee2c073f71 100644 --- a/src/backend/access/rmgrdesc/heapdesc.c +++ b/src/backend/access/rmgrdesc/heapdesc.c @@ -166,36 +166,34 @@ heap2_desc(StringInfo buf, XLogRecord *record) } } -static const char * -append_init(const char *str) -{ - static char x[32]; - - strcpy(x, str); - strcat(x, "+INIT"); - - return x; -} - const char * heap_identify(uint8 info) { const char *id = NULL; - switch (info & XLOG_HEAP_OPMASK) + switch (info & ~XLR_INFO_MASK) { case XLOG_HEAP_INSERT: id = "INSERT"; break; + case XLOG_HEAP_INSERT | XLOG_HEAP_INIT_PAGE: + id = "INSERT+INIT"; + break; case XLOG_HEAP_DELETE: id = "DELETE"; break; case XLOG_HEAP_UPDATE: id = "UPDATE"; break; + case XLOG_HEAP_UPDATE | XLOG_HEAP_INIT_PAGE: + id = "UPDATE+INIT"; + break; case XLOG_HEAP_HOT_UPDATE: id = "HOT_UPDATE"; break; + case XLOG_HEAP_HOT_UPDATE | XLOG_HEAP_INIT_PAGE: + id = "HOT_UPDATE+INIT"; + break; case XLOG_HEAP_LOCK: id = "LOCK"; break; @@ -204,9 +202,6 @@ heap_identify(uint8 info) break; } - if (info & XLOG_HEAP_INIT_PAGE) - id = append_init(id); - return id; } @@ -215,7 +210,7 @@ heap2_identify(uint8 info) { const char *id = NULL; - switch (info & XLOG_HEAP_OPMASK) + switch (info & ~XLR_INFO_MASK) { case XLOG_HEAP2_CLEAN: id = "CLEAN"; @@ -232,6 +227,9 @@ heap2_identify(uint8 info) case XLOG_HEAP2_MULTI_INSERT: id = "MULTI_INSERT"; break; + case XLOG_HEAP2_MULTI_INSERT | XLOG_HEAP_INIT_PAGE: + id = "MULTI_INSERT+INIT"; + break; case XLOG_HEAP2_LOCK_UPDATED: id = "LOCK_UPDATED"; break; @@ -243,8 +241,5 @@ heap2_identify(uint8 info) break; } - if (info & XLOG_HEAP_INIT_PAGE) - id = append_init(id); - return id; } diff --git a/src/backend/access/rmgrdesc/mxactdesc.c b/src/backend/access/rmgrdesc/mxactdesc.c index 177aebea07..afc5aca197 100644 --- a/src/backend/access/rmgrdesc/mxactdesc.c +++ b/src/backend/access/rmgrdesc/mxactdesc.c @@ -77,7 +77,7 @@ multixact_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_MULTIXACT_ZERO_OFF_PAGE: id = "ZERO_OFF_PAGE"; diff --git a/src/backend/access/rmgrdesc/nbtdesc.c b/src/backend/access/rmgrdesc/nbtdesc.c index 7eb3bbd847..8b63f2b6ba 100644 --- a/src/backend/access/rmgrdesc/nbtdesc.c +++ b/src/backend/access/rmgrdesc/nbtdesc.c @@ -126,7 +126,7 @@ btree_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_BTREE_INSERT_LEAF: id = "INSERT_LEAF"; diff --git a/src/backend/access/rmgrdesc/relmapdesc.c b/src/backend/access/rmgrdesc/relmapdesc.c index 39dcfb5003..ef7c533fe5 100644 --- a/src/backend/access/rmgrdesc/relmapdesc.c +++ b/src/backend/access/rmgrdesc/relmapdesc.c @@ -36,7 +36,7 @@ relmap_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_RELMAP_UPDATE: id = "UPDATE"; diff --git a/src/backend/access/rmgrdesc/seqdesc.c b/src/backend/access/rmgrdesc/seqdesc.c index d44fe62beb..73de3969df 100644 --- a/src/backend/access/rmgrdesc/seqdesc.c +++ b/src/backend/access/rmgrdesc/seqdesc.c @@ -35,7 +35,7 @@ seq_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_SEQ_LOG: id = "LOG"; diff --git a/src/backend/access/rmgrdesc/smgrdesc.c b/src/backend/access/rmgrdesc/smgrdesc.c index ee711bcacc..109e3eaf04 100644 --- a/src/backend/access/rmgrdesc/smgrdesc.c +++ b/src/backend/access/rmgrdesc/smgrdesc.c @@ -47,7 +47,7 @@ smgr_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_SMGR_CREATE: id = "CREATE"; diff --git a/src/backend/access/rmgrdesc/spgdesc.c b/src/backend/access/rmgrdesc/spgdesc.c index 5b41748385..3ee0427dcb 100644 --- a/src/backend/access/rmgrdesc/spgdesc.c +++ b/src/backend/access/rmgrdesc/spgdesc.c @@ -90,7 +90,7 @@ spg_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_SPGIST_CREATE_INDEX: id = "CREATE_INDEX"; diff --git a/src/backend/access/rmgrdesc/standbydesc.c b/src/backend/access/rmgrdesc/standbydesc.c index e480687558..d09041f8df 100644 --- a/src/backend/access/rmgrdesc/standbydesc.c +++ b/src/backend/access/rmgrdesc/standbydesc.c @@ -65,7 +65,7 @@ standby_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_STANDBY_LOCK: id = "LOCK"; diff --git a/src/backend/access/rmgrdesc/tblspcdesc.c b/src/backend/access/rmgrdesc/tblspcdesc.c index effeaf6680..b6b0e6394d 100644 --- a/src/backend/access/rmgrdesc/tblspcdesc.c +++ b/src/backend/access/rmgrdesc/tblspcdesc.c @@ -42,7 +42,7 @@ tblspc_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_TBLSPC_CREATE: id = "CREATE"; diff --git a/src/backend/access/rmgrdesc/xactdesc.c b/src/backend/access/rmgrdesc/xactdesc.c index 11e64af506..22a22efc73 100644 --- a/src/backend/access/rmgrdesc/xactdesc.c +++ b/src/backend/access/rmgrdesc/xactdesc.c @@ -193,7 +193,7 @@ xact_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_XACT_COMMIT: id = "COMMIT"; diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c index cdefaf57da..e0957ff3a8 100644 --- a/src/backend/access/rmgrdesc/xlogdesc.c +++ b/src/backend/access/rmgrdesc/xlogdesc.c @@ -139,7 +139,7 @@ xlog_identify(uint8 info) { const char *id = NULL; - switch (info) + switch (info & ~XLR_INFO_MASK) { case XLOG_CHECKPOINT_SHUTDOWN: id = "CHECKPOINT_SHUTDOWN"; diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 21f0052f68..544d76e852 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -9642,7 +9642,8 @@ xlog_outdesc(StringInfo buf, RmgrId rmid, XLogRecord *record) id = RmgrTable[rmid].rm_identify(record->xl_info); if (id == NULL) - appendStringInfo(buf, "UNKNOWN (%X): ", record->xl_info); + appendStringInfo(buf, "UNKNOWN (%X): ", + record->xl_info & ~XLR_INFO_MASK); else appendStringInfo(buf, "%s: ", id); diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index a1452b82b8..27b9899555 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -244,9 +244,6 @@ struct XLogRecord; * "VACUUM". rm_desc can then be called to obtain additional detail for the * record, if available (e.g. the last block). * - * The return value from rm_identify is a pointer to a statically allocated - * buffer, and only valid until the next invocation of the callback. - * * RmgrTable[] is indexed by RmgrId values (see rmgrlist.h). */ typedef struct RmgrData -- 2.40.0