Re: Add pg_walinspect function with block info columns

On Tue, Mar 07, 2023 at 03:56:22PM +0530, Bharath Rupireddy wrote:
> That would be a lot better. Not just the test, but also the
> documentation can have it. Simple way to generate such a record (both
> block data and FPI) is to just change the wal_level to logical in
> walinspect.conf [1], see code around REGBUF_KEEP_DATA and
> RelationIsLogicallyLogged in heapam.c

I don't agree that we need to go down to wal_level=logical for this.
The important part is to check that the non-NULL and NULL paths for
the block data and FPI data are both taken, making 4 paths to check.
So we need two tests at minimum, which would be either:
- One SQL generating no FPI with no block data and a second generating
a FPI with block data. v2 was doing that but did not cover the first
case.
- One SQL generating a FPI with no block data and a second generating
no FPI with block data.

So let's just geenrate a heap record on an UPDATE, for example, like
in the version attached.

> 2. Used int4 instead of int for fpilen just to be in sync with
> fpi_length of pg_get_wal_record_info.

Okay.

> 3. Changed to be consistent and use just FPI or "F/full page".
> /* FPI flags */
> /* No full page image, so store NULLs for all its fields */
> /* Full-page image */
> /* Full page exists, so let's save it. */
> * and end LSNs. This produces information about the full page images with
> * to a record. Decompression is applied to the full-page images, if

Fine by me.

> 4. I think we need to free raw_data, raw_page and flags as we loop
> over multiple blocks (XLR_MAX_BLOCK_ID) and will leak memory which can
> be a problem if we have many blocks assocated with a single WAL
> record.
> flags = (Datum *) palloc0(sizeof(Datum) * bitcnt);
> Also, we will leak all CStringGetTextDatum memory in the block_id for loop.
> Another way is to use and reset temp memory context in the for loop
> over block_ids. I prefer this approach over multiple pfree()s in
> block_id for loop.

I disagree, this was on purpose in the last version. This version
finishes by calling AllocSetContextCreate() and MemoryContextDelete()
once per *record*, which will not be free, and we are arguing about
resetting the memory context after scanning up to XLR_MAX_BLOCK_ID
blocks, or 32 blocks which would go up to 32kB per page in the worst
case. That's not going to matter in a large scan for each record, but
the extra AllocSet*() calls could. And we basically do the same thing
on HEAD.

> 5. I think it'd be good to say if the FPI is for WAL_VERIFICATION, so
> I changed it to the following. Feel free to ignore this if you think
> it's not required.
> if (blk->apply_image)
> flags[cnt++] = CStringGetTextDatum("APPLY");
> else
> flags[cnt++] = CStringGetTextDatum("WAL_VERIFICATION");

Disagreed here as well. WAL_VERIFICATION does not map with any of the
internal flags, and actually it may be finished by not being used
at replay if the LSN of the page read if higher than what the WAL
stores.

> 7. Added test case which shows both block data and fpi in the
> documentation.

Okay on that.

> 8. Changed wal_level to logical in walinspect.conf to test case with block data.

This change is not necessary, per the argument above.

Any comments?
--
Michael