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

If the block size is 32k, the function page_header of the pageinspect 
module returns negative numbers:

postgres=# select * from page_header(get_raw_page('t1',0));
     lsn    | checksum | flags | lower | upper | special | pagesize | 
version | prune_xid
-----------+----------+-------+-------+-------+---------+----------+---------+-----------
  0/174CF58 |        0 |     0 |    28 | 32736 |  -32768 |   -32768 | 
    4 |         0
(1 row)


This patch changes the output parameters lower, upper, special and 
pagesize to int32.

postgres=# select * from page_header(get_raw_page('t1',0));
     lsn    | checksum | flags | lower | upper | special | pagesize | 
version | prune_xid
-----------+----------+-------+-------+-------+---------+----------+---------+-----------
  0/19EA640 |        0 |     0 |    28 | 32736 |   32768 |    32768 | 
    4 |         0
(1 row)


--
Quan Zongliang

On Thu, Jul 8, 2021 at 6:26 AM Quan Zongliang <quanzongliang@yeah.net> wrote:
>
> If the block size is 32k, the function page_header of the pageinspect
> module returns negative numbers:
>
> postgres=# select * from page_header(get_raw_page('t1',0));
>      lsn    | checksum | flags | lower | upper | special | pagesize |
> version | prune_xid
> -----------+----------+-------+-------+-------+---------+----------+---------+-----------
>   0/174CF58 |        0 |     0 |    28 | 32736 |  -32768 |   -32768 |
>     4 |         0
> (1 row)
>
>
> This patch changes the output parameters lower, upper, special and
> pagesize to int32.
>
> postgres=# select * from page_header(get_raw_page('t1',0));
>      lsn    | checksum | flags | lower | upper | special | pagesize |
> version | prune_xid
> -----------+----------+-------+-------+-------+---------+----------+---------+-----------
>   0/19EA640 |        0 |     0 |    28 | 32736 |   32768 |    32768 |
>     4 |         0
> (1 row)

+1. int32 makes sense because the maximum allowed block size is 32768
and smallint with range -32768 to +32767 can't hold it. Internally,
lower, upper, special are treated as unit16. I looked at the patch,
how about using "int4" instead of just "int", just for readability?
And, do we need to change in pageinspect--1.1--1.2.sql and
pageinspect--1.0--1.1.sql along with pageinspect--1.5.sql?

Regards,
Bharath Rupireddy.

On Thu, Jul 08, 2021 at 09:12:26AM +0530, Bharath Rupireddy wrote:
> +1. int32 makes sense because the maximum allowed block size is 32768
> and smallint with range -32768 to +32767 can't hold it. Internally,
> lower, upper, special are treated as unit16. I looked at the patch,
> how about using "int4" instead of just "int", just for readability?
> And, do we need to change in pageinspect--1.1--1.2.sql and
> pageinspect--1.0--1.1.sql along with pageinspect--1.5.sql?

Changes in the object set of an extension requires a new SQL script
that changes the objects to reflect the change.  So, in this case,
what you need to do is to create pageinspect--1.9--1.10.sql, assuming
that the new extension version is 1.10 and change page_header()
accordingly.

You also need to be careful about compatibility with past versions of
this extension, as the code you are changing to use int8 could be used
at runtime by older versions of pageinspect where int4 is used.  I
would suggest to test that with some new extra tests in
oldextversions.sql.

The patch, and your suggestions, are incorrect on those aspects.
--
Michael

Justin Pryzby <pryzby@telsasoft.com> writes:
> On Thu, Jul 08, 2021 at 01:15:12PM +0900, Michael Paquier wrote:
>> Changes in the object set of an extension requires a new SQL script
>> that changes the objects to reflect the change.  So, in this case,
>> what you need to do is to create pageinspect--1.9--1.10.sql, assuming
>> that the new extension version is 1.10 and change page_header()
>> accordingly.

> I think it's common (and preferred) for changes in extension versions to be
> "folded" together within a major release of postgres.  Since v1.9 is new in
> pg14 (commit 756ab2912), this change can be included in the same, new version.

