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

Dear Bharath,

Thank you for reviewing! PSA new version.

> 1. A nit:
> +
> +    /*
> +     * We also skip decoding in 'fast_forward' mode. In passing set the
> +     * 'processing_required' flag to indicate, were it not for this mode,
> +     * processing *would* have been required.
> +     */
> How about "We also skip decoding in fast_forward mode. In passing set
> the processing_required flag to indicate that if it were not for
> fast_forward mode, processing would have been required."?

Fixed.

> 2. Don't we need InvalidateSystemCaches() after FreeDecodingContext()?
> 
> +    /* Clean up */
> +    FreeDecodingContext(ctx);

Right. Older system caches should be thrown away here for upcoming pg_dump.

> 3. Don't we need to put CreateDecodingContext in PG_TRY-PG_CATCH with
> InvalidateSystemCaches() in PG_CATCH block? I think we need to clear
> all timetravel entries with InvalidateSystemCaches(), no?

Added.

> 4. The following assertion better be an error? Or we ensure that
> binary_upgrade_slot_has_caught_up isn't called for an invalidated slot
> at all?
> +
> +    /* Slots must be valid as otherwise we won't be able to scan the WAL */
> +    Assert(MyReplicationSlot->data.invalidated == RS_INVAL_NONE);

I kept the Assert() because pg_upgrade won't call this function for invalidated
slots.

> 5. This better be an error instead of returning false? IMO, null value
> for slot name is an error.
> +    /* Quick exit if the input is NULL */
> +    if (PG_ARGISNULL(0))
> +        PG_RETURN_BOOL(false);

Hmm, OK, changed to elog(ERROR).
If current style is kept and NULL were to input, an empty string may be reported
as slotname in invalid_logical_replication_slots.txt. It is quite strange. Note
again that it won't be expected.

> 6. A nit: how about is_decodable_txn or is_decodable_change or some
> other instead of just a plain name processing_required?
> +    /* Do we need to process any change in 'fast_forward' mode? */
> +    bool        processing_required;

I preferred current one. Because not only decodable txn, non-txn change and
empty transactions also be processed.

> 7. Can the following pg_fatal message be consistent and start with
> lowercase letter something like "expected 0 logical replication slots
> ...."?
> +        pg_fatal("Expected 0 logical replication slots but found %d.",
> +                 nslots_on_new);

Note that the Upper/Lower case rule has been broken in this file. Lower case was
used here because I regarded this sentence as hint message. Please see previous
posts [1] [2].


> 8. s/problem/problematic - "A list of problematic slots is in the file:\n"
> +                 "A list of the problem slots is in the file:\n"

Fixed.

> 9. IMO, binary_upgrade_logical_replication_slot_has_caught_up seems
> better, meaningful and consistent despite a bit long than just
> binary_upgrade_slot_has_caught_up.

Fixed.

> 10. How about an assert that the passed-in replication slot is logical
> in binary_upgrade_slot_has_caught_up?

Fixed.

> 11. How about adding CheckLogicalDecodingRequirements too in
> binary_upgrade_slot_has_caught_up after CheckSlotPermissions just in
> case?

Not added. CheckLogicalDecodingRequirements() ensures that WALs can be decodable
and the changes can be applied, but both of them are not needed for fast_forward
mode. Also, pre-existing function pg_logical_replication_slot_advance() does not
call it.

> 12. Not necessary but adding ReplicationSlotValidateName(slot_name,
> ERROR); for the passed-in slotname in
> binary_upgrade_slot_has_caught_up may be a good idea, at least in
> assert builds to help with input validations.

Not added because ReplicationSlotAcquire() can report even if invalid name is
added. Also, pre-existing function pg_logical_replication_slot_advance() does not
call it.

> 13. Can the functionality of LogicalReplicationSlotHasPendingWal be
> moved to binary_upgrade_slot_has_caught_up and get rid of a separate
> function LogicalReplicationSlotHasPendingWal? Or is it that the
> function exists in logical.c to avoid extra dependencies between
> logical.c and pg_upgrade_support.c?

I kept current style. I think upgrade functions should be short so that actual
tasks should be done in other place. SetAttrMissing() is called only from an
upgrading function, so we do not have a policy to avoid deviding function.
Also, LogicalDecodingProcessRecord() is called from only files in src/backend/replication,
so we can keep them.

> 14. I think it's better to check if the old cluster contains the
> necessary function binary_upgrade_slot_has_caught_up instead of just
> relying on major version.
> +    /* Logical slots can be migrated since PG17. */
> +    if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
> +        return;

I kept current style because I could not find a merit for the approach. If the
patch is committed PG17.X surely has binary_upgrade_logical_replication_slot_has_caught_up().
Also, other upgrading function are not checked from the pg_proc catalog. If you
have some other things in your mind, please reply here.

[1]:
https://www.postgresql.org/message-id/TYAPR01MB586642D33208D190F67CDD7BF5F2A%40TYAPR01MB5866.jpnprd01.prod.outlook.com
[2]:
https://www.postgresql.org/message-id/TYAPR01MB58666936A0DB0EEDCC929CEEF5FEA%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED