From 06865859f3c5da8d735f52719ec0ac0632322be1 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Sun, 9 Apr 2023 18:14:27 -0700 Subject: [PATCH v1 1/2] Fix heapdesc infomask array output. --- src/include/access/rmgrdesc_utils.h | 38 +++++++ src/backend/access/rmgrdesc/heapdesc.c | 105 +++++++++++++------ src/backend/access/rmgrdesc/rmgrdesc_utils.c | 20 ---- 3 files changed, 112 insertions(+), 51 deletions(-) diff --git a/src/include/access/rmgrdesc_utils.h b/src/include/access/rmgrdesc_utils.h index 00e614649..16916e3e8 100644 --- a/src/include/access/rmgrdesc_utils.h +++ b/src/include/access/rmgrdesc_utils.h @@ -12,6 +12,44 @@ #ifndef RMGRDESC_UTILS_H_ #define RMGRDESC_UTILS_H_ +/* + * Guidelines for rmgr descriptor routine authors: + * + * The goal of these guidelines is to avoid gratuitous inconsistencies across + * each rmgr, and to allow users to parse desc output strings without too much + * difficulty. This is not an API specification or an interchange format. + * (Only heapam and nbtree desc routines follow these guidelines at present, + * in any case.) + * + * Record descriptions are similar to JSON style key/value objects. However, + * there is no explicit "string" type/string escaping. Top-level { } brackets + * should be omitted. For example: + * + * snapshotConflictHorizon: 0, flags: 0x03 + * + * Record descriptions may contain variable-length arrays. For example: + * + * nunused: 5, unused: [1, 2, 3, 4, 5] + * + * Nested objects are supported via { } brackets. They generally appear + * inside variable-length arrays. For example: + * + * ndeleted: 0, nupdated: 1, deleted: [], updated: [{ off: 45, nptids: 1, ptids: [0] }] + * + * Try to output things in an order that faithfully represents the order of + * things in the physical WAL record struct. It's a good idea if the number + * of items in the array appears before the array. + * + * It's okay for individual WAL record types to invent their own conventions. + * For example, heapam's PRUNE records output the follow representation of + * redirects: + * + * ... redirected: [39->46], ... + * + * Arguably the PRUNE desc routine should be using object notation instead. + * This ad-hoc representation of redirects has the advantage of being terse in + * a context where that might matter a lot. + */ extern void array_desc(StringInfo buf, void *array, size_t elem_size, int count, void (*elem_desc) (StringInfo buf, void *elem, void *data), void *data); diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c index 3bd083875..a64d14c2c 100644 --- a/src/backend/access/rmgrdesc/heapdesc.c +++ b/src/backend/access/rmgrdesc/heapdesc.c @@ -18,29 +18,75 @@ #include "access/rmgrdesc_utils.h" static void -out_infobits(StringInfo buf, uint8 infobits) +infobits_desc(StringInfo buf, uint8 infobits, const char *keyname) { - if ((infobits & XLHL_XMAX_IS_MULTI) == 0 && - (infobits & XLHL_XMAX_LOCK_ONLY) == 0 && - (infobits & XLHL_XMAX_EXCL_LOCK) == 0 && - (infobits & XLHL_XMAX_KEYSHR_LOCK) == 0 && - (infobits & XLHL_KEYS_UPDATED) == 0) - return; + bool first = true; - appendStringInfoString(buf, ", infobits: ["); + appendStringInfo(buf, "%s: [", keyname); if (infobits & XLHL_XMAX_IS_MULTI) - appendStringInfoString(buf, " IS_MULTI"); + { + appendStringInfoString(buf, "IS_MULTI"); + first = false; + } if (infobits & XLHL_XMAX_LOCK_ONLY) - appendStringInfoString(buf, ", LOCK_ONLY"); + { + if (first) + appendStringInfoString(buf, "LOCK_ONLY"); + else + appendStringInfoString(buf, ", LOCK_ONLY"); + first = false; + } if (infobits & XLHL_XMAX_EXCL_LOCK) - appendStringInfoString(buf, ", EXCL_LOCK"); + { + if (first) + appendStringInfoString(buf, "EXCL_LOCK"); + else + appendStringInfoString(buf, ", EXCL_LOCK"); + first = false; + } if (infobits & XLHL_XMAX_KEYSHR_LOCK) - appendStringInfoString(buf, ", KEYSHR_LOCK"); + { + if (first) + appendStringInfoString(buf, "KEYSHR_LOCK"); + else + appendStringInfoString(buf, ", KEYSHR_LOCK"); + first = false; + } if (infobits & XLHL_KEYS_UPDATED) - appendStringInfoString(buf, ", KEYS_UPDATED"); + { + if (first) + appendStringInfoString(buf, "KEYS_UPDATED"); + else + appendStringInfoString(buf, ", KEYS_UPDATED"); + first = false; + } - appendStringInfoString(buf, " ]"); + appendStringInfoString(buf, "]"); +} + +static void +truncate_flags_desc(StringInfo buf, uint8 flags) +{ + bool first = true; + + appendStringInfoString(buf, "flags: ["); + + if (flags & XLH_TRUNCATE_CASCADE) + { + appendStringInfoString(buf, "CASCADE"); + first = false; + } + if (flags & XLH_TRUNCATE_RESTART_SEQS) + { + if (first) + appendStringInfoString(buf, "RESTART_SEQS"); + else + appendStringInfoString(buf, ", RESTART_SEQS"); + first = false; + } + + appendStringInfoString(buf, "]"); } static void @@ -82,20 +128,20 @@ heap_desc(StringInfo buf, XLogReaderState *record) { xl_heap_delete *xlrec = (xl_heap_delete *) rec; - appendStringInfo(buf, "off: %u, flags: 0x%02X", + appendStringInfo(buf, "off: %u, flags: 0x%02X, ", xlrec->offnum, xlrec->flags); - out_infobits(buf, xlrec->infobits_set); + infobits_desc(buf, xlrec->infobits_set, "infobits"); } else if (info == XLOG_HEAP_UPDATE) { xl_heap_update *xlrec = (xl_heap_update *) rec; - appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X", + appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X, ", xlrec->old_offnum, xlrec->old_xmax, xlrec->flags); - out_infobits(buf, xlrec->old_infobits_set); + infobits_desc(buf, xlrec->old_infobits_set, "old_infobits"); appendStringInfo(buf, ", new off: %u, xmax %u", xlrec->new_offnum, xlrec->new_xmax); @@ -104,11 +150,11 @@ heap_desc(StringInfo buf, XLogReaderState *record) { xl_heap_update *xlrec = (xl_heap_update *) rec; - appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X", + appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X, ", xlrec->old_offnum, xlrec->old_xmax, xlrec->flags); - out_infobits(buf, xlrec->old_infobits_set); + infobits_desc(buf, xlrec->old_infobits_set, "old_infobits"); appendStringInfo(buf, ", new off: %u, xmax: %u", xlrec->new_offnum, xlrec->new_xmax); @@ -117,12 +163,7 @@ heap_desc(StringInfo buf, XLogReaderState *record) { xl_heap_truncate *xlrec = (xl_heap_truncate *) rec; - appendStringInfoString(buf, "flags: ["); - if (xlrec->flags & XLH_TRUNCATE_CASCADE) - appendStringInfoString(buf, " CASCADE"); - if (xlrec->flags & XLH_TRUNCATE_RESTART_SEQS) - appendStringInfoString(buf, ", RESTART_SEQS"); - appendStringInfoString(buf, " ]"); + truncate_flags_desc(buf, xlrec->flags); appendStringInfo(buf, ", nrelids: %u", xlrec->nrelids); appendStringInfoString(buf, ", relids:"); @@ -139,9 +180,9 @@ heap_desc(StringInfo buf, XLogReaderState *record) { xl_heap_lock *xlrec = (xl_heap_lock *) rec; - appendStringInfo(buf, "off: %u, xid: %u, flags: 0x%02X", + appendStringInfo(buf, "off: %u, xid: %u, flags: 0x%02X, ", xlrec->offnum, xlrec->locking_xid, xlrec->flags); - out_infobits(buf, xlrec->infobits_set); + infobits_desc(buf, xlrec->infobits_set, "infobits"); } else if (info == XLOG_HEAP_INPLACE) { @@ -230,7 +271,9 @@ heap2_desc(StringInfo buf, XLogReaderState *record) OffsetNumber *offsets; plans = (xl_heap_freeze_plan *) XLogRecGetBlockData(record, 0, NULL); - offsets = (OffsetNumber *) &plans[xlrec->nplans]; + offsets = (OffsetNumber *) ((char *) plans + + (xlrec->nplans * + sizeof(xl_heap_freeze_plan))); appendStringInfoString(buf, ", plans:"); array_desc(buf, plans, sizeof(xl_heap_freeze_plan), xlrec->nplans, &plan_elem_desc, &offsets); @@ -260,9 +303,9 @@ heap2_desc(StringInfo buf, XLogReaderState *record) { xl_heap_lock_updated *xlrec = (xl_heap_lock_updated *) rec; - appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X", + appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X, ", xlrec->offnum, xlrec->xmax, xlrec->flags); - out_infobits(buf, xlrec->infobits_set); + infobits_desc(buf, xlrec->infobits_set, "infobits"); } else if (info == XLOG_HEAP2_NEW_CID) { diff --git a/src/backend/access/rmgrdesc/rmgrdesc_utils.c b/src/backend/access/rmgrdesc/rmgrdesc_utils.c index 3d16b1fcb..90051f0df 100644 --- a/src/backend/access/rmgrdesc/rmgrdesc_utils.c +++ b/src/backend/access/rmgrdesc/rmgrdesc_utils.c @@ -16,26 +16,6 @@ #include "access/rmgrdesc_utils.h" #include "storage/off.h" -/* - * Guidelines for formatting desc functions: - * - * member1_name: member1_value, member2_name: member2_value - * - * If the value is a list, please use: - * - * member3_name: [ member3_list_value1, member3_list_value2 ] - * - * The first item appended to the string should not be prepended by any spaces - * or comma, however all subsequent appends to the string are responsible for - * prepending themselves with a comma followed by a space. - * - * Flags should be in ALL CAPS. - * - * For lists/arrays of items, the number of those items should be listed at - * the beginning with all of the other numbers. - * - * Composite objects in a list should be surrounded with { }. - */ void array_desc(StringInfo buf, void *array, size_t elem_size, int count, void (*elem_desc) (StringInfo buf, void *elem, void *data), -- 2.40.0