RE: [PoC] pg_upgrade: allow to upgrade publisher node - Mailing list pgsql-hackers

Dear Sawada-san, hackers,

Based on comments I made a fix. PSA the patch.

> 
> Is there any reason why this function can be executed only in binary
> upgrade mode? It seems to me that other functions in
> pg_upgrade_support.c must be called only in binary upgrade mode
> because it does some hacky changes internally. On the other hand,
> binary_upgrade_logical_slot_has_caught_up() just calls
> LogicalReplicationSlotHasPendingWal(), which doesn't change anything
> internally. If we make this function usable in normal mode, the user
> would be able to  check each slot's upgradability without pg_upgrade
> --check command (or without stopping the server if the user can ensure
> no more meaningful WAL records are generated).

I kept the function to be upgrade only because subsequent operations might generate
WALs. See [1].

> Also, the function checks if the user has the REPLICATION privilege
> but I think that only superuser can connect to the server in binary
> upgrade mode in the first place.

CheckSlotPermissions() was replaced to Assert().

> The following error message doesn't match the function name:
> 
>     /* We must check before dereferencing the argument */
>     if (PG_ARGISNULL(0))
>         elog(ERROR, "null argument to
> binary_upgrade_validate_wal_records is not allowed");

Per below comment, this elog(ERROR) was not needed anymore. Removed.

> { oid => '8046', descr => 'for use by pg_upgrade',
>   proname => 'binary_upgrade_logical_slot_has_caught_up', proisstrict => 'f',
>   provolatile => 'v', proparallel => 'u', prorettype => 'bool',
>   proargtypes => 'name',
>   prosrc => 'binary_upgrade_logical_slot_has_caught_up' },
> 
> The function is not a strict function but we check in the function if
> the passed argument is not null. I think it would be clearer to make
> it a strict function.

Per conclusion [2], I changed the function to the strict one. As shown in below,
binary_upgrade_logical_slot_has_caught_up() returned NULL when the input was NULL.

```
postgres=# SELECT * FROM pg_create_logical_replication_slot('slot', 'test_decoding');
 slot_name |    lsn    
-----------+-----------
 slot      | 0/152E7E0
(1 row)

postgres=# SELECT * FROM binary_upgrade_logical_slot_has_caught_up(NULL);
 binary_upgrade_logical_slot_has_caught_up 
-------------------------------------------
 
(1 row)
```

> LogicalReplicationSlotHasPendingWal() is defined in logical.c but I
> guess it's more suitable to be in slotfunc.s where similar functions
> such as pg_logical_replication_slot_advance() is also defined.

Committers had different opinions about it, so I kept current style [3].

[1]: https://www.postgresql.org/message-id/CALj2ACW7H-kAHia%3DvCbmdWDueGA_3pQfyzARfAQX0aGzHY57Zw%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAA4eK1LzK0NvMkWAY6RJ6yN%2BYYUgMg1f%3DmNOGV8CPXLT43FHMw%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAD21AoDkyyC%3Dwa2%3D1Ruo_L8g16xf_W5Xyhp-%3D3j9urT916b9gA%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED