From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Richard Guo <guofenglinux@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Date: Mon, 19 May 2025 17:17:55 +0900
Message-ID: <d9e0389dad5767fb608eedbff001d56e@oss.nttdata.com> (raw)
In-Reply-To: <5a494c43-326e-45c3-b391-e95501152bb2@oss.nttdata.com>
References: <e082e6027610fd0a4091ae6d033aa117@oss.nttdata.com>
<CAFiTN-tKxhMG0QNO1V3VNsfnhtkS=hrOJNj5Kxs6cGKtujdyhg@mail.gmail.com>
<7fde0acf-2395-44d8-b081-bfa0e10097dc@oss.nttdata.com>
<CAFiTN-vDKGHn76XkL2oTQVf5aCScKqs1GKf_aALGO5u7fuqNwg@mail.gmail.com>
<CAMbWs49T8hFr571c+b0T4GiDwKow5B1HUMMxav7qhUt75WHpKA@mail.gmail.com>
<8829854bbfc8635ddecd0846bb72dfda@oss.nttdata.com>
<CAFiTN-s_34r+vTWUdnyfAMA4F07QEdPOhAKkRVzwgZeeJPw3Yw@mail.gmail.com>
<5a494c43-326e-45c3-b391-e95501152bb2@oss.nttdata.com>
Thanks for your work and feedback!
I've updated the patches and added regular regression tests for
both pg_prewarm and amcheck.
On 2025-05-16 21:01, Fujii Masao wrote:
> Also, since the issue was introduced in v17, this patch should be
> back-patched to v17, right?
I agree with back-patching v3-0001. I was able to reproduce the issue
on the REL_17_STABLE branch. One concern is that this patch changes
the error message in production:
* v17.5 (without --enable-cassert)
> ERROR: fork "main" does not exist for this relation
* REL_17_STABLE with the v3-0001 patch (without --enable-cassert)
> ERROR: relation "test" does not have storage
However, I think preventing the assertion failure should take priority.
As for v3-0002, I think it's fine to apply it to the master branch only,
since it just makes a cosmetic improvement to the error message.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Attachments:download all (2 files, .tar.gz)v3-0001-Fix-assertion-failure-when-pg_prewarm-is-run-on-o.patch (text/x-diff, 3.9K) download
From 16027ecedc4301904965306d8a495f7a08c6d798 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Mon, 19 May 2025 16:24:55 +0900
Subject: [PATCH v3 1/2] Fix assertion failure when pg_prewarm() is run on
objects that don't have storage.
This issue was introduced by commit 049ef33.
Specifying objects that don't have storage as an argument to
pg_prewarm() could trigger an assertion failure:
Failed Assert("RelFileNumberIsValid(rlocator.relNumber)")
This fix ensures that such cases are handled appropriately.
---
contrib/pg_prewarm/Makefile | 2 ++
contrib/pg_prewarm/expected/pg_prewarm.out | 12 ++++++++++++
contrib/pg_prewarm/meson.build | 5 +++++
contrib/pg_prewarm/pg_prewarm.c | 8 ++++++++
contrib/pg_prewarm/sql/pg_prewarm.sql | 12 ++++++++++++
5 files changed, 39 insertions(+)
create mode 100644 contrib/pg_prewarm/expected/pg_prewarm.out
create mode 100644 contrib/pg_prewarm/sql/pg_prewarm.sql
diff --git a/contrib/pg_prewarm/Makefile b/contrib/pg_prewarm/Makefileindex 9cfde8c4e4f..617ac8e09b2 100644--- a/contrib/pg_prewarm/Makefile+++ b/contrib/pg_prewarm/Makefile@@ -10,6 +10,8 @@ EXTENSION = pg_prewarm
DATA = pg_prewarm--1.1--1.2.sql pg_prewarm--1.1.sql pg_prewarm--1.0--1.1.sql
PGFILEDESC = "pg_prewarm - preload relation data into system buffer cache"
+REGRESS = pg_prewarm+
TAP_TESTS = 1
ifdef USE_PGXS
diff --git a/contrib/pg_prewarm/expected/pg_prewarm.out b/contrib/pg_prewarm/expected/pg_prewarm.outnew file mode 100644index 00000000000..3bb644499d4--- /dev/null+++ b/contrib/pg_prewarm/expected/pg_prewarm.out@@ -0,0 +1,12 @@+-- Test pg_prewarm extension+CREATE EXTENSION pg_prewarm;+-- To specify the relation which does not have storage should fail.+CREATE TABLE test (c1 int) PARTITION BY RANGE (c1);+CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000);+INSERT INTO test SELECT generate_series(1, 100);+SELECT pg_prewarm('test', 'buffer');+ERROR: relation "test" does not have storage+DETAIL: This operation is not supported for partitioned tables.+-- Cleanup+DROP TABLE test;+DROP EXTENSION pg_prewarm;diff --git a/contrib/pg_prewarm/meson.build b/contrib/pg_prewarm/meson.buildindex 82b9851303c..f24c47ef6a5 100644--- a/contrib/pg_prewarm/meson.build+++ b/contrib/pg_prewarm/meson.build@@ -29,6 +29,11 @@ tests += {
'name': 'pg_prewarm',
'sd': meson.current_source_dir(),
'bd': meson.current_build_dir(),
+ 'regress': {+ 'sql': [+ 'pg_prewarm',+ ],+ },
'tap': {
'tests': [
't/001_basic.pl',
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.cindex 50808569bd7..1f89c9adc86 100644--- a/contrib/pg_prewarm/pg_prewarm.c+++ b/contrib/pg_prewarm/pg_prewarm.c@@ -112,6 +112,14 @@ pg_prewarm(PG_FUNCTION_ARGS)
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
+ /* Check that the storage exists. */+ if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))+ ereport(ERROR,+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),+ errmsg("relation \"%s\" does not have storage",+ RelationGetRelationName(rel)),+ errdetail_relkind_not_supported(rel->rd_rel->relkind)));+
/* Check that the fork exists. */
if (!smgrexists(RelationGetSmgr(rel), forkNumber))
ereport(ERROR,
diff --git a/contrib/pg_prewarm/sql/pg_prewarm.sql b/contrib/pg_prewarm/sql/pg_prewarm.sqlnew file mode 100644index 00000000000..2743fa5e778--- /dev/null+++ b/contrib/pg_prewarm/sql/pg_prewarm.sql@@ -0,0 +1,12 @@+-- Test pg_prewarm extension+CREATE EXTENSION pg_prewarm;++-- To specify the relation which does not have storage should fail.+CREATE TABLE test (c1 int) PARTITION BY RANGE (c1);+CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000);+INSERT INTO test SELECT generate_series(1, 100);+SELECT pg_prewarm('test', 'buffer');++-- Cleanup+DROP TABLE test;+DROP EXTENSION pg_prewarm;--
2.34.1
v3-0002-Fix-error-message-details-for-partitioned-indexes.patch (text/x-diff, 3.6K) download
From 500945b94f053c1230830e1211a727edc6e1f01b Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Mon, 19 May 2025 16:39:27 +0900
Subject: [PATCH v3 2/2] Fix error message details for partitioned indexes in
amcheck
Until now, the error detail could be misleading when bt_index_check()
is run on partitioned indexes. For example, the index
"pgbench_accounts_pkey" is both a btree and a partitioned index.
Before this change, the error message was:
> ERROR: expected "btree" index as targets for verification
> DETAIL: Relation "pgbench_accounts_pkey" is a btree index.
This change adds the word "partitioned" to the error detail to avoid
confusion about why the error occurred.
After this change, the error message becomes:
> ERROR: expected "btree" index as targets for verification
> DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned index.
---
contrib/amcheck/expected/check_btree.out | 8 ++++++++
contrib/amcheck/sql/check_btree.sql | 7 +++++++
contrib/amcheck/verify_common.c | 5 +++--
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.outindex c6f4b16c556..8947496d890 100644--- a/contrib/amcheck/expected/check_btree.out+++ b/contrib/amcheck/expected/check_btree.out@@ -60,6 +60,14 @@ SELECT bt_index_parent_check('bttest_a_brin_idx');
ERROR: expected "btree" index as targets for verification
DETAIL: Relation "bttest_a_brin_idx" is a brin index.
ROLLBACK;
+-- verify btree partitioned indexes are rejected (error)+BEGIN;+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');+ERROR: expected "btree" index as targets for verification+DETAIL: Relation "bttest_btree_partitioned_idx" is a btree partitioned index.+ROLLBACK;
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
bt_index_check
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sqlindex 0793dbfeebd..07750084b3a 100644--- a/contrib/amcheck/sql/check_btree.sql+++ b/contrib/amcheck/sql/check_btree.sql@@ -52,6 +52,13 @@ CREATE INDEX bttest_a_brin_idx ON bttest_a USING brin(id);
SELECT bt_index_parent_check('bttest_a_brin_idx');
ROLLBACK;
+-- verify btree partitioned indexes are rejected (error)+BEGIN;+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');+ROLLBACK;+
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
-- more expansive tests
diff --git a/contrib/amcheck/verify_common.c b/contrib/amcheck/verify_common.cindex d095e62ce55..b68ddaf895d 100644--- a/contrib/amcheck/verify_common.c+++ b/contrib/amcheck/verify_common.c@@ -169,8 +169,9 @@ index_checkable(Relation rel, Oid am_id)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("expected \"%s\" index as targets for verification", NameStr(((Form_pg_am) GETSTRUCT(amtup))->amname)),
- errdetail("Relation \"%s\" is a %s index.",- RelationGetRelationName(rel), NameStr(((Form_pg_am) GETSTRUCT(amtuprel))->amname))));+ errdetail("Relation \"%s\" is a %s %sindex.",+ RelationGetRelationName(rel), NameStr(((Form_pg_am) GETSTRUCT(amtuprel))->amname),+ (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) ? "partitioned " : "")));
}
if (RELATION_IS_OTHER_TEMP(rel))
--
2.34.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ikedamsh@oss.nttdata.com, masao.fujii@oss.nttdata.com, dilipbalaut@gmail.com, guofenglinux@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
In-Reply-To: <d9e0389dad5767fb608eedbff001d56e@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox