Thread

Re: Patch for migration of the pg_commit_ts directory

ls7777 <ls7777@yandex.ru> — 2025-10-09T10:24:33Z

  Hi,


  
    Followed all recommendations.
  
  
     
  
  
     
  


   


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


  Кому: 'ls7777' (ls7777@yandex.ru);


  Копия: pgsql-hackers@postgresql.org, orlovmg@gmail.com, amit.kapila16@gmail.com;


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


  09.10.2025, 11:22, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.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