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

On Fri, Jan 27, 2023 at 3:41 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>

>
> I am reviewing further...
> thanks
> Shveta

Few more comments:

v4-0001:

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.

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

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

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

===============
v9-0002:

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

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.

thanks
Shveta