[PATCH] s4-drs: Store uSNUrgent for Urgent Replication

[tridge at samba.org]

Hi Fernando,

Thanks for the patch!

I've had a look at it, and I think your broad approach is OK, but some
of the details need fixing up. 

When you look at updating your patch based on the comments below, I
think you should also have a look at my drs-linked-attribs branch, as
that contains some changes that overlap with your changes. I think it
would be best if you rebase your changes on the drs-linked-attribs
branch (see my email to Eduardo for how to checkout that branch, in
case you are not familiar with tracking multiple branches in git).

Let's first look at your changes in dsdb/common/util.c. You've changed
dsdb_save_partition_usn() to take a "const char *uSN_name" argument to
say which attribute to update. This would work, but I think it would
be more efficient to update both the uSNHighest and uSNUrgent
attributes with the same modify operation. Most modifies will change
both urgent and non-urgent attributes, but we don't want to pay the
overhead of two ldb modifies per operation.

I think it would be better to change dsdb_save_partition_usn() to look
like this:

 int dsdb_save_partition_usn(struct ldb_context *ldb, struct ldb_dn *dn,
                             uint64_t uSN, uint64_t urgent_uSN);

if there are no urgent changes, then the caller can set urgent_uSN to
0, which would tell dsdb_save_partition_usn() to not set it.

You also need to modify dsdb_load_partition_usn(), to look like this:

 int dsdb_load_partition_usn(struct ldb_context *ldb, struct ldb_dn *dn,
                             uint64_t *uSN, uint64_t *urgent_uSN);

so that callers get both the urgent and non-urgent values back. Then
the code in dsdb/repl/ that calls this function will need to be
updated to take advantage of this change, setting the urgent bit in
the notify_sync call when appropriate.

Your changes in repl_meta_data.c also need a few changes I think. On a
minor level there are some coding style changes, for example:

 - you should use an enum for the REPL_URGENT_ON_* values, rather than
   a uint8_t.

 - instead of using ARRAY_SIZE(), I think its better to follow the
   conventions elsewhere in the ldb module code which use a NULL
   termination for arrays. That also allows you to use the existing
   helper function ldb_attr_in_list() to check if an attribute is
   urgent, instead of coding your own loop.

 - use talloc_new() instead of talloc_init() to get a temporary
   context. That will hang it off an existing context.

 - use ldb_attr_cmp() not strcasecmp() to compare attributes

In the replmd_check_urgent(), I don't see why you are doing an
ldb_search(). It looks like what you are doing is saying that a change
is urgent if the object contains any urgent attributes, whereas I
think the correct behaviour is that a change is urgent if any urgent
attributes are changed by the operation. So for example, if you change
just the 'description' field of user, then the change is not urgent,
even though the user object does contain 'urgent' attributes.

I also think that replmd_check_urgent() is hooking into the wrong
place in the code. Right now you hook into replmd_add() and
replmd_modify(), but I think you would be better off hooking into
replmd_update_rpmd_element() and into the existing loop over the
element array in replmd_add() (the latter could also be removed, if it
was changed to call replmd_update_rpmd_element()).

The reason for hooking in these places is that we only want to check
for updates to replicated attributes. If an attribute is not
replicated at all (which is the case for quite a few attributes) then
we should not consider the change of the attribute to be urgent. Doing
it this way also allows you to avoid the loop over the message, as
that is already being done.

I also think that instead of adding a new ncs_urg entry in struct
replmd_private you would be better off just adding uint64_t
mod_usn_urgent to the existing nc_entry structure. The semantics would
be that every change to a replicable attribute would increase
nc->mod_usn, but some would also increase nc->mod_usn_urgent. Then
when dsdb_save_partition_usn() would pass both values down for
updating the @REPLCHANGED object.

I hope the above is sufficient to put you on the right track! If you
still have questions on Monday, then I think this would be a good
patch to look over in more detail during our tutorial.

Cheers, Tridge