(unnamed)

text/html

Download

<div dir='auto'><div><div><div class="elided-text">2025/05/21 12:54 Fujii Masao &lt;masao.fujii@gmail.com&gt;:<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 &lt;ikedamsh@oss.nttdata.com&gt; wrote:
<br>
&gt;
<br>
&gt; Thanks for your work and feedback!
<br>
&gt;
<br>
&gt; I've updated the patches and added regular regression tests for
<br>
&gt; 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>
-&nbsp;&nbsp;&nbsp; RelationGetRelationName(rel), NameStr(((Form_pg_am)
<br>
GETSTRUCT(amtuprel))-&gt;amname))));
<br>
+ errdetail("Relation \"%s\" is a %s %sindex.",
<br>
+&nbsp;&nbsp;&nbsp; RelationGetRelationName(rel), NameStr(((Form_pg_am)
<br>
GETSTRUCT(amtuprel))-&gt;amname),
<br>
+&nbsp;&nbsp;&nbsp; (rel-&gt;rd_rel-&gt;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>