Re: pg_walinspect - a new extension to get raw WAL data and WAL stats

At Wed, 23 Mar 2022 21:36:09 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> Here's a refactoring patch that basically moves the pg_waldump's
> functions and stats structures to xlogreader.h/.c so that the
> pg_walinspect can reuse them. If it looks okay, I will send the
> pg_walinspect patches based on it.


+void
+XLogRecGetBlockRefInfo(XLogReaderState *record, char *delimiter,
+					   uint32 *fpi_len, bool detailed_format,
+					   StringInfo buf)
...
+		if (detailed_format && delimiter)
+			appendStringInfoChar(buf, '\n');

It is odd that the variable "delimiter" is used as a bool in the
function, though it is a "char *", which I meant that it is used as
delimiter string (assuming that you might want to insert ", " between
two blkref descriptions).


+get_forkname(ForkNumber num)

forkNames[] is public and used in reinit.c.  I think we don't need
this function.


+#define MAX_XLINFO_TYPES 16
...
+	XLogRecStats	rmgr_stats[RM_NEXT_ID];
+	XLogRecStats	record_stats[RM_NEXT_ID][MAX_XLINFO_TYPES];
+} XLogStats;
+

This doesn't seem to be a part of xlogreader.  Couldn't we add a new
module "xlogstats"?  XLogRecGetBlockRefInfo also doesn't seem to me as
a part of xlogreader, the xlogstats looks like a better place.


+#define XLOG_GET_STATS_PERCENTAGE(n_pct, rec_len_pct, fpi_len_pct, \
+								  tot_len_pct, total_count, \

It doesn't need to be a macro. However in the first place I don't
think it is useful to have.  Rather it may be harmful since it doesn't
reduce complexity much but instead just hides details.  If we want to
avoid tedious repetitions of the same statements, a macro like the
following may work.

#define CALC_PCT (num, denom) ((denom) == 0 ? 0.0 ? 100.0 * (num) / (denom))
...
>   n_pct = CALC_PCT(n, total_count);
>   rec_len_pct = CALC_PCT(rec_len, total_rec_len);
>   fpi_len_pct = CALC_PCT(fpi_len, total_fpi_len);
>   tot_len_pct = CALC_PCT(tot_len, total_len);

But it is not seem that different if we directly write out the detail.

>   n_pct = (total_count == 0 ? 0 : 100.0 * n / total_count);
>   rec_len_pct = (total_rec_len == 0 ? 0 : 100.0 * rec_len / total_rec_len);
>   fpi_len_pct = (total_fpi_len == 0 ? 0 : 100.0 * fpi_len / total_fpi_len);
>   tot_len_pct = (total_len == 0 ? 0 : 100.0 * tot_len / total_len);

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center