From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'ls7777' <ls7777(at)yandex(dot)ru> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, "orlovmg(at)gmail(dot)com" <orlovmg(at)gmail(dot)com>, "amit(dot)kapila16(at)gmail(dot)com" <amit(dot)kapila16(at)gmail(dot)com> |
Subject: | RE: Patch for migration of the pg_commit_ts directory |
Date: | 2025-10-09 06:22:26 |
Message-ID: | OSCPR01MB14966359349E2A205DFB2D6CAF5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | shveta malik | 2025-10-09 06:32:20 | Re: Logical Replication of sequences |
Previous Message | Tatsuo Ishii | 2025-10-09 06:08:58 | User defined window functions calling WinGetFuncArgInPartition/WINDOW_SEEK_HEAD |