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