Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
amcheck: Improve error message for partitioned index target.
- 88a658a42e1b 19 (unreleased) landed
-
Fix assertion failure in pg_prewarm() on objects without storage.
- b64c585fde77 17.6 landed
- 3c4d7557e03b 18.0 landed
-
Prevent assertion failure in contrib/pg_freespacemap.
- 4623d71443de 18.0 cited
-
Don't try to open visibilitymap when analyzing a foreign table
- 049ef3398d05 17.0 cited
-
Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2025-05-15T08:52:06Z
Hi, I encountered an assertion failure when a partitioned table is specified as an argument to pg_prewarm. Below are the steps to reproduce the issue: $ pgbench -i -s 1 --partitions=3 $ psql <<EOF CREATE EXTENSION pg_prewarm; SELECT pg_prewarm('pgbench_accounts'); EOF The following assertion failure occurs: TRAP: failed Assert("RelFileNumberIsValid(rlocator.relNumber)"), File: "smgr.c", Line: 246, PID: 1246282 postgres: ikeda postgres [local] SELECT(ExceptionalCondition+0xbb)[0x55edd16725c1] postgres: ikeda postgres [local] SELECT(smgropen+0x5e)[0x55edd145c1ff] It looks like this may have been overlooked in commit 049ef33. What do you think? Regards, -- Masahiro Ikeda NTT DATA CORPORATION -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Dilip Kumar <dilipbalaut@gmail.com> — 2025-05-15T09:20:30Z
On Thu, May 15, 2025 at 2:22 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: > > Hi, > > I encountered an assertion failure when a partitioned table is specified > as an argument to pg_prewarm. Below are the steps to reproduce the > issue: > > $ pgbench -i -s 1 --partitions=3 > $ psql <<EOF > CREATE EXTENSION pg_prewarm; > SELECT pg_prewarm('pgbench_accounts'); > EOF > > The following assertion failure occurs: > > TRAP: failed Assert("RelFileNumberIsValid(rlocator.relNumber)"), File: > "smgr.c", Line: 246, PID: 1246282 > postgres: ikeda postgres [local] > SELECT(ExceptionalCondition+0xbb)[0x55edd16725c1] > postgres: ikeda postgres [local] SELECT(smgropen+0x5e)[0x55edd145c1ff] > > > It looks like this may have been overlooked in commit 049ef33. > What do you think? Yeah, this should be fixed, don't you think that instead of checking the relnumber is valid, we shall check whether it's a partitioned rel and give a separate error that prewarm is not supported for partitioned tables? And in fact, we can think of supporting this for the partitioned tables as well in the future, where we can loop through all the partitions and do prewarm for each of them. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-05-15T09:44:29Z
On 2025/05/15 18:20, Dilip Kumar wrote: > On Thu, May 15, 2025 at 2:22 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: >> >> Hi, >> >> I encountered an assertion failure when a partitioned table is specified >> as an argument to pg_prewarm. Below are the steps to reproduce the >> issue: Thanks for the report! This assertion failure can also occur when pg_prewarm() is run on objects like foreign tables, plain views, or other relations that don't have storage - not just partitioned tables. >> $ pgbench -i -s 1 --partitions=3 >> $ psql <<EOF >> CREATE EXTENSION pg_prewarm; >> SELECT pg_prewarm('pgbench_accounts'); >> EOF >> >> The following assertion failure occurs: >> >> TRAP: failed Assert("RelFileNumberIsValid(rlocator.relNumber)"), File: >> "smgr.c", Line: 246, PID: 1246282 >> postgres: ikeda postgres [local] >> SELECT(ExceptionalCondition+0xbb)[0x55edd16725c1] >> postgres: ikeda postgres [local] SELECT(smgropen+0x5e)[0x55edd145c1ff] >> >> >> It looks like this may have been overlooked in commit 049ef33. >> What do you think? > > Yeah, this should be fixed, don't you think that instead of checking > the relnumber is valid, +1 How about adding a check to see whether the target relation has storage, using something like RELKIND_HAS_STORAGE()? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Dilip Kumar <dilipbalaut@gmail.com> — 2025-05-15T09:50:26Z
On Thu, May 15, 2025 at 3:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2025/05/15 18:20, Dilip Kumar wrote: > > On Thu, May 15, 2025 at 2:22 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: > >> > >> Hi, > >> > >> I encountered an assertion failure when a partitioned table is specified > >> as an argument to pg_prewarm. Below are the steps to reproduce the > >> issue: > > Thanks for the report! > > This assertion failure can also occur when pg_prewarm() is run on objects like > foreign tables, plain views, or other relations that don't have storage - not just > partitioned tables. > > > >> $ pgbench -i -s 1 --partitions=3 > >> $ psql <<EOF > >> CREATE EXTENSION pg_prewarm; > >> SELECT pg_prewarm('pgbench_accounts'); > >> EOF > >> > >> The following assertion failure occurs: > >> > >> TRAP: failed Assert("RelFileNumberIsValid(rlocator.relNumber)"), File: > >> "smgr.c", Line: 246, PID: 1246282 > >> postgres: ikeda postgres [local] > >> SELECT(ExceptionalCondition+0xbb)[0x55edd16725c1] > >> postgres: ikeda postgres [local] SELECT(smgropen+0x5e)[0x55edd145c1ff] > >> > >> > >> It looks like this may have been overlooked in commit 049ef33. > >> What do you think? > > > > Yeah, this should be fixed, don't you think that instead of checking > > the relnumber is valid, > > +1 > > How about adding a check to see whether the target relation has storage, > using something like RELKIND_HAS_STORAGE()? Yeah, that makes more sense. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Richard Guo <guofenglinux@gmail.com> — 2025-05-15T10:57:27Z
On Thu, May 15, 2025 at 6:50 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Thu, May 15, 2025 at 3:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > How about adding a check to see whether the target relation has storage, > > using something like RELKIND_HAS_STORAGE()? > Yeah, that makes more sense. +1. FWIW, not long ago we fixed a similar Assert failure in contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before trying to access the storage (see 4623d7144). Wondering if there are other similar issues elsewhere in contrib ... Thanks Richard
-
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2025-05-16T06:21:11Z
Thank you for your comments! I updated the patch to use RELKIND_HAS_STORAGE() as done in commit 4623d7144. Please see the v2-0001 patch for the changes. On 2025-05-15 19:57, Richard Guo wrote: > +1. FWIW, not long ago we fixed a similar Assert failure in > contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before > trying to access the storage (see 4623d7144). Wondering if there are > other similar issues elsewhere in contrib ... I tested all contrib functions that take regclass arguments (see attached test.sql and test_result.txt). The result shows that only pg_prewarm() can lead to the assertion failure. However, I found that amcheck's error messages can be misleading when run on partitioned indexes. For example, on the master branch, amcheck (e.g., bt_index_check function) shows this error: > ERROR: expected "btree" index as targets for verification > DETAIL: Relation "pgbench_accounts_pkey" is a btree index. This message says it expects a "btree" index, yet also states the relation is a "btree" index, which can seem contradictory. The actual issue is that the index is a btree partitioned index, but this detail is missing, causing possible confusion. Therefore, I thought it would be better to update the detailed message as shown in the v2-0002 patch: * master with 0002 patch > ERROR: expected "btree" index as targets for verification > DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned index. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
-
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Dilip Kumar <dilipbalaut@gmail.com> — 2025-05-16T06:33:22Z
On Fri, May 16, 2025 at 11:51 AM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: > > Thank you for your comments! > > I updated the patch to use RELKIND_HAS_STORAGE() as done in > commit 4623d7144. Please see the v2-0001 patch for the changes. > > On 2025-05-15 19:57, Richard Guo wrote: > > +1. FWIW, not long ago we fixed a similar Assert failure in > > contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before > > trying to access the storage (see 4623d7144). Wondering if there are > > other similar issues elsewhere in contrib ... > > I tested all contrib functions that take regclass arguments (see > attached test.sql and test_result.txt). The result shows that only > pg_prewarm() can lead to the assertion failure. Right, even while I was searching, I see this is used in 3 contrib modules, amcheck, autoprewarm, and pg_prewarm. amcheck is already checking for AM type, and Autoprewarm is identifying the relation by block, so there is no chance of getting the relation which do not have storage. > > However, I found that amcheck's error messages can be misleading > when run on partitioned indexes. > > For example, on the master branch, amcheck (e.g., bt_index_check > function) > shows this error: > > > ERROR: expected "btree" index as targets for verification > > DETAIL: Relation "pgbench_accounts_pkey" is a btree index. > > This message says it expects a "btree" index, yet also states the > relation > is a "btree" index, which can seem contradictory. The actual issue is > that > the index is a btree partitioned index, but this detail is missing, > causing > possible confusion. Yes, I found the error message confusing during testing as well, so it makes sense to improve it. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-05-16T12:01:24Z
On 2025/05/16 15:33, Dilip Kumar wrote: > On Fri, May 16, 2025 at 11:51 AM Masahiro Ikeda > <ikedamsh@oss.nttdata.com> wrote: >> >> Thank you for your comments! >> >> I updated the patch to use RELKIND_HAS_STORAGE() as done in >> commit 4623d7144. Please see the v2-0001 patch for the changes. Thanks for updating the patch! You added a test for pg_prewarm() with a partitioned table in the TAP tests. But, wouldn't it be simpler and easier to maintain if we added this as a regular regression test (i.e., using .sql and .out files) instead of TAP tests? That should be sufficient for this case? Also, since the issue was introduced in v17, this patch should be back-patched to v17, right? >> On 2025-05-15 19:57, Richard Guo wrote: >>> +1. FWIW, not long ago we fixed a similar Assert failure in >>> contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before >>> trying to access the storage (see 4623d7144). Wondering if there are >>> other similar issues elsewhere in contrib ... >> >> I tested all contrib functions that take regclass arguments (see >> attached test.sql and test_result.txt). The result shows that only >> pg_prewarm() can lead to the assertion failure. > > Right, even while I was searching, I see this is used in 3 contrib > modules, amcheck, autoprewarm, and pg_prewarm. amcheck is already > checking for AM type, and Autoprewarm is identifying the relation by > block, so there is no chance of getting the relation which do not have > storage. > >> >> However, I found that amcheck's error messages can be misleading >> when run on partitioned indexes. >> >> For example, on the master branch, amcheck (e.g., bt_index_check >> function) >> shows this error: >> >> > ERROR: expected "btree" index as targets for verification >> > DETAIL: Relation "pgbench_accounts_pkey" is a btree index. >> >> This message says it expects a "btree" index, yet also states the >> relation >> is a "btree" index, which can seem contradictory. The actual issue is >> that >> the index is a btree partitioned index, but this detail is missing, >> causing >> possible confusion. > > Yes, I found the error message confusing during testing as well, so it > makes sense to improve it. +1 Regarding the patch, how about also adding a regression test for amcheck with a partitioned index? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
-
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2025-05-19T08:17:55Z
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
-
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Fujii Masao <masao.fujii@gmail.com> — 2025-05-21T03:54:36Z
On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: > > Thanks for your work and feedback! > > I've updated the patches and added regular regression tests for > both pg_prewarm and amcheck. Thanks for updating the patches! Regarding the 0001 patch: +CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000); +INSERT INTO test SELECT generate_series(1, 100); These lines don't seem necessary for the test. How about removing them? It would simplify the test and slightly reduce the execution time - though the time savings are very minimal. +-- To specify the relation which does not have storage should fail. This comment could be clearer as: "pg_prewarm() should fail if the target relation has no storage." + /* Check that the storage exists. */ Maybe rephrase to: "Check that the relation has storage."? Regarding the 0002 patch: - 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 " : ""))); Instead of manually building the message, how about using errdetail_relkind_not_supported()? It would be more consistent with similar error reporting elsewhere. > I agree with back-patching v3-0001. I was able to reproduce the issue > on the REL_17_STABLE branch. Thanks for the confirmation. > 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. Yes. Regards, -- Fujii Masao -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2025-05-26T07:55:33Z
<div dir='auto'><div><div><div class="elided-text">2025/05/21 12:54 Fujii Masao <masao.fujii@gmail.com>:<br type="attribution"><blockquote style="margin:0 0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: <br> > <br> > Thanks for your work and feedback! <br> > <br> > I've updated the patches and added regular regression tests for <br> > both pg_prewarm and amcheck. <br> <br> Thanks for updating the patches! <br> <br> Regarding the 0001 patch: <br> <br> +CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000); <br> +INSERT INTO test SELECT generate_series(1, 100); <br> <br> These lines don't seem necessary for the test. How about removing them? <br> It would simplify the test and slightly reduce the execution time - though <br> the time savings are very minimal. <br> <br> +-- To specify the relation which does not have storage should fail. <br> <br> This comment could be clearer as: <br> "pg_prewarm() should fail if the target relation has no storage." <br> <br> + /* Check that the storage exists. */ <br> <br> Maybe rephrase to: <br> "Check that the relation has storage." ?<br></p></blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">Thanks! I will fix them.</div><div dir="auto"><br></div><div dir="auto"><div><div class="elided-text"><blockquote style="margin:0 0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr"> Regarding the 0002 patch: <br> <br> - errdetail("Relation \"%s\" is a %s index.", <br> - RelationGetRelationName(rel), NameStr(((Form_pg_am) <br> GETSTRUCT(amtuprel))->amname)))); <br> + errdetail("Relation \"%s\" is a %s %sindex.", <br> + RelationGetRelationName(rel), NameStr(((Form_pg_am) <br> GETSTRUCT(amtuprel))->amname), <br> + (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) ? <br> "partitioned " : ""))); <br> <br> Instead of manually building the message, how about using <br> errdetail_relkind_not_supported()? <br> It would be more consistent with similar error reporting elsewhere.</p></blockquote></div></div></div><div dir="auto">I was thinking of using errdetail_relkind_not_supported(),</div><div dir="auto">but I’m reconsidering building the message manually</div><div dir="auto">since the AM name seems to be important for the error.</div><div dir="auto">What do you think?</div><div dir="auto"><br></div><div dir="auto">Regards,</div><div dir="auto">--</div><div dir="auto">Masahiro Ikeda</div><div dir="auto"><br></div><div dir="auto"><div><br></div></div></div> -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-05-28T01:10:12Z
On 2025/05/26 16:55, ikedamsh wrote: > 2025/05/21 12:54 Fujii Masao <masao.fujii@gmail.com>: > > On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: > > > > Thanks for your work and feedback! > > > > I've updated the patches and added regular regression tests for > > both pg_prewarm and amcheck. > > Thanks for updating the patches! > > Regarding the 0001 patch: > > +CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000); > +INSERT INTO test SELECT generate_series(1, 100); > > These lines don't seem necessary for the test. How about removing them? > It would simplify the test and slightly reduce the execution time - though > the time savings are very minimal. > > +-- To specify the relation which does not have storage should fail. > > This comment could be clearer as: > "pg_prewarm() should fail if the target relation has no storage." > > + /* Check that the storage exists. */ > > Maybe rephrase to: > "Check that the relation has storage." ? > > > Thanks! I will fix them. Thanks! > Regarding the 0002 patch: > > - 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 " : ""))); > > Instead of manually building the message, how about using > errdetail_relkind_not_supported()? > It would be more consistent with similar error reporting elsewhere. > > I was thinking of using errdetail_relkind_not_supported(), > but I’m reconsidering building the message manually > since the AM name seems to be important for the error. > What do you think? Understood. I was trying to better understand the error message, as I found the following is still a bit confusing for users. However, I don't have a better suggestion at the moment, so I'm okay with the proposed change. ERROR: expected "btree" index as targets for verification DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned This is not directly relation to your proposal, but while reading the index_checkable() function, I noticed that ReleaseSysCache() is not called after SearchSysCache1(). Shouldn't we call ReleaseSysCache() here? Alternatively, we could use get_am_name() instead of SearchSysCache1(), which might be simpler. I also observed that the error code ERRCODE_FEATURE_NOT_SUPPORTED is used when the relation is not the expected type in index_checkable(). However, based on similar cases (e.g., pgstattuple), it seems that ERRCODE_WRONG_OBJECT_TYPE might be more appropriate in this situation. Thought? Regards, -- Fujii Masao NTT DATA Japan Corporation -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2025-05-29T02:46:41Z
Thanks for your feedback. I've attached the updated patches. On 2025-05-28 10:10, Fujii Masao wrote: > On 2025/05/26 16:55, ikedamsh wrote: >> 2025/05/21 12:54 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>: >> Regarding the 0002 patch: >> >> - 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 " : ""))); >> >> Instead of manually building the message, how about using >> errdetail_relkind_not_supported()? >> It would be more consistent with similar error reporting >> elsewhere. >> >> I was thinking of using errdetail_relkind_not_supported(), >> but I’m reconsidering building the message manually >> since the AM name seems to be important for the error. >> What do you think? > > Understood. > I was trying to better understand the error message, as I found > the following is still a bit confusing for users. However, I don't > have a better suggestion at the moment, so I'm okay with > the proposed change. > > ERROR: expected "btree" index as targets for verification > DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned What do you think about adding new error messages specifically for partitioned indexes? If the target is a partitioned index, the error messages would be: ERROR: expected index as targets for verification DETAIL: This operation is not supported for partitioned indexes. If the target is an index, but its access method is not supported, the error messages would be: ERROR: expected "btree" index as targets for verification DETAIL: Relation "bttest_a_brin_idx" is a brin index. > This is not directly relation to your proposal, but while reading > the index_checkable() function, I noticed that ReleaseSysCache() > is not called after SearchSysCache1(). Shouldn't we call > ReleaseSysCache() here? Alternatively, we could use get_am_name() > instead of SearchSysCache1(), which might be simpler. Agreed. > I also observed that the error code ERRCODE_FEATURE_NOT_SUPPORTED > is used when the relation is not the expected type in > index_checkable(). > However, based on similar cases (e.g., pgstattuple), it seems that > ERRCODE_WRONG_OBJECT_TYPE might be more appropriate in this situation. > Thought? Agreed. I also change the error code to ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when the index is not valid. Regards, -- Masahiro Ikeda NTT DATA Japan Corporation -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-05-29T16:44:25Z
On 2025/05/29 11:46, Masahiro Ikeda wrote: > Thanks for your feedback. I've attached the updated patches. I've pushed the 0001 patch. Thanks! > What do you think about adding new error messages specifically for partitioned > indexes? > > If the target is a partitioned index, the error messages would be: > > ERROR: expected index as targets for verification > DETAIL: This operation is not supported for partitioned indexes. > > If the target is an index, but its access method is not supported, the error > messages would be: > > ERROR: expected "btree" index as targets for verification > DETAIL: Relation "bttest_a_brin_idx" is a brin index. Thanks for considering this. The proposed messages look good to me. >> This is not directly relation to your proposal, but while reading >> the index_checkable() function, I noticed that ReleaseSysCache() >> is not called after SearchSysCache1(). Shouldn't we call >> ReleaseSysCache() here? Alternatively, we could use get_am_name() >> instead of SearchSysCache1(), which might be simpler. > > Agreed. I may have been mistaken earlier. Based on the comment in SearchSysCache(), the tuple returned by SearchSysCache1() is valid until the end of the transaction. Since index_checkable() raises an error and ends the transaction immediately after calling SearchSysCache1(), it seems safe to skip ReleaseSysCache() in this case. Using get_am_name() instead seems simpler, though. Thought? >> I also observed that the error code ERRCODE_FEATURE_NOT_SUPPORTED >> is used when the relation is not the expected type in index_checkable(). >> However, based on similar cases (e.g., pgstattuple), it seems that >> ERRCODE_WRONG_OBJECT_TYPE might be more appropriate in this situation. >> Thought? > > Agreed. I also change the error code to ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE > when the index is not valid. +1. Should we commit patch 0003 before 0002? Also, should we consider back-patching it? Regards, -- Fujii Masao NTT DATA Japan Corporation
-
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2025-06-02T07:32:40Z
On 2025-05-30 01:44, Fujii Masao wrote: > I've pushed the 0001 patch. Thanks! Thanks! >>> This is not directly relation to your proposal, but while reading >>> the index_checkable() function, I noticed that ReleaseSysCache() >>> is not called after SearchSysCache1(). Shouldn't we call >>> ReleaseSysCache() here? Alternatively, we could use get_am_name() >>> instead of SearchSysCache1(), which might be simpler. >> >> Agreed. > > I may have been mistaken earlier. Based on the comment in > SearchSysCache(), > the tuple returned by SearchSysCache1() is valid until the end of the > transaction. > Since index_checkable() raises an error and ends the transaction > immediately > after calling SearchSysCache1(), it seems safe to skip > ReleaseSysCache() > in this case. Using get_am_name() instead seems simpler, though. > Thought? As you said, it seems safe since SearchSysCache() is only used for constructing the error message. However, using get_am_name() is simpler and cleaner. >>> I also observed that the error code ERRCODE_FEATURE_NOT_SUPPORTED >>> is used when the relation is not the expected type in >>> index_checkable(). >>> However, based on similar cases (e.g., pgstattuple), it seems that >>> ERRCODE_WRONG_OBJECT_TYPE might be more appropriate in this >>> situation. >>> Thought? >> >> Agreed. I also change the error code to >> ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE >> when the index is not valid. > > +1. > Should we commit patch 0003 before 0002? Also, should we consider > back-patching it? OK, I think v5-0002 should be back-patched, since using incorrect error codes is essentially a bug. Regards, -- Masahiro Ikeda NTT DATA Japan Corporation
-
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-04T11:32:37Z
On 2025/06/02 16:32, Masahiro Ikeda wrote: > OK, I think v5-0002 should be back-patched, since using incorrect error codes is essentially a bug. Thanks for updating the patches! While reviewing the code: amtup = SearchSysCache1(AMOID, ObjectIdGetDatum(am_id)); amtuprel = SearchSysCache1(AMOID, ObjectIdGetDatum(rel->rd_rel->relam)); ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + (errcode(ERRCODE_WRONG_OBJECT_TYPE), 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)))); I initially thought switching to ERRCODE_WRONG_OBJECT_TYPE made sense, but after checking similar cases, I'm reconsidering. For instance, pgstat_relation() in pgstattuple.c uses ERRCODE_FEATURE_NOT_SUPPORTED when the access method is unexpected. That suggests using FEATURE_NOT_SUPPORTED here may not be incorrect. if (!rel->rd_index->indisvalid) ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot check index \"%s\"", RelationGetRelationName(rel)), errdetail("Index is not valid."))); Other parts of the codebase (including pgstattuple) use ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when rejecting invalid index. So, changing to this error code still seems reasonable and more consistent overall. Given that, I'd like to merge just this error code change from the 0002 patch into 0003, and apply the combined patch to the master branch only. Although I briefly considered backpatching the change of error code, it could be seen as a behavioral change, and the existing error code doesn't cause any real problem in older branches. So it's probably better to leave it as-is there. The updated patch is attached. It also changes index_checkable() from extern to static, since it's only used within verify_common.c. Thoughts? Regards, -- Fujii Masao NTT DATA Japan Corporation -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2025-07-14T08:13:15Z
On 2025-07-04 20:32, Fujii Masao wrote: > On 2025/06/02 16:32, Masahiro Ikeda wrote: >> OK, I think v5-0002 should be back-patched, since using incorrect >> error codes is essentially a bug. > > Thanks for updating the patches! > > While reviewing the code: > > amtup = SearchSysCache1(AMOID, > ObjectIdGetDatum(am_id)); > amtuprel = SearchSysCache1(AMOID, > ObjectIdGetDatum(rel->rd_rel->relam)); > ereport(ERROR, > - > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + (errcode(ERRCODE_WRONG_OBJECT_TYPE), > 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)))); > > I initially thought switching to ERRCODE_WRONG_OBJECT_TYPE made sense, > but after checking similar cases, I'm reconsidering. For instance, > pgstat_relation() in pgstattuple.c uses ERRCODE_FEATURE_NOT_SUPPORTED > when the access method is unexpected. That suggests using > FEATURE_NOT_SUPPORTED here may not be incorrect. OK, that seems reasonable to me. > if (!rel->rd_index->indisvalid) > ereport(ERROR, > - > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + > (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > errmsg("cannot check index \"%s\"", > > RelationGetRelationName(rel)), > errdetail("Index is not valid."))); > > Other parts of the codebase (including pgstattuple) use > ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when rejecting invalid index. > So, changing to this error code still seems reasonable and more > consistent overall. > > Given that, I'd like to merge just this error code change from > the 0002 patch into 0003, and apply the combined patch to > the master branch only. Although I briefly considered backpatching > the change of error code, it could be seen as a behavioral change, > and the existing error code doesn't cause any real problem in older > branches. So it's probably better to leave it as-is there. > > The updated patch is attached. It also changes index_checkable() > from extern to static, since it's only used within verify_common.c. > > Thoughts? Thanks for updating the patch. I agree with your idea. BTW, is "witha" a typo in the commit message? Aside from that, I have no additional comments on your patch. Regards, -- Masahiro Ikeda NTT DATA Japan Corporation -
Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-14T11:10:40Z
On 2025/07/14 17:13, Masahiro Ikeda wrote: > On 2025-07-04 20:32, Fujii Masao wrote: >> On 2025/06/02 16:32, Masahiro Ikeda wrote: >>> OK, I think v5-0002 should be back-patched, since using incorrect error codes is essentially a bug. >> >> Thanks for updating the patches! >> >> While reviewing the code: >> >> amtup = SearchSysCache1(AMOID, ObjectIdGetDatum(am_id)); >> amtuprel = SearchSysCache1(AMOID, >> ObjectIdGetDatum(rel->rd_rel->relam)); >> ereport(ERROR, >> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), >> + (errcode(ERRCODE_WRONG_OBJECT_TYPE), >> 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)))); >> >> I initially thought switching to ERRCODE_WRONG_OBJECT_TYPE made sense, >> but after checking similar cases, I'm reconsidering. For instance, >> pgstat_relation() in pgstattuple.c uses ERRCODE_FEATURE_NOT_SUPPORTED >> when the access method is unexpected. That suggests using >> FEATURE_NOT_SUPPORTED here may not be incorrect. > > OK, that seems reasonable to me. > >> if (!rel->rd_index->indisvalid) >> ereport(ERROR, >> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), >> + >> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), >> errmsg("cannot check index \"%s\"", >> RelationGetRelationName(rel)), >> errdetail("Index is not valid."))); >> >> Other parts of the codebase (including pgstattuple) use >> ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when rejecting invalid index. >> So, changing to this error code still seems reasonable and more >> consistent overall. >> >> Given that, I'd like to merge just this error code change from >> the 0002 patch into 0003, and apply the combined patch to >> the master branch only. Although I briefly considered backpatching >> the change of error code, it could be seen as a behavioral change, >> and the existing error code doesn't cause any real problem in older >> branches. So it's probably better to leave it as-is there. >> >> The updated patch is attached. It also changes index_checkable() >> from extern to static, since it's only used within verify_common.c. >> >> Thoughts? > > Thanks for updating the patch. I agree with your idea. > BTW, is "witha" a typo in the commit message? Aside from that, I have > no additional comments on your patch. Thanks for the review! You're right, that was a typo. I've fixed it and pushed the patch. Thanks! Regards, -- Fujii Masao NTT DATA Japan Corporation