(unnamed)

text/html

Download

<div>Hi,</div><div><div>Followed all recommendations.</div><div> </div><div> </div></div><div> </div><div>----------------</div><div>Кому: 'ls7777' (ls7777@yandex.ru);</div><div>Копия: pgsql-hackers@postgresql.org, orlovmg@gmail.com, amit.kapila16@gmail.com;</div><div>Тема: Patch for migration of the pg_commit_ts directory;</div><div>09.10.2025, 11:22, "Hayato Kuroda (Fujitsu)" &lt;kuroda.hayato@fujitsu.com&gt;:</div><blockquote><p>Hi,<br /><br />Thanks for updating the patch and sorry for the late reply. Here are my comments.<br /><br />01.<br />```<br />+ prep_status("Checking for pg_commit_ts");<br />```<br /><br />I think we must clarify which node is being checked. Something like:<br />Checking for new cluster configuration for commit timestamp<br /><br />02.<br />```<br />        }<br />-<br />         /* we have the result of cmd in "output". so parse it line by line now */<br />```<br /><br />This change is not needed.<br /><br />03.<br />```<br />+ /*<br />+ * Copy pg_commit_ts<br />+ */<br />```<br /><br />I feel it can be removed or have more meanings. Something lile:<br />Copy old commit_timestamp data to new, if available.<br /><br />04.<br /><br />Regarding the test,<br /><br /><br />05.<br />```<br />+sub command_output<br />```<br /><br />Can run_command() be used instead of defining new function?<br /><br />06.<br />```<br />+$old-&gt;command_ok([ 'pgbench', '-i', '-s', '1' ], 'init pgbench');<br />+$old-&gt;command_ok([ 'pgbench', '-t', '1', '-j', '1' ], 'pgbench it');<br />```<br /><br />I think no need to use pgbench anymore. Can we define tables and insert tuples<br />by myself?<br /><br />07.<br />```<br />+command_fails(<br />+ [<br />+ 'pg_upgrade', '--no-sync',<br />+ '-d', $old-&gt;data_dir,<br />+ '-D', $new-&gt;data_dir,<br />+ '-b', $old-&gt;config_data('--bindir'),<br />+ '-B', $new-&gt;config_data('--bindir'),<br />+ '-s', $new-&gt;host,<br />+ '-p', $old-&gt;port,<br />+ '-P', $new-&gt;port,<br />+ $mode<br />+ ],<br />+ 'run of pg_upgrade fails with mismatch parameter track_commit_timestamp');<br />```<br /><br />According to other test files, we do not use the shorten option.<br />Also, please verify the actual output by command_ok_or_fails_like() or command_checks_all().<br /><br />08.<br />```<br />+sub xact_commit_ts<br />```<br /><br />Not sure, but this function is introduced because we have 100 transactions. Can we omit this now?<br /><br />09.<br />```<br />+# The new cluster should contain timestamps created during the pg_upgrade and<br />+# some more created by the pgbench.<br />+#<br />+print "\nCommit timestamp only in new cluster:\n";<br />+for my $line (@<a href="mailto:b" rel="noopener noreferrer">b</a>) {<!-- --><br />+ print "$line\n" unless $h1{$line};<br />+}<br />```<br /><br />I don't think this is needed because the output is not verified.<br /><br />Best regards,<br />Hayato Kuroda<br />FUJITSU LIMITED<br /> </p></blockquote>