(unnamed)

text/html

Download

<div>Hi,</div><div> </div><blockquote><div><div>You did consider the case that track_commit_timestamp is set to on but the instance<br />has not started before the pg_upgrade, right? Valid point.</div></div><div> </div></blockquote><div>Yes.</div><div> </div><div><blockquote><div>Sorry, I'm not very clear this part. Can you describe the point bit more?</div></blockquote></div><div><div>Easy-to-read code. But I've already changed everything.</div></div><div> </div><div><div>I've followed all the recommendations.</div><div>Removed the function. Shortened the test.</div><div>01. Removed initialization of variables.</div><div>02. Change</div><div>03. Removed</div><div>04. Add</div><div> </div></div><div> </div><div>----------------</div><div>Кому: 'ls7777' (ls7777@yandex.ru), orlovmg@gmail.com (orlovmg@gmail.com), amit.kapila16@gmail.com (amit.kapila16@gmail.com);</div><div>Копия: pgsql-hackers@postgresql.org;</div><div>Тема: Patch for migration of the pg_commit_ts directory;</div><div>07.10.2025, 10:57, "Hayato Kuroda (Fujitsu)" &lt;kuroda.hayato@fujitsu.com&gt;:</div><blockquote><p>Hi,<br /> </p><blockquote> At the time check_control_data is launched for the new cluster, the control file<br /> does not contain information about the set track_commit_timestamp=on. It is<br /> installed only in postgresql.conf.</blockquote><p><br />You did consider the case that track_commit_timestamp is set to on but the instance<br />has not started before the pg_upgrade, right? Valid point.<br /> </p><blockquote> The check_track_commit_timestamp_parameter function is only needed for the new<br /> cluster. And you can not run it for the old cluster, but use data from the<br /> old_cluster.controldata.chkpnt_newstCommitTsxid control file. But this will be<br /> less clear than using the check_track_commit_timestamp_parameter function.</blockquote><p><br />Sorry, I'm not very clear this part. Can you describe the point bit more?<br /><br />Other comments:<br /><br />01. get_control_data<br /><br />```<br />+ if (GET_MAJOR_VERSION(cluster-&gt;major_version) &lt; 905)<br />+ {<!-- --><br />+ cluster-&gt;controldata.chkpnt_oldstCommitTsxid = 0;<br />+ cluster-&gt;controldata.chkpnt_newstCommitTsxid = 0;<br />+ }<br />```<br /><br />IIUC, checksum_version is checked like this because got_data_checksum_version<br />must be also set.<br />Since we do not have the boolean for commit_ts, not sure it is needed.<br />old_cluster and new_cluster are the global variable and they are initialized with<br />zero.<br /><br />02. check_track_commit_timestamp_parameter<br /><br />```<br />+ conn = connectToServer(cluster, "template1");<br />+ res = executeQueryOrDie(conn, "SELECT count(*) AS is_set "<br />+ "FROM pg_settings WHERE name = 'track_commit_timestamp' and setting = 'on'");<br />+ is_set = PQfnumber(res, "is_set");<br />+ cluster-&gt;track_commit_timestamp_on = atoi(PQgetvalue(res, 0, is_set)) == 1;<br />```<br /><br />The SQL looks strange for me. Isn't it enough to directly obtain "setting" attribute<br />and check it is "on" or not? Also, conn and res can be the local variable of the<br />else part.<br /><br />03. ClusterInfo<br /><br />```<br />+ bool track_commit_timestamp_on; /* track_commit_timestamp for<br />+ * cluster */<br />```<br /><br />The indent is broken, please run pgindent.<br /><br />04. test<br /><br />Could you please update meson.build? Otherwise the added test could not be run for meson build.<br /><br />Best regards,<br />Hayato Kuroda<br />FUJITSU LIMITED<br /> </p></blockquote>