First of all, thanks for the review and remarks!
18.02.2022 08:02, Michael Paquier пишет:
On Thu, Feb 17, 2022 at 05:40:41PM +0800, Julien Rouhaud wrote:
About the patch, it's incorrectly using a hardcoded 8192 block-size rather than
using the computed :block_size (or computing one when it's not already the
case).
Well, the tests of pageinspect fail would already fail when using a
different page size than 8k, like the checksum ones :)
Anywa, I agree with your point that if this is not a reason to not
make the tests more portable if we can do easily.
Fixed.
I'm also wondering if it wouldn't be better to return NULL rather than throwing
an error for uninitialized pages.
Agreed that this is a sensible choice. NULL would be helpful for the
case where one compiles all the checksums of a relation with a full
scan based on the relation size, for example. All these behaviors
ought to be documented properly, as well. For SRFs, this should just
return no rows rather than generating an error.
So, I tried to implement this remark. However, further getting rid of
throwing an error and replacing it with a NULL return led to reproduce
the problem that Alexander Lakhin mentioned And here I need little more
time to figure it out.
And one more addition. In the previous version of the patch, I forgot to
add tests for the gist index, but the described problem is also relevant
for it.
--
Daria Lepikhova
Postgres Professional
From 7dbfa7f01bf485d2812b24af012721b4a1e1e785 Mon Sep 17 00:00:00 2001
From: "d.lepikhova" <d.lepikh...@postgrespro.ru>
Date: Tue, 22 Feb 2022 16:25:31 +0500
Subject: [PATCH] Add checks for null pages into pageinspect
---
contrib/pageinspect/brinfuncs.c | 10 ++++++++++
contrib/pageinspect/btreefuncs.c | 10 ++++++++++
contrib/pageinspect/expected/brin.out | 13 +++++++++++++
contrib/pageinspect/expected/btree.out | 5 +++++
contrib/pageinspect/expected/checksum.out | 7 +++++++
contrib/pageinspect/expected/gin.out | 7 +++++++
contrib/pageinspect/expected/gist.out | 5 +++++
contrib/pageinspect/expected/hash.out | 9 +++++++++
contrib/pageinspect/rawpage.c | 10 ++++++++++
contrib/pageinspect/sql/brin.sql | 6 ++++++
contrib/pageinspect/sql/btree.sql | 3 +++
contrib/pageinspect/sql/checksum.sql | 3 +++
contrib/pageinspect/sql/gin.sql | 5 +++++
contrib/pageinspect/sql/gist.sql | 5 +++++
contrib/pageinspect/sql/hash.sql | 6 ++++++
15 files changed, 104 insertions(+)
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index 50892b5cc20..a2fee13e2d1 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -63,6 +63,10 @@ brin_page_type(PG_FUNCTION_ARGS)
errdetail("Expected size %d, got %d",
BLCKSZ, raw_page_size)));
+ /* check that the page is initialized before accessing any fields */
+ if (PageIsNew(page))
+ PG_RETURN_NULL();
+
switch (BrinPageType(page))
{
case BRIN_PAGETYPE_META:
@@ -103,6 +107,12 @@ verify_brin_page(bytea *raw_page, uint16 type, const char *strtype)
page = VARDATA(raw_page);
+ /* check that the page is initialized before accessing any fields */
+ if (PageIsNew(page))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("input page is empty")));
+
/* verify the special space says this page is what we want */
if (BrinPageType(page) != type)
ereport(ERROR,
diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index 03debe336ba..5ea7af5b998 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -519,6 +519,12 @@ bt_page_items_internal(PG_FUNCTION_ARGS, enum pageinspect_version ext_version)
UnlockReleaseBuffer(buffer);
relation_close(rel, AccessShareLock);
+ /* check that the page is initialized before accessing any fields */
+ if (PageIsNew(uargs->page))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("input page is empty")));
+
uargs->offset = FirstOffsetNumber;
opaque = (BTPageOpaque) PageGetSpecialPointer(uargs->page);
@@ -615,6 +621,10 @@ bt_page_items_bytea(PG_FUNCTION_ARGS)
uargs->page = VARDATA(raw_page);
+ /* check that the page is initialized before accessing any fields */
+ if (PageIsNew(uargs->page))
+ PG_RETURN_NULL();
+
uargs->offset = FirstOffsetNumber;
opaque = (BTPageOpaque) PageGetSpecialPointer(uargs->page);
diff --git a/contrib/pageinspect/expected/brin.out b/contrib/pageinspect/expected/brin.out
index 71eb190380c..2839ee9435b 100644
--- a/contrib/pageinspect/expected/brin.out
+++ b/contrib/pageinspect/expected/brin.out
@@ -48,4 +48,17 @@ SELECT * FROM brin_page_items(get_raw_page('test1_a_idx', 2), 'test1_a_idx')
1 | 0 | 1 | f | f | f | {1 .. 1}
(1 row)
+-- Check that all functions fail gracefully with an empty page imput
+select brin_page_type(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ brin_page_type
+----------------
+
+(1 row)
+
+select brin_metapage_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
+select brin_revmap_data(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
+select brin_page_items(repeat(E'\\000', current_setting('block_size')::int)::bytea, 'test1_a_idx');
+ERROR: input page is empty
DROP TABLE test1;
diff --git a/contrib/pageinspect/expected/btree.out b/contrib/pageinspect/expected/btree.out
index c60bc88560c..7c503877826 100644
--- a/contrib/pageinspect/expected/btree.out
+++ b/contrib/pageinspect/expected/btree.out
@@ -70,4 +70,9 @@ tids |
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2));
ERROR: block number 2 is out of range for relation "test1_a_idx"
+-- Check that bt_page_items() fails gracefully with an empty page input
+select bt_page_items(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+-[ RECORD 1 ]-+-
+bt_page_items |
+
DROP TABLE test1;
diff --git a/contrib/pageinspect/expected/checksum.out b/contrib/pageinspect/expected/checksum.out
index a85388e158e..033eb2ca2bc 100644
--- a/contrib/pageinspect/expected/checksum.out
+++ b/contrib/pageinspect/expected/checksum.out
@@ -38,3 +38,10 @@ SELECT blkno,
100 | 1139 | 28438 | 3648 | -30881 | -16305 | -27349
(3 rows)
+ --Check that page_checksum() with an empty page fails gracefully
+ select page_checksum(repeat(E'\\000', current_setting('block_size')::int)::bytea, 1);
+ page_checksum
+---------------
+
+(1 row)
+
diff --git a/contrib/pageinspect/expected/gin.out b/contrib/pageinspect/expected/gin.out
index ef7570b9723..a37a3749b80 100644
--- a/contrib/pageinspect/expected/gin.out
+++ b/contrib/pageinspect/expected/gin.out
@@ -35,4 +35,11 @@ FROM gin_leafpage_items(get_raw_page('test1_y_idx',
-[ RECORD 1 ]
?column? | t
+-- Check that all functions fail gracefully with an empty page imput
+select gin_metapage_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
+select gin_page_opaque_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
+select gin_leafpage_items(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
DROP TABLE test1;
diff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.out
index b26b56ba15d..d69d653abf5 100644
--- a/contrib/pageinspect/expected/gist.out
+++ b/contrib/pageinspect/expected/gist.out
@@ -64,4 +64,9 @@ SELECT itemoffset, ctid, itemlen FROM gist_page_items_bytea(get_raw_page('test_g
6 | (6,65535) | 40
(6 rows)
+-- Check that all functions fail gracefully with an empty page imput
+SELECT * FROM gist_page_opaque_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
+SELECT * FROM gist_page_items(repeat(E'\\000', current_setting('block_size')::int)::bytea, 'test_gist_idx');
+ERROR: input page is empty
DROP TABLE test_gist;
diff --git a/contrib/pageinspect/expected/hash.out b/contrib/pageinspect/expected/hash.out
index bd0628d0136..f8820f07c42 100644
--- a/contrib/pageinspect/expected/hash.out
+++ b/contrib/pageinspect/expected/hash.out
@@ -163,4 +163,13 @@ SELECT * FROM hash_page_items(get_raw_page('test_hash_a_idx', 4));
SELECT * FROM hash_page_items(get_raw_page('test_hash_a_idx', 5));
ERROR: page is not a hash bucket or overflow page
+-- Check that all functions fail gracefully with an empty page imput
+select hash_page_type(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
+select hash_metapage_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
+select hash_page_stats(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
+select hash_page_items(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+ERROR: input page is empty
DROP TABLE test_hash;
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 7e41af045f3..e1eb80ad3d4 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -230,6 +230,12 @@ get_page_from_raw(bytea *raw_page)
memcpy(page, VARDATA_ANY(raw_page), raw_page_size);
+ /* check that the page is initialized */
+ if (PageIsNew((Page) page))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("input page is empty")));
+
return page;
}
@@ -375,6 +381,10 @@ page_checksum_internal(PG_FUNCTION_ARGS, enum pageinspect_version ext_version)
page = (PageHeader) VARDATA(raw_page);
+ /* check that the page is initialized */
+ if (PageIsNew((Page) page))
+ PG_RETURN_NULL();
+
PG_RETURN_INT16(pg_checksum_page((char *) page, blkno));
}
diff --git a/contrib/pageinspect/sql/brin.sql b/contrib/pageinspect/sql/brin.sql
index 735bc3b6733..65da9403995 100644
--- a/contrib/pageinspect/sql/brin.sql
+++ b/contrib/pageinspect/sql/brin.sql
@@ -15,4 +15,10 @@ SELECT * FROM brin_revmap_data(get_raw_page('test1_a_idx', 1)) LIMIT 5;
SELECT * FROM brin_page_items(get_raw_page('test1_a_idx', 2), 'test1_a_idx')
ORDER BY blknum, attnum LIMIT 5;
+-- Check that all functions fail gracefully with an empty page imput
+select brin_page_type(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+select brin_metapage_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+select brin_revmap_data(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+select brin_page_items(repeat(E'\\000', current_setting('block_size')::int)::bytea, 'test1_a_idx');
+
DROP TABLE test1;
diff --git a/contrib/pageinspect/sql/btree.sql b/contrib/pageinspect/sql/btree.sql
index 96359179597..e35e894c531 100644
--- a/contrib/pageinspect/sql/btree.sql
+++ b/contrib/pageinspect/sql/btree.sql
@@ -21,4 +21,7 @@ SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 0));
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 1));
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2));
+-- Check that bt_page_items() fails gracefully with an empty page input
+select bt_page_items(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+
DROP TABLE test1;
diff --git a/contrib/pageinspect/sql/checksum.sql b/contrib/pageinspect/sql/checksum.sql
index b877db0611f..4c1922dc09a 100644
--- a/contrib/pageinspect/sql/checksum.sql
+++ b/contrib/pageinspect/sql/checksum.sql
@@ -29,3 +29,6 @@ SELECT blkno,
page_checksum(decode(repeat('e6d6', :block_size / 2), 'hex'), blkno) AS checksum_e6d6,
page_checksum(decode(repeat('4a5e', :block_size / 2), 'hex'), blkno) AS checksum_4a5e
FROM generate_series(0, 100, 50) AS a (blkno);
+
+ --Check that page_checksum() with an empty page fails gracefully
+ select page_checksum(repeat(E'\\000', current_setting('block_size')::int)::bytea, 1);
diff --git a/contrib/pageinspect/sql/gin.sql b/contrib/pageinspect/sql/gin.sql
index 423f5c57499..02826023d3e 100644
--- a/contrib/pageinspect/sql/gin.sql
+++ b/contrib/pageinspect/sql/gin.sql
@@ -18,4 +18,9 @@ FROM gin_leafpage_items(get_raw_page('test1_y_idx',
(pg_relation_size('test1_y_idx') /
current_setting('block_size')::bigint)::int - 1));
+-- Check that all functions fail gracefully with an empty page imput
+select gin_metapage_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+select gin_page_opaque_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+select gin_leafpage_items(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+
DROP TABLE test1;
diff --git a/contrib/pageinspect/sql/gist.sql b/contrib/pageinspect/sql/gist.sql
index 1560d1e15c3..d3277ba71d5 100644
--- a/contrib/pageinspect/sql/gist.sql
+++ b/contrib/pageinspect/sql/gist.sql
@@ -26,4 +26,9 @@ SELECT * FROM gist_page_items(get_raw_page('test_gist_idx', 1), 'test_gist_idx')
-- platform-dependent (endianess), so omit the actual key data from the output.
SELECT itemoffset, ctid, itemlen FROM gist_page_items_bytea(get_raw_page('test_gist_idx', 0));
+-- Check that all functions fail gracefully with an empty page imput
+SELECT * FROM gist_page_opaque_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+SELECT * FROM gist_page_items(repeat(E'\\000', current_setting('block_size')::int)::bytea, 'test_gist_idx');
+
+
DROP TABLE test_gist;
diff --git a/contrib/pageinspect/sql/hash.sql b/contrib/pageinspect/sql/hash.sql
index 64f33f1d52f..dba5b5f9c33 100644
--- a/contrib/pageinspect/sql/hash.sql
+++ b/contrib/pageinspect/sql/hash.sql
@@ -78,5 +78,11 @@ SELECT * FROM hash_page_items(get_raw_page('test_hash_a_idx', 3));
SELECT * FROM hash_page_items(get_raw_page('test_hash_a_idx', 4));
SELECT * FROM hash_page_items(get_raw_page('test_hash_a_idx', 5));
+-- Check that all functions fail gracefully with an empty page imput
+select hash_page_type(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+select hash_metapage_info(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+select hash_page_stats(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+select hash_page_items(repeat(E'\\000', current_setting('block_size')::int)::bytea);
+
DROP TABLE test_hash;
--
2.25.1
