Few observations in Replication slots related code:
1. In function StartupReplicationSlots(XLogRecPtr checkPointRedo),
parameter checkPointRedo is not used.
2. Few check are in different order in functions
pg_create_physical_replication_slot() and
pg_create_logical_replication_slot().
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
check_permissions();
CheckLogicalDecodingRequirements();
I don't think it makes any difference, however having checks in similar
order seems better unless there is a reason for not doing so.
3. Currently there is any Assert (Assert(!MyReplicationSlot);) in
pg_create_logical_replication_slot(), is there a need for similar
Assert in pg_create_physical_replication_slot()?
4.
StartupDecodingContext()
{
..
context = AllocSetContextCreate(CurrentMemoryContext,
"Changeset Extraction Context",
}
To be consistent, shall we name this context as
logical | logical decoding?
5.
pg_create_logical_replication_slot()
{
..
CreateInitDecodingContext()
..
ReplicationSlotPersist()
}
Function pg_create_logical_replication_slot() is trying to
save slot twice once during CreateInitDecodingContext() and
then in ReplicationSlotPersist(), isn't it better if we can make
it do it just once?
6.
elog(ERROR, "cannot handle changeset extraction yet");
Shouldn't it be better to use logical replication instead
of changeset extraction?
7.
+ * XXX: It might make sense to introduce ephemeral slots and always use
+ * the slot mechanism.
Already there are RS_EPHEMERAL slots, might be this
comment needs bit of rephrasing.
8. Is there a chance of inconsistency, if pg_restxlog resets the
xlog and after restart, one of the slots contains xlog position
older than what is resetted by pg_resetxlog?
9.
void
LogicalIncreaseXminForSlot(XLogRecPtr current_lsn, TransactionId xmin)
{
..
/*
* don't overwrite if we already have a newer xmin. This can happen if we
* restart decoding in a slot.
*/
if (TransactionIdPrecedesOrEquals(xmin, slot->data.catalog_xmin))
{
}
..
}
Should we just release spinlock in this loop and just return,
rather than keeping no action loop?
10.
* Iff ri_type = REPLICA_IDENTITY_INDEX, indexOid must be the Oid of a
suitable
* index. Otherwise, it should be InvalidOid.
*/
static void
relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
bool is_internal)
typo - *Iff*
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
