Improve code around the recently added rm_identify rmgr callback.

anarazel · anarazel · commit 604f7956b946 · 2014-09-22T17:49:34.000+02:00
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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ clog_identify(uint8 info)`
`37`	`37`	`{`
`38`	`38`	`const char *id = NULL;`
`39`	`39`
`40`		`- switch (info)`
	`40`	`+ switch (info & ~XLR_INFO_MASK)`
`41`	`41`	`{`
`42`	`42`	`case CLOG_ZEROPAGE:`
`43`	`43`	`id = "ZEROPAGE";`
Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,7 @@ dbase_identify(uint8 info)`
`46`	`46`	`{`
`47`	`47`	`const char *id = NULL;`
`48`	`48`
`49`		`- switch (info)`
	`49`	`+ switch (info & ~XLR_INFO_MASK)`
`50`	`50`	`{`
`51`	`51`	`case XLOG_DBASE_CREATE:`
`52`	`52`	`id = "CREATE";`
Original file line number	Diff line number	Diff line change
`@@ -181,7 +181,7 @@ gin_identify(uint8 info)`
`181`	`181`	`{`
`182`	`182`	`const char *id = NULL;`
`183`	`183`
`184`		`- switch (info)`
	`184`	`+ switch (info & ~XLR_INFO_MASK)`
`185`	`185`	`{`
`186`	`186`	`case XLOG_GIN_CREATE_INDEX:`
`187`	`187`	`id = "CREATE_INDEX";`
Original file line number	Diff line number	Diff line change
`@@ -69,7 +69,7 @@ gist_identify(uint8 info)`
`69`	`69`	`{`
`70`	`70`	`const char *id = NULL;`
`71`	`71`
`72`		`- switch (info)`
	`72`	`+ switch (info & ~XLR_INFO_MASK)`
`73`	`73`	`{`
`74`	`74`	`case XLOG_GIST_PAGE_UPDATE:`
`75`	`75`	`id = "PAGE_UPDATE";`
Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ multixact_identify(uint8 info)`
`77`	`77`	`{`
`78`	`78`	`const char *id = NULL;`
`79`	`79`
`80`		`- switch (info)`
	`80`	`+ switch (info & ~XLR_INFO_MASK)`
`81`	`81`	`{`
`82`	`82`	`case XLOG_MULTIXACT_ZERO_OFF_PAGE:`
`83`	`83`	`id = "ZERO_OFF_PAGE";`
Original file line number	Diff line number	Diff line change
`@@ -126,7 +126,7 @@ btree_identify(uint8 info)`
`126`	`126`	`{`
`127`	`127`	`const char *id = NULL;`
`128`	`128`
`129`		`- switch (info)`
	`129`	`+ switch (info & ~XLR_INFO_MASK)`
`130`	`130`	`{`
`131`	`131`	`case XLOG_BTREE_INSERT_LEAF:`
`132`	`132`	`id = "INSERT_LEAF";`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ relmap_identify(uint8 info)`
`36`	`36`	`{`
`37`	`37`	`const char *id = NULL;`
`38`	`38`
`39`		`- switch (info)`
	`39`	`+ switch (info & ~XLR_INFO_MASK)`
`40`	`40`	`{`
`41`	`41`	`case XLOG_RELMAP_UPDATE:`
`42`	`42`	`id = "UPDATE";`
Original file line number	Diff line number	Diff line change
`@@ -35,7 +35,7 @@ seq_identify(uint8 info)`
`35`	`35`	`{`
`36`	`36`	`const char *id = NULL;`
`37`	`37`
`38`		`- switch (info)`
	`38`	`+ switch (info & ~XLR_INFO_MASK)`
`39`	`39`	`{`
`40`	`40`	`case XLOG_SEQ_LOG:`
`41`	`41`	`id = "LOG";`