Hi,
Thanks for looking through this!
On 2015-05-20 19:27:05 +0530, Amit Kapila wrote:
> 5.
> origin.c
>
> * * To create and drop replication origins an exclusive lock on
> * pg_replication_slot is required for the
> duration. That allows us to
> * safely and conflict free assign new origins using a dirty snapshot.
> b. "..safely and conflict free assign..", I understand this part
> of comment, but not sure if this is the best way to write it.
Hm, don't see a problem with that part.
> 8.
> origin.c
> replorigin_drop()
> {
> ..
> tuple = SearchSysCache1(REPLORIGIDENT, ObjectIdGetDatum(roident));
> simple_heap_delete(rel, &tuple-
> >t_self);
> ..
> }
>
> Isn't it better to have check for a valid tuple after SearchSysCache1()?
> if (!HeapTupleIsValid(tuple))
> elog(ERROR, "cache lookup failed
Sounds good to me.
> 9.
> origin.c
> ReplicationOriginShmemSize(void)
> {
> ..
> * XXX: max_replication_slots is arguablethe wrong
> ..
> }
> b. One thing that in favour of using a separate/new guc for
> ReplicationState is that even if the user has configured
> max_replication_slots for some other usage (other than
> tracking Replication Origin) of ReplicationSlots, even then we
> will end up allocating shared memory which will never be used,
> OTOH as the memory will not be huge, so we can even ignore it.
I don't think it matters much for now, as you say it's only a small
amount of memory.
> 12.
> In funcions replorigin_advance() and replorigin_session_setup(),
> different ways (free_state and free_slot) are used. Isn't it better
> to use same way?
Phew, I don't really care.
> 13.
> In function replorigin_session_setup() and or
> replorigin_session_advance(), don't we need to WAL log the
> use of Replication state?
No, the point is that the replication progress is persisted via an extra
data block in the commit record. That's important for both performance
and correctness, because otherwise it gets hard to tie a transaction
made during replay with the update to the progress. Unless you use 2PC
which isn't really an alternative.
Greetings,
Andres Freund