added patch and test
design at: http://www.port389.org/docs/389ds/design/csn-pending-lists-and-ruv-update.html

My comments:

• design

I like the design. How do we transmit knowledge of which csns are prim and which are op to a replica?

• test patch

ack from me, this looks reasonable.

• csn patch

176 +»       int repl_alloc; /* max number of replicas  */
177 +»       int repl_cnt; /* number of replicas affected by operation */

Please don't use bare int - use int32_t or int64_t (preferred).

250  
251 +void
252 +add_replica_to_primcsn(CSNPL_CTX *csnpl_ctx, Replica *repl)
253 +{
254 +»       int found = 0;
255 +»       int it = 0;
256 +»       while (it < csnpl_ctx->repl_cnt) {

it should be size_t not int. Same with int found, should be int64_t or similar.

263 +»       if (found) return;

should have braces for clarity.

469 +int ruv_cancel_csn_inprogress (void *repl, RUV *ruv, const CSN *csn, ReplicaId local_rid)
...
511 +»       for (it=0; it<prim_csn->repl_cnt; it++) {

shoud be for (size_t it = 0; ...).

178 +»       Replica *def_repl[CSNPL_CTX_REPLCNT]; /* pirm array of affected replicas */
179 +»       Replica **replicas; /* additional replicas affected */

Rather than having two lists of the replicas, shouldn't we just have **replicas, and default init to CSNPL_CTX_REPLCNT? IE 4? This way it makes the replicas handling easier, as it's always just the one array.

I'm sure that other people will have more. I think that it's a nice design, thanks for all your work on this!

@firstyear
thanks for your feedback, here are some answers

• design
  not sure I fully understand your question (so the design needs to be more clear), but I'll try to answer:
  the thread context always contains the bare primcsn and the replica it was generetaed for first.
  and in the pending list, each added csn has the prim_csn and prim_replica in its data, so we always know for each csn in a pending list if it is prim or not
  outside the pending list if we need to decide if a csn is prim, we need to get the prim_ctx from the thread data and check

• implementation
  yes, I am sloppy with the usage of types, and I will change it
  about the replica array: only in exceptional cases we have more than one replica for a prim_csn, so I wanted to avoid the allocation of an array to almost always add only replica. But your remark lets me think of a better solution. I noticed at some point that we need to disinguish and track the prim_replica and I took advantage of the case that the prim_replica is always replicas[0].
  I think we can change this to

  Replica *prim_replica;
  Replica **sec_replicas;

so we have one distinguished replica always available - and in the rare cases where we need more will allocate the secondary replicas

The patch, design and explanations are so nice !!
Few comments/questions:

• A csn is only valid in the topology where it was generated. To be sure a csn belongs to the right topology you are using 'prim_replica' and 'prim_repl'. IMHO using prim_repl word is a bit confusing. we do not care which replica generated the csn but we care about the topology where this replica belongs. I am fine keeping those names but do you think a macro could help to clarify this ?

• in ruv_cancel_csn_inprogress, we remove secondary_csn (children of prim_csn) at the condition the replica is not a consumer. Why this restriction ? an internal update on a consumer is it rejected ?

• csnpldata_free, you may comment that prim_replica and prim_csn should not be freed

A csn is only valid in the topology where it was generated. To be sure a csn belongs to the right topology you are using 'prim_replica' and 'prim_repl'. IMHO using prim_repl word is a bit confusing. we do not care which replica generated the csn but we care about the topology where this replica belongs. I am fine keeping those names but do you think a macro could help to clarify this ?

we are only operating in one server and with multiple backends we need to know on which backend the operation originated (this indirectly identifies the topology). When I use replica or repl it is a reference to the Replica object, which is corresponding to the backend. So I think it is correct to use the replica and I don't know a better name.
I don't see what ou want to replace by a macro and macros also are not always well received, so if you make a suggestion I will think about it

in ruv_cancel_csn_inprogress, we remove secondary_csn (children of prim_csn) at the condition the replica is not a consumer. Why this restriction ? an internal update on a consumer is it rejected ?

this condition did exist before my fix, I think the reason is that the read only replica is not part of the RUV and there are a few checks for this, but it is worth to look into it, if it could affect the csnpl handling, although I think not

csnpldata_free, you may comment that prim_replica and prim_csn should not be freed

will do

about the readonly replica: an operation in a read only replica doesn't get a csn assigned, so there is no csn ro insert or remove. The check seems to be useless. It might be a leftover of legacy replication, but not sure about this.
If you want I can remove this check.

I agree with you with the use of the replica pointer to identify on which backend (aka topology) belong the csn. I was just saying that pending list code is interested in backends and talking of replicas in that place looked to me a bit confusing.

I made a comment patch isolating the access to the replica info. It is not a macro but a wrapper just to comment that we want to check which backend/topology the csn belongs to.

thanks, that's looking good

I just made some changes to get it compiled, but I agree that your fix is more clear

I'm happy with the type changes you made here, so my side is ack, but I think tbordaz should check as well as his knowledge of this is better than mine.

The patch looks good to me as well --> ack

tentative patch to handle failueres in read only replicas

Hi,

The patch looks good. Just to be sure I understand it correctly.
You switched the order of csnplRemoveAll calls from local_rid first with prim_rid second.
Is the order important ? I believe it is not.

Now the fix prevents a call to ruvGetReplica with a read-only replica. I guess this is to prevent to get NULL repl_ruv and then dereference it. Am I correct ?

the order is not important, we need to prevent a call to remove the readonly replica id.
I changed the order as we always should have th eprimary rid to be removed and only if a replicated op generates a csn a different local rid.
We should probably do an extra check if the ruv element id not NULL, coverity will probably request this anyway

Thanks for the explanations, indeed checking returned repl_ruv from ruvGetReplica appears quite systematically in rest of the code. It would be great to add this in the committed patch.
ACK

Thanks,
here is a new version:

This is looking great !. ACK (already set)

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2346

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.