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

On Thu, Mar 24, 2022 at 10:22 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
> +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).

I'm passing NULL if the delimiter isn't required (for pg_walinspect)
and I wanted to check if it's passed, so I was using the delimiter in
the condition. However, I now changed it to delimiter != NULL.

> +get_forkname(ForkNumber num)
>
> forkNames[] is public and used in reinit.c. I think we don't need
> this function.

Yes. I removed it.

> +#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.

I'm not sure if it's worth adding new files xlogstats.h/.c just for 2
structures, 1 macro, and 2 functions with no plan to add new stats
structures or functions. Since xlogreader is the one that reads the
WAL, and is being included by both backend and other modules (tools
and extensions) IMO it's the right place. However, I can specify in
xlogreader that if at all the new stats related structures or
functions are going to be added, it's good to move them into a new
header and .c file.

Thoughts?

> +#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);

I removed the XLOG_GET_STATS_PERCENTAGE macro.

Attaching v14 patch-set here with. It has bunch of other changes along
with the above:

1) Used STRICT for all the functions and introduced _till_end_of_wal
versions for pg_get_wal_records_info and pg_get_wal_stats.
2) Most of the code is reused between pg_walinspect and pg_waldump and
also within pg_walinspect.
3) Added read_local_xlog_page_no_wait without duplicating the code so
that the pg_walinspect functions don't wait even while finding the
first valid WAL record.
4) No function waits for future WAL lsn even to find the first valid WAL record.
5) Addressed the review comments raised upthread by Andres.

I hope this version makes the patch cleaner, please review it further.

Regards,
Bharath Rupireddy.