Re: bugfix: when the blocksize is 32k, the function page_header of pageinspect returns negative numbers.

On Fri, Jul 09, 2021 at 05:26:37PM +0530, Bharath Rupireddy wrote:
> 1) How about just adding a comment /* support for old extension
> version */ before INT2OID handling?
> + case INT2OID:
> + values[3] = UInt16GetDatum(page->pd_lower);
> + break;

Yes, having a comment to document from which version this is done
would be nice.  This is more consistent with the surroundings.

> 2) Do we ever reach the error statement elog(ERROR, "incorrect output
> types");? We have the function either defined with smallint or int, I
> don't think so we ever reach it. Instead, an assertion would work as
> suggested by Micheal.

I would keep an elog() here for the default case.  I was referring to
the use of assertions if changing the code into a single switch/case,
with assertions checking that the other arguments have the expected
type.

> 3) Isn't this test case unstable when regression tests are run with a
> different BLCKSZ setting? Or is it okay that some of the other tests
> for pageinspect already outputs page_size, hash_page_stats.
> +SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
> + pagesize | version
> +----------+---------
> +     8192 |       4

I don't think it matters much, most of the tests of pageinspect
already rely on 8k pages.  So let's keep it as-is.

> 4) Can we arrange pageinspect--1.8--1.9.sql  into the first line itself?
> +DATA =  pageinspect--1.9--1.10.sql \
> + pageinspect--1.8--1.9.sql \

That's a nit, but why not.

> 5) How about using "int4" instead of just "int", just for readability?

Any way is fine, I'd stick with "int" as the other fields used
"smallint".

> 6) How about something like below instead of repetitive switch statements?
> static inline Datum
> get_page_header_attr(TupleDesc desc, int attno, int val)
> {
> Oid atttypid;
> Datum datum;

Nah.  It does not seem like an improvement to me in terms of
readability.

So I would finish with the attached, close enough to what Quan has
sent upthread.
--
Michael