Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication

Hi,

I mistakenly attached v9 in my previous email.
Please see attached v6 and v10 for the previous and below changes.

shveta malik <shveta(dot)malik(at)gmail(dot)com>, 31 Oca 2023 Sal, 12:59 tarihinde
şunu yazdı:

> On Fri, Jan 27, 2023 at 3:41 PM shveta malik <shveta(dot)malik(at)gmail(dot)com>
> wrote:
> 1)
> REPLICATION_SLOT_SNAPSHOT
> --Do we need 'CREATE' prefix with it i.e. CREATE_REPLICATION_SNAPSHOT
> (or some other brief one with CREATE?). 'REPLICATION_SLOT_SNAPSHOT'
> does not look like a command/action and thus is confusing.
>

Renamed it as CREATE_REPLICATION_SNAPSHOT

> 2)
> is used in the currenct transaction. This command is currently only
> supported
> for logical replication.
> slots.
> --typo: currenct-->current
> --slots can be moved to previous line
>

Done.

> 3)
> /*
> * Signal that we don't need the timeout mechanism. We're just creating
> * the replication slot and don't yet accept feedback messages or send
> * keepalives. As we possibly need to wait for further WAL the walsender
> * would otherwise possibly be killed too soon.
> */
> We're just creating the replication slot --> We're just creating the
> replication snapshot
>

Done.

> 4)
> I see XactReadOnly check in CreateReplicationSlot, do we need the same
> in ReplicationSlotSnapshot() as well?
>

Added this check too.

> ===============
> v9-0002:
>
> 5)
> /* We are safe to drop the replication trackin origin after this
> --typo: tracking
>

Done.

> 6)
> slot->data.catalog_xmin = xmin_horizon;
> slot->effective_xmin = xmin_horizon;
> SpinLockRelease(&slot->mutex);
> xmin_horizon =
> GetOldestSafeDecodingTransactionId(!need_full_snapshot);
> ReplicationSlotsComputeRequiredXmin(true);
>
> --do we need to set xmin_horizon in slot after
> 'GetOldestSafeDecodingTransactionId' call, otherwise it will be set to
> InvalidId in slot. Is that intentional? I see that we do set this
> correct xmin_horizon in builder->initial_xmin_horizon but the slot is
> carrying Invalid one.
>

I think you're right. Moved GetOldestSafeDecodingTransactionId call before
xmin_horizon assignment.

Thanks,
--
Melih Mutlu
Microsoft