On Fri, Apr 7, 2023 at 5:43 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Fri, Apr 7, 2023 at 1:33 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > Attached v3 is cleaned up and includes a pg_walinspect docs update as
> > well as some edited comments in rmgr_utils.c
>
> Attached v4 has some small tweaks on your v3. Mostly just whitespace
> tweaks. Two slightly notable tweaks:
>
> * I changed the approach to globbing in the Makefile, rather than use
> your original overwide formulation for the new rmgrdesc_utils.c file.
>
> What do you think of this approach?
Seems fine.
> * Removed use of the restrict keyword.
>
> While "restrict" is C99, I'm not completely sure that it's totally
> supported by Postgres. I'm a bit surprised that you opted to use it in
> this particular patch.
>
> I meant to ask you about this earlier...why use restrict in this patch?
So, I think the signature I meant to have was:
void
array_desc(StringInfo buf, void *array, size_t elem_size, int count,
void (*elem_desc) (StringInfo buf, const void *elem, void *data),
void *data)
Basically I wanted to indicate that elem was not and should not be
modified and data can be modified but that they should not be the same
element or overlap at all.
> > I've added such an example to pg_walinspect docs.
>
> There already was a PRUNE example, though -- for the
> pg_get_wal_record_info function (singular, not to be confused with
> pg_get_wal_records_info).
>
> v4 makes the example a VACUUM record, which replaces the previous
> pg_get_wal_record_info PRUNE example -- that needed to be updated
> anyway. This approach has the advantage of not being too verbose,
> which still showing some of this kind of detail.
>
> This has the advantage of allowing pg_get_wal_records_info's example
> to continue to be an example that lacks a block reference (and so has
> a NULL block_ref). This is a useful contrast against the new
> pg_get_wal_block_info function.
LGTM
- Melanie