Re: Add pg_walinspect function with block info columns - Mailing list pgsql-hackers

From Peter Geoghegan
Subject Re: Add pg_walinspect function with block info columns
Date
Msg-id CAH2-Wz=V2u0B9X1aEXDW=A-x6HnjGgNx0fkCzcpnd0U-YePXJg@mail.gmail.com
Whole thread Raw
In response to Re: Add pg_walinspect function with block info columns  (Michael Paquier <michael@paquier.xyz>)
List pgsql-hackers
On Tue, Mar 28, 2023 at 3:34 PM Michael Paquier <michael@paquier.xyz> wrote:
> I disagree with this argument.  Personally, I have a *much* better
> experience with textual representation because there is no need to
> cross-check the internals of the code in case you don't remember what
> a given number means in an enum or in a set of bits, especially if
> you're in a hurry of looking at a production or customer deployment.

I couldn't tell you which fork number is which right off the top of my
head, either (except that main fork is 0). But it doesn't matter.
There are only ever two relevant fork numbers. And even if that
changes, it's perfectly clear which is which from context. There will
be two block references for a given (say) VISIBLE record, one of which
is obviously for the main fork, the other of which is obviously for
the VM fork.

Plus it's just more consistent that way. The existing
pg_get_wal_block_info() output parameters look like they were directly
copied from pg_buffercache (the same names are already used), so why
not do the same with relforknumber?

> In short, it makes for less mistakes because you don't have to think
> about some extra mapping between some integers and what they actually
> mean through text.  The clauses you'd apply for a group by on the
> forks, or for a filter with IN clauses don't change, they're just made
> easier to understand for the common user, and that includes
> experienced people.

It's slightly harder to write a query that filters on text, and it
won't perform as well.

> We'd better think about that like the text[]
> arrays we use for the flag values, like the FPI flags, or why we've
> introduced text[] for the HEAP_* flags in the heap functions of
> pageinspect.

I think that those other things are fine, because they're much less
obvious, and are very unlikely to ever appear in a query predicate.

> There's even more consistency with pageinspect in using a fork name,
> where we can pass down a fork name to get a raw page.

pageinspect provides a way of mapping raw infomask/informask2 fields
to status flags, through its heap_tuple_infomask_flags function. But
the actual function that returns information from tuples
(heap_page_items) faithfully represents the raw on-disk format
directly -- the heap_tuple_infomask_flags part is totally optional. So
that doesn't seem like an example that supports your argument -- quite
the contrary.

I wouldn't mind adding something to the docs about fork number. In
fact it's definitely going to be necessary to have an explanation that
at least matches the one from the pg_buffercache docs.

--
Peter Geoghegan



pgsql-hackers by date:

Previous
From: Daniel Gustafsson
Date:
Subject: Re: pgindent vs. git whitespace check
Next
From: "Daniel Verite"
Date:
Subject: Re: TAP tests for psql \g piped into program