RE: Patch for migration of the pg_commit_ts directory
От | Hayato Kuroda (Fujitsu) |
---|---|
Тема | RE: Patch for migration of the pg_commit_ts directory |
Дата | |
Msg-id | OSCPR01MB14966359349E2A205DFB2D6CAF5EEA@OSCPR01MB14966.jpnprd01.prod.outlook.com обсуждение исходный текст |
Ответ на | RE: Patch for migration of the pg_commit_ts directory ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>) |
Список | 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
В списке pgsql-hackers по дате отправления: