Horton

Thread

Commits

  1. pageinspect: Improve page_header() for pages of 32kB

  2. pageinspect: Change block number arguments to bigint

  3. Add functions to 'pageinspect' to inspect GiST indexes.

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

    Quan Zongliang <quanzongliang@yeah.net> — 2021-07-08T00:56:19Z

    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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-07-08T03:42:26Z

    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.

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

    Michael Paquier <michael@paquier.xyz> — 2021-07-08T04:15:12Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2021-07-08T04:28:06Z

    On Thu, Jul 08, 2021 at 01:15:12PM +0900, Michael Paquier wrote:
> 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.

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.

> 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.

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

-- 
Justin

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-08T04:49:25Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2021-07-08T05:01:05Z

    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.
--
Michael

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

    Daniel Gustafsson <daniel@yesql.se> — 2021-07-08T06:23:58Z

    > 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/

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

    Michael Paquier <michael@paquier.xyz> — 2021-07-08T07:54:49Z

    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.
--
Michael

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

    Quan Zongliang <quanzongliang@yeah.net> — 2021-07-08T10:02:03Z

    
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
>

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

    Quan Zongliang <quanzongliang@yeah.net> — 2021-07-09T01:26:37Z

    
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
>

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

    Michael Paquier <michael@paquier.xyz> — 2021-07-09T01:50:57Z

    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

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

    Quan Zongliang <quanzongliang@yeah.net> — 2021-07-09T03:11:46Z

    
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

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

    Michael Paquier <michael@paquier.xyz> — 2021-07-09T05:16:54Z

    On Fri, Jul 09, 2021 at 11:11:46AM +0800, Quan Zongliang wrote:
> Thanks for the comments.

Thanks.  Having four switches is a bit repetitive so I would just use
one of these, and perhaps complete with some assertions to make sure
that atttypid matches to what is expected.  That's a minor comment
though, this looks rather fine to me reading through.
--
Michael

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-07-09T11:56:37Z

    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.

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

    Michael Paquier <michael@paquier.xyz> — 2021-07-10T11:29:51Z

    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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-07-10T13:39:10Z

    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.

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

    Michael Paquier <michael@paquier.xyz> — 2021-07-12T02:16:10Z

    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