Re: Patch for migration of the pg_commit_ts directory

From: ls7777 <ls7777(at)yandex(dot)ru>
To: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, "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 06:52:03
Message-ID: 1809191759818745@mail.yandex.ru
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


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(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);

Копия: pgsql-hackers(at)postgresql(dot)org;

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

07.10.2025, 10:57, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)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

 

Attachment Content-Type Size
unknown_filename text/html 3.5 KB
v4-Migration-of-the-pg_commit_ts-directory.patch text/x-diff 9.5 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Arseniy Mukhin 2025-10-07 07:06:41 Re: GIN tries to form a tuple with a partial compressedList during insertion
Previous Message Michael Paquier 2025-10-07 06:46:10 Re: Remove custom redundant full page write description from GIN