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
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;
signature.asc
Description: PGP signature
