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
Attachments:
pageinspect.patchtext/plain; charset=UTF-8; name=pageinspect.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/contrib/pageinspect/pageinspect--1.5.sql b/contrib/pageinspect/pageinspect--1.5.sql
index 1e40c3c97e..f71eff19c5 100644
--- a/contrib/pageinspect/pageinspect--1.5.sql
+++ b/contrib/pageinspect/pageinspect--1.5.sql
@@ -23,10 +23,10 @@ CREATE FUNCTION page_header(IN page bytea,
OUT lsn pg_lsn,
OUT checksum smallint,
OUT flags smallint,
- OUT lower smallint,
- OUT upper smallint,
- OUT special smallint,
- OUT pagesize smallint,
+ OUT lower int,
+ OUT upper int,
+ OUT special int,
+ OUT pagesize int,
OUT version smallint,
OUT prune_xid xid)
AS 'MODULE_PATHNAME', 'page_header'
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 7272b21016..af04c1a688 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -316,10 +316,10 @@ page_header(PG_FUNCTION_ARGS)
values[0] = LSNGetDatum(lsn);
values[1] = UInt16GetDatum(page->pd_checksum);
values[2] = UInt16GetDatum(page->pd_flags);
- values[3] = UInt16GetDatum(page->pd_lower);
- values[4] = UInt16GetDatum(page->pd_upper);
- values[5] = UInt16GetDatum(page->pd_special);
- values[6] = UInt16GetDatum(PageGetPageSize(page));
+ values[3] = Int32GetDatum(page->pd_lower);
+ values[4] = Int32GetDatum(page->pd_upper);
+ values[5] = Int32GetDatum(page->pd_special);
+ values[6] = Int32GetDatum(PageGetPageSize(page));
values[7] = UInt16GetDatum(PageGetPageLayoutVersion(page));
values[8] = TransactionIdGetDatum(page->pd_prune_xid);
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
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
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 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
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 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 +0100pageinspect: 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
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 +0100pageinspect: 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.
Show quoted text
--
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 +0100pageinspect: 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
Show quoted text
--
Michael
Attachments:
pageinspect.patchtext/plain; charset=UTF-8; name=pageinspect.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 2d330ddb28..9999068134 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -13,7 +13,8 @@ OBJS = \
rawpage.o
EXTENSION = pageinspect
-DATA = pageinspect--1.8--1.9.sql \
+DATA = pageinspect--1.9--1.10.sql \
+ pageinspect--1.8--1.9.sql \
pageinspect--1.7--1.8.sql pageinspect--1.6--1.7.sql \
pageinspect--1.5.sql pageinspect--1.5--1.6.sql \
pageinspect--1.4--1.5.sql pageinspect--1.3--1.4.sql \
diff --git a/contrib/pageinspect/expected/oldextversions.out b/contrib/pageinspect/expected/oldextversions.out
index 04dc7f8640..9546791e23 100644
--- a/contrib/pageinspect/expected/oldextversions.out
+++ b/contrib/pageinspect/expected/oldextversions.out
@@ -36,5 +36,32 @@ SELECT * FROM bt_page_items('test1_a_idx', 1);
1 | (0,1) | 16 | f | f | 01 00 00 00 00 00 00 01 | f | (0,1) |
(1 row)
+\df page_header
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type
+--------+-------------+------------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+------
+ public | page_header | record | page bytea, OUT lsn pg_lsn, OUT checksum smallint, OUT flags smallint, OUT lower smallint, OUT upper smallint, OUT special smallint, OUT pagesize smallint, OUT version smallint, OUT prune_xid xid | func
+(1 row)
+
+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)
+
+ALTER EXTENSION pageinspect UPDATE TO '1.10';
+\df page_header
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type
+--------+-------------+------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+------
+ public | page_header | record | page bytea, OUT lsn pg_lsn, OUT checksum smallint, OUT flags smallint, OUT lower integer, OUT upper integer, OUT special integer, OUT pagesize integer, OUT version smallint, OUT prune_xid xid | func
+(1 row)
+
+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)
+
DROP TABLE test1;
DROP EXTENSION pageinspect;
diff --git a/contrib/pageinspect/pageinspect.control b/contrib/pageinspect/pageinspect.control
index bd716769a1..7cdf37913d 100644
--- a/contrib/pageinspect/pageinspect.control
+++ b/contrib/pageinspect/pageinspect.control
@@ -1,5 +1,5 @@
# pageinspect extension
comment = 'inspect the contents of database pages at a low level'
-default_version = '1.9'
+default_version = '1.10'
module_pathname = '$libdir/pageinspect'
relocatable = true
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index e9fee73bc4..2d1f90e3bc 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -296,10 +296,27 @@ page_header(PG_FUNCTION_ARGS)
values[0] = LSNGetDatum(lsn);
values[1] = UInt16GetDatum(page->pd_checksum);
values[2] = UInt16GetDatum(page->pd_flags);
- values[3] = UInt16GetDatum(page->pd_lower);
- values[4] = UInt16GetDatum(page->pd_upper);
- values[5] = UInt16GetDatum(page->pd_special);
- values[6] = UInt16GetDatum(PageGetPageSize(page));
+
+ if (TupleDescAttr(tupdesc, 3)->atttypid == INT2OID)
+ values[3] = UInt16GetDatum(page->pd_lower);
+ else
+ values[3] = Int32GetDatum(page->pd_lower);
+
+ if (TupleDescAttr(tupdesc, 4)->atttypid == INT2OID)
+ values[4] = UInt16GetDatum(page->pd_upper);
+ else
+ values[4] = Int32GetDatum(page->pd_upper);
+
+ if (TupleDescAttr(tupdesc, 5)->atttypid == INT2OID)
+ values[5] = UInt16GetDatum(page->pd_special);
+ else
+ values[5] = Int32GetDatum(page->pd_special);
+
+ if (TupleDescAttr(tupdesc, 5)->atttypid == INT2OID)
+ values[6] = UInt16GetDatum(PageGetPageSize(page));
+ else
+ values[6] = Int32GetDatum(PageGetPageSize(page));
+
values[7] = UInt16GetDatum(PageGetPageLayoutVersion(page));
values[8] = TransactionIdGetDatum(page->pd_prune_xid);
diff --git a/contrib/pageinspect/sql/oldextversions.sql b/contrib/pageinspect/sql/oldextversions.sql
index 78e08f40e8..1c0d73b441 100644
--- a/contrib/pageinspect/sql/oldextversions.sql
+++ b/contrib/pageinspect/sql/oldextversions.sql
@@ -16,5 +16,11 @@ SELECT page_checksum(get_raw_page('test1', 0), 0) IS NOT NULL AS silly_checksum_
SELECT * FROM bt_page_stats('test1_a_idx', 1);
SELECT * FROM bt_page_items('test1_a_idx', 1);
+\df page_header
+SELECT lower, upper, special, pagesize, version FROM page_header(get_raw_page('test1', 0));
+ALTER EXTENSION pageinspect UPDATE TO '1.10';
+\df page_header
+SELECT lower, upper, special, pagesize, version FROM page_header(get_raw_page('test1', 0));
+
DROP TABLE test1;
DROP EXTENSION pageinspect;
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
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
Attachments:
pageinspect.patchtext/plain; charset=UTF-8; name=pageinspect.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 2d330ddb28..9999068134 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -13,7 +13,8 @@ OBJS = \
rawpage.o
EXTENSION = pageinspect
-DATA = pageinspect--1.8--1.9.sql \
+DATA = pageinspect--1.9--1.10.sql \
+ pageinspect--1.8--1.9.sql \
pageinspect--1.7--1.8.sql pageinspect--1.6--1.7.sql \
pageinspect--1.5.sql pageinspect--1.5--1.6.sql \
pageinspect--1.4--1.5.sql pageinspect--1.3--1.4.sql \
diff --git a/contrib/pageinspect/expected/oldextversions.out b/contrib/pageinspect/expected/oldextversions.out
index 04dc7f8640..1bb3919504 100644
--- a/contrib/pageinspect/expected/oldextversions.out
+++ b/contrib/pageinspect/expected/oldextversions.out
@@ -36,5 +36,19 @@ SELECT * FROM bt_page_items('test1_a_idx', 1);
1 | (0,1) | 16 | f | f | 01 00 00 00 00 00 00 01 | f | (0,1) |
(1 row)
+ALTER EXTENSION pageinspect UPDATE TO '1.9';
+\df page_header
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type
+--------+-------------+------------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+------
+ public | page_header | record | page bytea, OUT lsn pg_lsn, OUT checksum smallint, OUT flags smallint, OUT lower smallint, OUT upper smallint, OUT special smallint, OUT pagesize smallint, OUT version smallint, OUT prune_xid xid | func
+(1 row)
+
+SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
+ pagesize | version
+----------+---------
+ 8192 | 4
+(1 row)
+
DROP TABLE test1;
DROP EXTENSION pageinspect;
diff --git a/contrib/pageinspect/pageinspect--1.9--1.10.sql b/contrib/pageinspect/pageinspect--1.9--1.10.sql
new file mode 100644
index 0000000000..8dd9a27505
--- /dev/null
+++ b/contrib/pageinspect/pageinspect--1.9--1.10.sql
@@ -0,0 +1,21 @@
+/* contrib/pageinspect/pageinspect--1.9--1.10.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pageinspect UPDATE TO '1.10'" to load this file. \quit
+
+--
+-- page_header()
+--
+DROP FUNCTION page_header(IN page bytea);
+CREATE FUNCTION page_header(IN page bytea,
+ OUT lsn pg_lsn,
+ OUT checksum smallint,
+ OUT flags smallint,
+ OUT lower int,
+ OUT upper int,
+ OUT special int,
+ OUT pagesize int,
+ OUT version smallint,
+ OUT prune_xid xid)
+AS 'MODULE_PATHNAME', 'page_header'
+LANGUAGE C STRICT PARALLEL SAFE;
diff --git a/contrib/pageinspect/pageinspect.control b/contrib/pageinspect/pageinspect.control
index bd716769a1..7cdf37913d 100644
--- a/contrib/pageinspect/pageinspect.control
+++ b/contrib/pageinspect/pageinspect.control
@@ -1,5 +1,5 @@
# pageinspect extension
comment = 'inspect the contents of database pages at a low level'
-default_version = '1.9'
+default_version = '1.10'
module_pathname = '$libdir/pageinspect'
relocatable = true
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index e9fee73bc4..fdfb93ec1a 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -296,10 +296,59 @@ page_header(PG_FUNCTION_ARGS)
values[0] = LSNGetDatum(lsn);
values[1] = UInt16GetDatum(page->pd_checksum);
values[2] = UInt16GetDatum(page->pd_flags);
- values[3] = UInt16GetDatum(page->pd_lower);
- values[4] = UInt16GetDatum(page->pd_upper);
- values[5] = UInt16GetDatum(page->pd_special);
- values[6] = UInt16GetDatum(PageGetPageSize(page));
+
+ switch (TupleDescAttr(tupdesc, 3)->atttypid)
+ {
+ case INT2OID:
+ values[3] = UInt16GetDatum(page->pd_lower);
+ break;
+ case INT4OID:
+ values[3] = Int32GetDatum(page->pd_lower);
+ break;
+ default:
+ elog(ERROR, "incorrect output types");
+ break;
+ }
+
+ switch (TupleDescAttr(tupdesc, 4)->atttypid)
+ {
+ case INT2OID:
+ values[4] = UInt16GetDatum(page->pd_upper);
+ break;
+ case INT4OID:
+ values[4] = Int32GetDatum(page->pd_upper);
+ break;
+ default:
+ elog(ERROR, "incorrect output types");
+ break;
+ }
+
+ switch (TupleDescAttr(tupdesc, 5)->atttypid)
+ {
+ case INT2OID:
+ values[5] = UInt16GetDatum(page->pd_special);
+ break;
+ case INT4OID:
+ values[5] = Int32GetDatum(page->pd_special);
+ break;
+ default:
+ elog(ERROR, "incorrect output types");
+ break;
+ }
+
+ switch (TupleDescAttr(tupdesc, 6)->atttypid)
+ {
+ case INT2OID:
+ values[6] = UInt16GetDatum(PageGetPageSize(page));
+ break;
+ case INT4OID:
+ values[6] = Int32GetDatum(PageGetPageSize(page));
+ break;
+ default:
+ elog(ERROR, "incorrect output types");
+ break;
+ }
+
values[7] = UInt16GetDatum(PageGetPageLayoutVersion(page));
values[8] = TransactionIdGetDatum(page->pd_prune_xid);
diff --git a/contrib/pageinspect/sql/oldextversions.sql b/contrib/pageinspect/sql/oldextversions.sql
index 78e08f40e8..b400e7ceeb 100644
--- a/contrib/pageinspect/sql/oldextversions.sql
+++ b/contrib/pageinspect/sql/oldextversions.sql
@@ -16,5 +16,9 @@ SELECT page_checksum(get_raw_page('test1', 0), 0) IS NOT NULL AS silly_checksum_
SELECT * FROM bt_page_stats('test1_a_idx', 1);
SELECT * FROM bt_page_items('test1_a_idx', 1);
+ALTER EXTENSION pageinspect UPDATE TO '1.9';
+\df page_header
+SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
+
DROP TABLE test1;
DROP EXTENSION pageinspect;
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
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 | 44) 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
Attachments:
pageinspect-fields-v3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 2d330ddb28..5c0736564a 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -13,7 +13,7 @@ OBJS = \
rawpage.o
EXTENSION = pageinspect
-DATA = pageinspect--1.8--1.9.sql \
+DATA = pageinspect--1.9--1.10.sql pageinspect--1.8--1.9.sql \
pageinspect--1.7--1.8.sql pageinspect--1.6--1.7.sql \
pageinspect--1.5.sql pageinspect--1.5--1.6.sql \
pageinspect--1.4--1.5.sql pageinspect--1.3--1.4.sql \
diff --git a/contrib/pageinspect/expected/oldextversions.out b/contrib/pageinspect/expected/oldextversions.out
index 04dc7f8640..f5c4b61bd7 100644
--- a/contrib/pageinspect/expected/oldextversions.out
+++ b/contrib/pageinspect/expected/oldextversions.out
@@ -36,5 +36,21 @@ SELECT * FROM bt_page_items('test1_a_idx', 1);
1 | (0,1) | 16 | f | f | 01 00 00 00 00 00 00 01 | f | (0,1) |
(1 row)
+-- page_header() uses int instead of smallint for lower, upper, special and
+-- pagesize in pageinspect >= 1.10.
+ALTER EXTENSION pageinspect UPDATE TO '1.9';
+\df page_header
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type
+--------+-------------+------------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+------
+ public | page_header | record | page bytea, OUT lsn pg_lsn, OUT checksum smallint, OUT flags smallint, OUT lower smallint, OUT upper smallint, OUT special smallint, OUT pagesize smallint, OUT version smallint, OUT prune_xid xid | func
+(1 row)
+
+SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
+ pagesize | version
+----------+---------
+ 8192 | 4
+(1 row)
+
DROP TABLE test1;
DROP EXTENSION pageinspect;
diff --git a/contrib/pageinspect/pageinspect--1.9--1.10.sql b/contrib/pageinspect/pageinspect--1.9--1.10.sql
new file mode 100644
index 0000000000..8dd9a27505
--- /dev/null
+++ b/contrib/pageinspect/pageinspect--1.9--1.10.sql
@@ -0,0 +1,21 @@
+/* contrib/pageinspect/pageinspect--1.9--1.10.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pageinspect UPDATE TO '1.10'" to load this file. \quit
+
+--
+-- page_header()
+--
+DROP FUNCTION page_header(IN page bytea);
+CREATE FUNCTION page_header(IN page bytea,
+ OUT lsn pg_lsn,
+ OUT checksum smallint,
+ OUT flags smallint,
+ OUT lower int,
+ OUT upper int,
+ OUT special int,
+ OUT pagesize int,
+ OUT version smallint,
+ OUT prune_xid xid)
+AS 'MODULE_PATHNAME', 'page_header'
+LANGUAGE C STRICT PARALLEL SAFE;
diff --git a/contrib/pageinspect/pageinspect.control b/contrib/pageinspect/pageinspect.control
index bd716769a1..7cdf37913d 100644
--- a/contrib/pageinspect/pageinspect.control
+++ b/contrib/pageinspect/pageinspect.control
@@ -1,5 +1,5 @@
# pageinspect extension
comment = 'inspect the contents of database pages at a low level'
-default_version = '1.9'
+default_version = '1.10'
module_pathname = '$libdir/pageinspect'
relocatable = true
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index e9fee73bc4..4bfa346c24 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -296,10 +296,33 @@ page_header(PG_FUNCTION_ARGS)
values[0] = LSNGetDatum(lsn);
values[1] = UInt16GetDatum(page->pd_checksum);
values[2] = UInt16GetDatum(page->pd_flags);
- values[3] = UInt16GetDatum(page->pd_lower);
- values[4] = UInt16GetDatum(page->pd_upper);
- values[5] = UInt16GetDatum(page->pd_special);
- values[6] = UInt16GetDatum(PageGetPageSize(page));
+
+ /* pageinspect >= 1.10 uses int4 instead of int2 for those fields */
+ switch (TupleDescAttr(tupdesc, 3)->atttypid)
+ {
+ case INT2OID:
+ Assert(TupleDescAttr(tupdesc, 4)->atttypid == INT2OID &&
+ TupleDescAttr(tupdesc, 5)->atttypid == INT2OID &&
+ TupleDescAttr(tupdesc, 6)->atttypid == INT2OID);
+ values[3] = UInt16GetDatum(page->pd_lower);
+ values[4] = UInt16GetDatum(page->pd_upper);
+ values[5] = UInt16GetDatum(page->pd_special);
+ values[6] = UInt16GetDatum(PageGetPageSize(page));
+ break;
+ case INT4OID:
+ Assert(TupleDescAttr(tupdesc, 4)->atttypid == INT4OID &&
+ TupleDescAttr(tupdesc, 5)->atttypid == INT4OID &&
+ TupleDescAttr(tupdesc, 6)->atttypid == INT4OID);
+ values[3] = Int32GetDatum(page->pd_lower);
+ values[4] = Int32GetDatum(page->pd_upper);
+ values[5] = Int32GetDatum(page->pd_special);
+ values[6] = Int32GetDatum(PageGetPageSize(page));
+ break;
+ default:
+ elog(ERROR, "incorrect output types");
+ break;
+ }
+
values[7] = UInt16GetDatum(PageGetPageLayoutVersion(page));
values[8] = TransactionIdGetDatum(page->pd_prune_xid);
diff --git a/contrib/pageinspect/sql/oldextversions.sql b/contrib/pageinspect/sql/oldextversions.sql
index 78e08f40e8..9f953492c2 100644
--- a/contrib/pageinspect/sql/oldextversions.sql
+++ b/contrib/pageinspect/sql/oldextversions.sql
@@ -16,5 +16,11 @@ SELECT page_checksum(get_raw_page('test1', 0), 0) IS NOT NULL AS silly_checksum_
SELECT * FROM bt_page_stats('test1_a_idx', 1);
SELECT * FROM bt_page_items('test1_a_idx', 1);
+-- page_header() uses int instead of smallint for lower, upper, special and
+-- pagesize in pageinspect >= 1.10.
+ALTER EXTENSION pageinspect UPDATE TO '1.9';
+\df page_header
+SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
+
DROP TABLE test1;
DROP EXTENSION pageinspect;
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