Thread

Re: Patch for migration of the pg_commit_ts directory

ls7777 <ls7777@yandex.ru> — 2025-10-07T06:52:03Z

  Hi,


   


  
    You did consider the case that track_commit_timestamp is set to on but the instance

    has not started before the pg_upgrade, right? Valid point.
  
  
     
  


  Yes.


   


  
    
      Sorry, I'm not very clear this part. Can you describe the point bit more?
    
  


  Easy-to-read code. But I've already changed everything.


   


  
    I've followed all the recommendations.
  
  
    Removed the function. Shortened the test.
  
  
    01. Removed initialization of variables.
  
  
    02. Change
  
  
    03. Removed
  
  
    04. Add
  
  
     
  


   


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


  Кому: 'ls7777' (ls7777@yandex.ru), orlovmg@gmail.com (orlovmg@gmail.com), amit.kapila16@gmail.com (amit.kapila16@gmail.com);


  Копия: pgsql-hackers@postgresql.org;


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


  07.10.2025, 10:57, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>:


  

    Hi,

     
  
  
     At the time check_control_data is launched for the new cluster, the control file

     does not contain information about the set track_commit_timestamp=on. It is

     installed only in postgresql.conf.
  
  

    

    You did consider the case that track_commit_timestamp is set to on but the instance

    has not started before the pg_upgrade, right? Valid point.

     
  
  
     The check_track_commit_timestamp_parameter function is only needed for the new

     cluster. And you can not run it for the old cluster, but use data from the

     old_cluster.controldata.chkpnt_newstCommitTsxid control file. But this will be

     less clear than using the check_track_commit_timestamp_parameter function.
  
  

    

    Sorry, I'm not very clear this part. Can you describe the point bit more?

    

    Other comments:

    

    01. get_control_data

    

    ```

    + if (GET_MAJOR_VERSION(cluster->major_version) < 905)

    + {

    + cluster->controldata.chkpnt_oldstCommitTsxid = 0;

    + cluster->controldata.chkpnt_newstCommitTsxid = 0;

    + }

    ```

    

    IIUC, checksum_version is checked like this because got_data_checksum_version

    must be also set.

    Since we do not have the boolean for commit_ts, not sure it is needed.

    old_cluster and new_cluster are the global variable and they are initialized with

    zero.

    

    02. check_track_commit_timestamp_parameter

    

    ```

    + conn = connectToServer(cluster, "template1");

    + res = executeQueryOrDie(conn, "SELECT count(*) AS is_set "

    + "FROM pg_settings WHERE name = 'track_commit_timestamp' and setting = 'on'");

    + is_set = PQfnumber(res, "is_set");

    + cluster->track_commit_timestamp_on = atoi(PQgetvalue(res, 0, is_set)) == 1;

    ```

    

    The SQL looks strange for me. Isn't it enough to directly obtain "setting" attribute

    and check it is "on" or not? Also, conn and res can be the local variable of the

    else part.

    

    03. ClusterInfo

    

    ```

    + bool track_commit_timestamp_on; /* track_commit_timestamp for

    + * cluster */

    ```

    

    The indent is broken, please run pgindent.

    

    04. test

    

    Could you please update meson.build? Otherwise the added test could not be run for meson build.

    

    Best regards,

    Hayato Kuroda

    FUJITSU LIMITED