Обсуждение: Issues in Replication Progress Tracking

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

On 2015-05-21 09:40:58 +0530, Amit Kapila wrote:
> On Thu, May 21, 2015 at 12:42 AM, Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2015-05-20 19:27:05 +0530, Amit Kapila wrote:
> >
> > > 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.
> >
> 
> Okay, but what triggered this question was the difference of those functions
> as compare to when user call function pg_replication_origin_advance().
> pg_replication_origin_advance() will WAL log the information during that
> function call itself (via replorigin_advance()).  So even if the transaction
> issuing pg_replication_origin_advance() function will abort, it will still
> update
> the Replication State, why so?

I don't see a problem here. pg_replication_origin_advance() is for
setting up the initial position/update the position upon configuration
changes. It'd be a fair amount of infrastructure to make it tie into
transactions - without a point to it afaics?

(Just to be clear, I plan to address all the points I've not commented
upon)

Greetings,

Andres Freund

On May 22, 2015 10:23:06 PM PDT, Amit Kapila <amit.kapila16@gmail.com> wrote:
>On Fri, May 22, 2015 at 11:20 PM, Andres Freund <andres@anarazel.de>
>wrote:
>>
>> On 2015-05-21 09:40:58 +0530, Amit Kapila wrote:
>> > On Thu, May 21, 2015 at 12:42 AM, Andres Freund
><andres@anarazel.de>
>wrote:
>> > >
>> > > On 2015-05-20 19:27:05 +0530, Amit Kapila wrote:
>> > >
>> > > > 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.
>> > >
>> >
>> > Okay, but what triggered this question was the difference of those
>functions
>> > as compare to when user call function
>pg_replication_origin_advance().
>> > pg_replication_origin_advance() will WAL log the information during
>that
>> > function call itself (via replorigin_advance()).  So even if the
>transaction
>> > issuing pg_replication_origin_advance() function will abort, it
>will
>still
>> > update
>> > the Replication State, why so?
>>
>> I don't see a problem here. pg_replication_origin_advance() is for
>> setting up the initial position/update the position upon
>configuration
>> changes.
>
>Okay, I am not aware how exactly these API's will be used for
>replication
>but let me try to clarify what I have in mind related to this API
>usage.
>
>Can we use pg_replication_origin_advance()  for node where Replay has
>to happen, if Yes, then Let us say user of
>pg_replication_origin_advance()
>API set the lsn position to X for the node N1 on which replay has to
>happen, so now replay will proceed from X + 1 even though the
>information related to X is not persisted, so now it could so happen
>X will get written after the replay of X + 1 which might lead to
>problem
>after crash recovery?

Then you'd use the session variant - which does tie into transactions. Is there a reason that's be unsuitable for you?

Andres


--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.