Re: Add pg_walinspect function with block info columns

On Tue, Mar 28, 2023 at 5:29 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>
> On Mon, Mar 27, 2023 at 12:42 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > Thanks. Here's the v6 patch (last patch that I have with me for
> > pg_walinspect) for adding per-record info to pg_get_wal_block_info.
> > Note that I addressed all review comments received so far. Any
> > thoughts?
>
> Looking at this now, with the intention of committing it for 16.
>
> In addition to what I said a little while ago about the forknum
> parameter and parameter ordering, I have a concern about the data
> type: perhaps the forknum paramater should be declared as
> "relforknumber smallint", instead of using text? That would match the
> approach taken by pg_buffercache, and would be more efficient.
>
> I don't think that using a text column with the fork name adds too
> much, since this is after all supposed to be a tool used by experts.
> Plus it's usually pretty clear what it is from context. Not that many
> WAL records touch the visibility map, and those that do make it
> relatively obvious which block is from the VM based on other details.
> Details such as blockid and relblocknumber (the VM is approximately
> 32k times smaller than the heap). Once I see that the record is (say)
> a VISIBLE record, I'm already looking at the order of each block
> reference, and maybe at relblocknumber -- I'm not likely to visually
> scan the forknum column at all.

Hm, agreed. Changed in the attached v7-0002 patch. We can as well
write a case statement in the create function SQL to output forkname
instead forknumber, but I'd stop doing that to keep in sync with
pg_buffercache.

On Tue, Mar 28, 2023 at 6:37 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>
> On Mon, Mar 27, 2023 at 4:59 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> > Looking at this now, with the intention of committing it for 16.
>
> I see a bug on HEAD, following yesterday's commit 0276ae42dd.
>
> GetWALRecordInfo() will now output the value of the fpi_len variable
> before it has actually been set by our call to XXXX. So it'll always
> be 0.
>
> Can you post a bugfix patch for this, Bharath?

Oh, thanks for finding it out. Fixed in the attached v7-0001 patch. I
also removed the "invalid fork number" error as users can figure that
out if at all the fork number is wrong.

On the ordering of the columns, I kept start_lsn, end_lsn and prev_lsn
first and then the rel** columns (this rel** columns order follows
pg_buffercache) and then block data related columns. Michael and
Kyotaro are of the opinion that it's better to keep LSNs first to be
consistent and also given that this function is WAL related, it makes
sense to have LSNs first.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com