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

On Fri, Jan 27, 2023 at 3:41 PM shveta malik <shveta.malik@gmail.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