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

On Fri, Jul 9, 2021 at 8:43 AM Quan Zongliang <quanzongliang(at)yeah(dot)net> wrote:
> Thanks for the comments.
> Done

Thanks for the patch. Few comments:

1) How about just adding a comment /* support for old extension
version */ before INT2OID handling?
+ case INT2OID:
+ values[3] = UInt16GetDatum(page->pd_lower);
+ break;

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.

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

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 \

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

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;

atttypid = TupleDescAttr(desc, attno)->atttypid;
Assert(atttypid == INT2OID || atttypid == INT4OID);

if (atttypid == INT2OID)
datum = UInt16GetDatum(val);
else if (atttypid == INT4OID)
datum = Int32GetDatum(val);

return datum;
}

values[3] = get_page_header_attr(tupdesc, 3, page->pd_lower);
values[4] = get_page_header_attr(tupdesc, 4, page->pd_upper);
values[5] = get_page_header_attr(tupdesc, 5, page->pd_special);
values[6] = get_page_header_attr(tupdesc, 6, PageGetPageSize(page));

Regards,
Bharath Rupireddy.