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

On Tue, Nov 28, 2023 at 11:06 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> One month has already been passed since this main patch got committed
> but reading this change, I have some questions on new
> binary_upgrade_logical_slot_has_caught_up() function:
>
> 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).

It may happen that such a user-facing function tells there's no
unconsumed WAL, but later on the WAL gets generated during pg_upgrade.
Therefore, the information the function gives turns out to be
incorrect. I don't see a real-world use-case for such a function right
now. If there's one, it's not a big change to turn it into a
user-facing function.

> ---
> 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.

If that were true, I don't see a problem in having
CheckSlotPermissions() there, in fact it can act as an assertion.

> ---
> 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");
>
> ---
> { 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.

I think it has been done that way similar to
binary_upgrade_create_empty_extension().

> ---
> 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.

Why not in logicalfuncs.c?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com