On Fri, Jul 09, 2021 at 09:26:37AM +0800, Quan Zongliang wrote:
> new patch attached

That's mostly fine at quick glance.  Here are some comments.

Please add pageinspect--1.9--1.10.sql within the patch.  Using git,
you can do that with a simple "git add".  With the current shape of
the patch, one has to manually copy pageinspect--1.9--1.10.sql into
contrib/pageinspect/ to be able to test it.

+SELECT lower, upper, special, pagesize, version FROM
page_header(get_raw_page('test1', 0));
+ lower | upper | special | pagesize | version
+-------+-------+---------+----------+---------
+    28 |  8152 |    8192 |     8192 |       4
+(1 row)
I would not test all the fields, just pagesize and version perhaps?

+   if (TupleDescAttr(tupdesc, 3)->atttypid == INT2OID)
+       values[3] = UInt16GetDatum(page->pd_lower);
+   else
+       values[3] = Int32GetDatum(page->pd_lower);
Let's make the style more consistent with brinfuncs.c, by grouping all
the fields together in a switch/case, like that:
switch ((TupleDescAttr(tupdesc, 3)->atttypid)):
{
    case INT2OID:
        /* fill in values with UInt16GetDatum() */
        break;
    case INT4OID:
        /* fill in values with Int32GetDatum() */
        break;
    default:
        elog(ERROR, "blah");
}

+ALTER EXTENSION pageinspect UPDATE TO '1.10';
+\df page_header
+SELECT lower, upper, special, pagesize, version FROM 
page_header(get_raw_page('test1', 0));
No need for this test as page.sql already stresses page_header() for
the latest version.  Perhaps we could just UPDATE TO '1.9' before
running your query to show that it is the last version of pageinspect
where the older page_header() appeared.

The documentation of pageinspect does not require an update, as far as
I can see.  So we are good there.
--
Michael

Attachment: signature.asc
Description: PGP signature

Reply via email to