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