Re: Patch for migration of the pg_commit_ts directory

Hi,

Followed all recommendations.


 


 

 

----------------

Кому: 'ls7777' (ls7777(at)yandex(dot)ru);

Копия: pgsql-hackers(at)postgresql(dot)org, orlovmg(at)gmail(dot)com, amit(dot)kapila16(at)gmail(dot)com;

Тема: Patch for migration of the pg_commit_ts directory;

09.10.2025, 11:22, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>:

Hi,

Thanks for updating the patch and sorry for the late reply. Here are my comments.

01.

```

+ prep_status("Checking for pg_commit_ts");

```

I think we must clarify which node is being checked. Something like:

Checking for new cluster configuration for commit timestamp

02.

```

        }

-

         /* we have the result of cmd in "output". so parse it line by line now */

```

This change is not needed.

03.

```

+ /*

+ * Copy pg_commit_ts

+ */

```

I feel it can be removed or have more meanings. Something lile:

Copy old commit_timestamp data to new, if available.

04.

Regarding the test,

05.

```

+sub command_output

```

Can run_command() be used instead of defining new function?

06.

```

+$old->command_ok([ 'pgbench', '-i', '-s', '1' ], 'init pgbench');

+$old->command_ok([ 'pgbench', '-t', '1', '-j', '1' ], 'pgbench it');

```

I think no need to use pgbench anymore. Can we define tables and insert tuples

by myself?

07.

```

+command_fails(

+ [

+ 'pg_upgrade', '--no-sync',

+ '-d', $old->data_dir,

+ '-D', $new->data_dir,

+ '-b', $old->config_data('--bindir'),

+ '-B', $new->config_data('--bindir'),

+ '-s', $new->host,

+ '-p', $old->port,

+ '-P', $new->port,

+ $mode

+ ],

+ 'run of pg_upgrade fails with mismatch parameter track_commit_timestamp');

```

According to other test files, we do not use the shorten option.

Also, please verify the actual output by command_ok_or_fails_like() or command_checks_all().

08.

```

+sub xact_commit_ts

```

Not sure, but this function is introduced because we have 100 transactions. Can we omit this now?

09.

```

+# The new cluster should contain timestamps created during the pg_upgrade and

+# some more created by the pgbench.

+#

+print "\nCommit timestamp only in new cluster:\n";

+for my $line (@b) {

+ print "$line\n" unless $h1{$line};

+}

```

I don't think this is needed because the output is not verified.

Best regards,

Hayato Kuroda

FUJITSU LIMITED