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' ([email protected]), [email protected] ([email protected]), [email protected] ([email protected]);
Копия: [email protected];
Тема: Patch for migration of the pg_commit_ts directory;
07.10.2025, 10:57, "Hayato Kuroda (Fujitsu)" <[email protected]>:
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
From 1d5499d87687ca15dd7d4daf1b98d68dd11dba74 Mon Sep 17 00:00:00 2001 From: Sergey Levin <[email protected]> Date: Sat, 4 Oct 2025 20:22:46 +0500 Subject: [PATCH] Migration of the pg_commit_ts directory
---
src/bin/pg_upgrade/check.c | 22 +++
src/bin/pg_upgrade/controldata.c | 21 ++-
src/bin/pg_upgrade/meson.build | 1 +
src/bin/pg_upgrade/pg_upgrade.c | 20 ++-
src/bin/pg_upgrade/pg_upgrade.h | 2 +
.../pg_upgrade/t/007_transfer_commit_ts.pl | 139 ++++++++++++++++++
6 files changed, 201 insertions(+), 4 deletions(-)
create mode 100644 src/bin/pg_upgrade/t/007_transfer_commit_ts.pl
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 1e17d64b3ec..f0e25dd3191 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -32,6 +32,7 @@ static void check_new_cluster_replication_slots(void);
static void check_new_cluster_subscription_configuration(void);
static void check_old_cluster_for_valid_slots(void);
static void check_old_cluster_subscription_state(void);
+static void check_new_cluster_pg_commit_ts(void);
/*
* DataTypesUsageChecks - definitions of data type checks for the old cluster
@@ -767,9 +768,30 @@ check_new_cluster(void)
check_new_cluster_replication_slots();
check_new_cluster_subscription_configuration();
+
+ check_new_cluster_pg_commit_ts();
+
}
+void
+check_new_cluster_pg_commit_ts(void)
+{
+ PGconn *conn;
+ PGresult *res;
+ bool track_commit_timestamp_on;
+
+ prep_status("Checking for pg_commit_ts");
+ conn = connectToServer(&new_cluster, "template1");
+ res = executeQueryOrDie(conn, "SELECT setting "
+ "FROM pg_settings WHERE name = 'track_commit_timestamp'");
+ track_commit_timestamp_on = strcmp(PQgetvalue(res, 0, 0), "on") == 0;
+ PQclear(res);
+ PQfinish(conn);
+ if (old_cluster.controldata.chkpnt_newstCommitTsxid > 0 && !track_commit_timestamp_on)
+ pg_fatal("\"track_commit_timestamp\" must be \"on\" but is set to \"off\"");
+ check_ok();
+}
void
report_clusters_compatible(void)
{
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index 90cef0864de..991000265ad 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -208,7 +208,6 @@ get_control_data(ClusterInfo *cluster)
cluster->controldata.data_checksum_version = 0;
got_data_checksum_version = true;
}
-
/* we have the result of cmd in "output". so parse it line by line now */
while (fgets(bufin, sizeof(bufin), output))
{
@@ -321,6 +320,26 @@ get_control_data(ClusterInfo *cluster)
cluster->controldata.chkpnt_nxtmulti = str2uint(p);
got_multi = true;
}
+ else if ((p = strstr(bufin, "Latest checkpoint's oldestCommitTsXid:")) != NULL)
+ {
+ p = strchr(p, ':');
+
+ if (p == NULL || strlen(p) <= 1)
+ pg_fatal("%d: controldata retrieval problem", __LINE__);
+
+ p++; /* remove ':' char */
+ cluster->controldata.chkpnt_oldstCommitTsxid = str2uint(p);
+ }
+ else if ((p = strstr(bufin, "Latest checkpoint's newestCommitTsXid:")) != NULL)
+ {
+ p = strchr(p, ':');
+
+ if (p == NULL || strlen(p) <= 1)
+ pg_fatal("%d: controldata retrieval problem", __LINE__);
+
+ p++; /* remove ':' char */
+ cluster->controldata.chkpnt_newstCommitTsxid = str2uint(p);
+ }
else if ((p = strstr(bufin, "Latest checkpoint's oldestXID:")) != NULL)
{
p = strchr(p, ':');
diff --git a/src/bin/pg_upgrade/meson.build b/src/bin/pg_upgrade/meson.build
index ac992f0d14b..030618152d0 100644
--- a/src/bin/pg_upgrade/meson.build
+++ b/src/bin/pg_upgrade/meson.build
@@ -47,6 +47,7 @@ tests += {
't/004_subscription.pl',
't/005_char_signedness.pl',
't/006_transfer_modes.pl',
+ 't/007_transfer_commit_ts.pl',
],
'test_kwargs': {'priority': 40}, # pg_upgrade tests are slow
},
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 490e98fa26f..98d01e8600c 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -772,6 +772,8 @@ copy_subdir_files(const char *old_subdir, const char *new_subdir)
static void
copy_xact_xlog_xid(void)
{
+ bool is_copy_commit_ts;
+
/*
* Copy old commit logs to new data dir. pg_clog has been renamed to
* pg_xact in post-10 clusters.
@@ -781,6 +783,15 @@ copy_xact_xlog_xid(void)
GET_MAJOR_VERSION(new_cluster.major_version) <= 906 ?
"pg_clog" : "pg_xact");
+ /*
+ * Copy pg_commit_ts
+ */
+ is_copy_commit_ts = old_cluster.controldata.chkpnt_oldstCommitTsxid > 0
+ && old_cluster.controldata.chkpnt_newstCommitTsxid > 0;
+
+ if (is_copy_commit_ts)
+ copy_subdir_files("pg_commit_ts", "pg_commit_ts");
+
prep_status("Setting oldest XID for new cluster");
exec_prog(UTILITY_LOG_FILE, NULL, true, true,
"\"%s/pg_resetwal\" -f -u %u \"%s\"",
@@ -798,12 +809,15 @@ copy_xact_xlog_xid(void)
"\"%s/pg_resetwal\" -f -e %u \"%s\"",
new_cluster.bindir, old_cluster.controldata.chkpnt_nxtepoch,
new_cluster.pgdata);
- /* must reset commit timestamp limits also */
+
+ /*
+ * must reset commit timestamp limits also or copy from the old cluster
+ */
exec_prog(UTILITY_LOG_FILE, NULL, true, true,
"\"%s/pg_resetwal\" -f -c %u,%u \"%s\"",
new_cluster.bindir,
- old_cluster.controldata.chkpnt_nxtxid,
- old_cluster.controldata.chkpnt_nxtxid,
+ is_copy_commit_ts ? old_cluster.controldata.chkpnt_oldstCommitTsxid : old_cluster.controldata.chkpnt_nxtxid,
+ is_copy_commit_ts ? old_cluster.controldata.chkpnt_newstCommitTsxid : old_cluster.controldata.chkpnt_nxtxid,
new_cluster.pgdata);
check_ok();
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 0ef47be0dc1..83fdbc44b13 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -238,6 +238,8 @@ typedef struct
uint32 chkpnt_nxtmxoff;
uint32 chkpnt_oldstMulti;
uint32 chkpnt_oldstxid;
+ uint32 chkpnt_oldstCommitTsxid;
+ uint32 chkpnt_newstCommitTsxid;
uint32 align;
uint32 blocksz;
uint32 largesz;
diff --git a/src/bin/pg_upgrade/t/007_transfer_commit_ts.pl b/src/bin/pg_upgrade/t/007_transfer_commit_ts.pl
new file mode 100644
index 00000000000..97d0b88ff2a
--- /dev/null
+++ b/src/bin/pg_upgrade/t/007_transfer_commit_ts.pl
@@ -0,0 +1,139 @@
+# Copyright (c) 2025, PostgreSQL Global Development Group
+
+# Tests for transfer pg_commit_ts directory.
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+sub command_output
+{
+ my ($cmd) = @_;
+ my ($stdout, $stderr);
+
+ print("# Running: " . join(" ", @{$cmd}) . "\n");
+
+ my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr;
+ ok($result, "@$cmd exit code 0");
+ is($stderr, '', "@$cmd no stderr");
+
+ return $stdout;
+}
+
+# Can be changed to test the other modes
+my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
+
+# Initialize old cluster
+my $old = PostgreSQL::Test::Cluster->new('old');
+$old->init;
+$old->append_conf('postgresql.conf', 'track_commit_timestamp = on');
+$old->start;
+$old->command_ok([ 'pgbench', '-i', '-s', '1' ], 'init pgbench');
+$old->command_ok([ 'pgbench', '-t', '1', '-j', '1' ], 'pgbench it');
+$old->stop;
+
+# Initialize new cluster
+my $new = PostgreSQL::Test::Cluster->new('new');
+$new->init;
+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');
+
+$new->append_conf('postgresql.conf', 'track_commit_timestamp = on');
+command_ok(
+ [
+ '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 ok');
+
+
+sub xact_commit_ts
+{
+ my ($node) = @_;
+ my ($oldest, $newest);
+ my $out = command_output([ 'pg_controldata', '-D', $node->data_dir ]);
+
+ if ($out =~ /oldestCommitTsXid:(\d+)/) {
+ $oldest = $1;
+ }
+ if ($out =~ /newestCommitTsXid:(\d+)/) {
+ $newest= $1;
+ }
+
+ for my $line (grep { /\S/ } split /\n/, $out) {
+ if ($line =~ /Latest checkpoint's NextXID:/) {
+ print $line . "\n";
+ }
+ if ($line =~ /Latest checkpoint's oldestXID:/) {
+ print $line . "\n";
+ }
+ if ($line =~ /Latest checkpoint's oldestCommitTsXid/) {
+ print $line . "\n";
+ }
+ if ($line =~ /Latest checkpoint's newestCommitTsXid:/) {
+ print $line . "\n";
+ }
+ }
+
+ $node->start;
+ my $res = $node->safe_psql('postgres', "
+ WITH xids AS (
+ SELECT v::text::xid AS x
+ FROM generate_series($oldest, $newest) v
+ )
+ SELECT x, pg_xact_commit_timestamp(x::text::xid)
+ FROM xids;
+ ");
+ $node->stop;
+ return grep { /\S/ } split /\n/, $res;
+}
+
+my @a = xact_commit_ts($old);
+my @b = xact_commit_ts($new);
+my %h1 = map { $_ => 1 } @a;
+my %h2 = map { $_ => 1 } @b;
+
+#
+# All timestamps from the old cluster should appear in the new one.
+#
+my $count = 0;
+print "Commit timestamp only in old cluster:\n";
+for my $line (@a) {
+ unless ($h2{$line}) {
+ print "$line\n";
+ $count = $count + 1;
+ }
+}
+ok($count == 0, "all the timestamp transferred successfully");
+
+#
+# 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};
+}
+
+done_testing();
--
2.42.2