Since we're already past beta2, I'm not sure that's a good idea.  We
can't really treat pageinspect 1.9 as something that the world has
never seen.

            regards, tom lane

> On 8 Jul 2021, at 07:01, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Thu, Jul 08, 2021 at 12:49:25AM -0400, Tom Lane wrote:
>> Since we're already past beta2, I'm not sure that's a good idea.  We
>> can't really treat pageinspect 1.9 as something that the world has
>> never seen.
> 
> Yeah, that's why I would object to new changes in 1.9 and
> REL_14_STABLE.  So my take would be to just have 1.10, and do those
> changes on HEAD.

+1, this should go into a 1.10 version.

--
Daniel Gustafsson        https://vmware.com/

On 2021/7/8 3:54 下午, Michael Paquier wrote:
> On Wed, Jul 07, 2021 at 11:28:06PM -0500, Justin Pryzby wrote:
>> I think you can refer to this prior commit for guidance.
>>
>> commit f18aa1b203930ed28cfe42e82d3418ae6277576d
>> Author: Peter Eisentraut <peter@eisentraut.org>
>> Date:   Tue Jan 19 10:28:05 2021 +0100
>>
>>      pageinspect: Change block number arguments to bigint
> 
> Yes, thanks.  Peter's recent work is what I had in mind.  I agree that
> we could improve the situation, and the change is not complicated once
> you know what needs to be done.  It needs to be done as follows:
> - Create a new pageinspect--1.9--1.10.sql.
> - Provide a proper implementation for older extension version based on
> the output parameter types, with a lookup at the TupleDesc for the
> function to adapt.
> - Add tests to sql/oldextversions.sql to stress the old function based
> on smallint results.
> - Bump pageinspect.control.
> 
> Quan, would you like to produce a patch?  That's a good exercise to
> understand how the maintenance of extensions is done.

Ok. I'll do it.

> --
> Michael
> 

On 2021/7/8 3:54 下午, Michael Paquier wrote:
> On Wed, Jul 07, 2021 at 11:28:06PM -0500, Justin Pryzby wrote:
>> I think you can refer to this prior commit for guidance.
>>
>> commit f18aa1b203930ed28cfe42e82d3418ae6277576d
>> Author: Peter Eisentraut <peter@eisentraut.org>
>> Date:   Tue Jan 19 10:28:05 2021 +0100
>>
>>      pageinspect: Change block number arguments to bigint
> 
> Yes, thanks.  Peter's recent work is what I had in mind.  I agree that
> we could improve the situation, and the change is not complicated once
> you know what needs to be done.  It needs to be done as follows:
> - Create a new pageinspect--1.9--1.10.sql.
> - Provide a proper implementation for older extension version based on
> the output parameter types, with a lookup at the TupleDesc for the
> function to adapt.
> - Add tests to sql/oldextversions.sql to stress the old function based
> on smallint results.
> - Bump pageinspect.control.
> 
> Quan, would you like to produce a patch?  That's a good exercise to
> understand how the maintenance of extensions is done.

new patch attached

> --
> Michael
>

On 2021/7/9 9:50 上午, Michael Paquier wrote:
> 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
> 

Thanks for the comments.
Done

On Fri, Jul 9, 2021 at 8:43 AM Quan Zongliang <quanzongliang@yeah.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.

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

On Sat, Jul 10, 2021 at 4:59 PM Michael Paquier <michael@paquier.xyz> wrote:
> So I would finish with the attached, close enough to what Quan has
> sent upthread.

Thanks. The patch looks good to me, except a minor comment - isn't it
"int2 for these fields" as the fields still exist? + /* pageinspect >=
1.10 uses int4 instead of int2 for those fields */

Regards,
Bharath Rupireddy.

On Sat, Jul 10, 2021 at 07:09:10PM +0530, Bharath Rupireddy wrote:
> Thanks. The patch looks good to me, except a minor comment - isn't it
> "int2 for these fields" as the fields still exist? + /* pageinspect >=
> 1.10 uses int4 instead of int2 for those fields */

This comment looks fine to me as-is, so applied on HEAD.
--
Michael