On Mon, Mar 20, 2023 at 7:34 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Sun, Mar 19, 2023 at 8:21 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> > It is not an issue with this patch, but as I look at this version, I'm
> > starting to feel uneasy about the subtle differences between what
> > GetWALRecordsInfo and GetWALBlockInfo do. One solution might be to
> > have GetWALBlockInfo return a values array for each block, but that
> > could make things more complex than needed. Alternatively, could we
> > get GetWALRecordsInfo to call tuplestore_putvalues() internally? This
> > way, both functions can manage the temporary memory context within
> > themselves.
>
> Agreed. I'm also not sure what to do about it, though.
>
> > This means GetWALBlockInfo overwrites the last two columns generated
> > by GetWalRecordInfo, but I don't think this approach is clean and
> > stable. I agree we don't want the final columns in a block info tuple
> > but we don't want to duplicate the common code path.
>
> > I initially thought we could devide the function into
> > GetWALCommonInfo(), GetWALRecordInfo() and GetWALBlockInfo(), but it
> > doesn't seem that simple.. In the end, I think we should have separate
> > GetWALRecordInfo() and GetWALBlockInfo() that have duplicate
> > "values[i++] = .." lines.
>
> I agree. A little redundancy is better when the alternative is fragile
> code, and I'm pretty sure that that applies here -- there won't be
> very many duplicated lines, and the final code will be significantly
> clearer. There can be a comment about keeping GetWALRecordInfo and
> GetWALBlockInfo in sync.
So, I also agree that it is better to have the two separate functions
instead of overwriting the last two columns. As for keeping them in
sync, we could define the number of common columns as a macro like:
#define WALINSPECT_INFO_NUM_COMMON_COLS 10
and use that to calculate the size of the values/nulls array in
GetWalRecordInfo() and GetWALBlockInfo() (assuming a new version where
those two functions duplicate the setting of values[x] = y).
That way, if a new column of information is added and one of the two
functions forgets to set it in the values array, it would still cause an
empty column and it will be easier for the programmer to see it needs to
be added.
We could even define an enum like:
typedef enum walinspect_common_col
{
WALINSPECT_START_LSN,
WALINSPECT_END_LSN,
WALINSPECT_PREV_LSN,
WALINSPECT_XID,
WALINSPECT_RMGR,
WALINSPECT_REC_TYPE,
WALINSPECT_REC_LENGTH,
WALINSPECT_MAIN_DATA_LENGTH,
WALINSPECT_FPILEN,
WALINSPECT_DESC,
WALINSPECT_NUM_COMMON_COL,
} walinspect_common_col;
and set values in both functions like
values[WALINSPECT_FPILEN] = y
if we kept the order of common columns the same and as the first N
columns for both functions. This would keep us from having to manually
update a macro like WALINSPECT_INFO_NUM_COMMON_COLS.
Though, I'm not sure how much value that actually adds.
- Melanie
