RE: Patch for migration of the pg_commit_ts directory

From: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
To: 'ls7777' <ls7777(at)yandex(dot)ru>, "orlovmg(at)gmail(dot)com" <orlovmg(at)gmail(dot)com>, "amit(dot)kapila16(at)gmail(dot)com" <amit(dot)kapila16(at)gmail(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: Patch for migration of the pg_commit_ts directory
Date: 2025-10-07 05:57:14
Message-ID: OSCPR01MB14966709F525419547BA3F99DF5E0A@OSCPR01MB14966.jpnprd01.prod.outlook.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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
 

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jeremy Schneider 2025-10-07 05:59:33 sync_standbys_defined and pg_stat_replication
Previous Message Tom Lane 2025-10-07 05:43:16 Re: Optimize LISTEN/NOTIFY