RE: [PoC] pg_upgrade: allow to upgrade publisher node

Dear Vignesh,

Thanks for revieing! New patch can be available in [1].

> Few comments:
> 1) Even if we comment 3rd point "Emit a non-transactional message",
> test_slot2 still appears in the invalid_logical_replication_slots.txt
> file. There is something wrong here.
> + # 2. Advance the slot test_slot2 up to the current WAL location, but
> + # test_slot1 still has unconsumed WAL records.
> + $old_publisher->safe_psql('postgres',
> + "SELECT pg_replication_slot_advance('test_slot2', NULL);");
> +
> + # 3. Emit a non-transactional message. test_slot2 detects the message
> so
> + # that this slot will be also reported by upcoming pg_upgrade.
> + $old_publisher->safe_psql('postgres',
> + "SELECT count(*) FROM pg_logical_emit_message('false',
> 'prefix', 'This is a non-transactional message');"
> + );

The comment was updated based on others. How do you think?

> 2) If the test fails here, it is difficult to debug as the
> pg_upgrade_output.d directory was removed, so better to keep the
> directory as it is this case:
> + # Check the file content. Both slots should be reporting that they have
> + # unconsumed WAL records.
> + like(
> + slurp_file($slots_filename),
> + qr/The slot \"test_slot1\" has not consumed the WAL yet/m,
> + 'the previous test failed due to unconsumed WALs');
> + like(
> + slurp_file($slots_filename),
> + qr/The slot \"test_slot2\" has not consumed the WAL yet/m,
> + 'the previous test failed due to unconsumed WALs');
> +
> + # Clean up
> + rmtree($new_publisher->data_dir . "/pg_upgrade_output.d");

Right. Current style just follows the 002 test. I removed rmtree().

> 3) The below could be changed:
> + # Check the file content. Both slots should be reporting that they have
> + # unconsumed WAL records.
> + like(
> + slurp_file($slots_filename),
> + qr/The slot \"test_slot1\" has not consumed the WAL yet/m,
> + 'the previous test failed due to unconsumed WALs');
> + like(
> + slurp_file($slots_filename),
> + qr/The slot \"test_slot2\" has not consumed the WAL yet/m,
> + 'the previous test failed due to unconsumed WALs');
>
> to:
> my $result = slurp_file($slots_filename);
> is( $result, qq(The slot "test_slot1" has not consumed the WAL yet
> The slot "test_slot2" has not consumed the WAL yet
> ),
> 'the previous test failed due to unconsumed WALs');
>

Replaced, but the formatting seems not good. I wanted to hear opinions from others.

[1]: https://www.postgresql.org/message-id/TYCPR01MB587007EA2F9AB92F0E1F5957F5D4A%40TYCPR01MB5870.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED