[389-devel] Review of plugin code

[William Brown]

On Thu, 2015-08-06 at 14:25 -0700, Noriko Hosoi wrote:
> Hi William,
> 
> Very interesting plug-in!

Thanks. As a plugin, it's value is quite useless due to the nsDS5ReplicaType
flags. But it's a nice simple exercise to get ones head around how the plugin
architecture works from scratch. It's one thing to patch a plugin, compared to
writing one from nothing.

> 
> Regarding betxn plug-in, it is for putting the entire operation -- the 
> primary update + associated updates by the enabled plug-ins -- in one 
> transaction.  By doing so, the entire updates are committed to the DB if 
> and only if all of the updates are successful. Otherwise, all of them 
> are rolled back.  That guarantees there will be no consistency among 
> entries.

Okay, so if I can be a pain, how to betxn handle reads? Do reads come from
within the transaction? Or is there a way to read from the database outside the
transaction. 

Say for example:

begin
add some object Y
read Y
commit

Does read Y see the object within the transaction? Is there a way to make the
search happen so that it occurs outside the transaction, IE it doesn't see Y?

> 
> In that sense, your read-only plug-in is not a good example for betxn 
> since it does not do any updates. :)  Considering the purpose of the 
> "read-only" plug-in, invoking it at the pre-op timing (before the 
> transaction) would be the best.

Very true! I kind of knew what betxn did, but I wanted to confirm more
completely in my mind. So I think what my read-only plugin does at the moment
works quite nicely then outside of betxn.

Is there a piece of documentation (perhaps the plugin guide) that lists the
order in which these operations are called? 

> 
> Since MEP requires the updates on the DB, it's supposed to be called in 
> betxn.  That way, what was done in the MEP plug-in is committed or 
> rolled back together with the primary updates.

Makes sense. 

> 
> The toughest part is the deadlock prevention.  At the start transaction, 
> it holds a DB lock.  And most plug-ins maintain its own mutex to protect 
> its resource.  It'd easily cause deadlock situation especially when 
> multiple plug-ins are enabled (which is common :). So, please be careful 
> not to acquire/free locks in the wrong order...

Of course. This is always an issue in multi-threaded code and anything with
locking. Stress tests are probably good to find these deadlocks, no?

> 
> About your commented out code in read_only.c, I guess you copied the 
> part from mep.c and are wondering what it is for?

> 
> There are various type of plug-ins.
> 
>     $ egrep nsslapd-pluginType dse.ldif | sort | uniq
>     nsslapd-pluginType: accesscontrol
>     nsslapd-pluginType: bepreoperation
>     nsslapd-pluginType: betxnpostoperation
>     nsslapd-pluginType: betxnpreoperation
>     nsslapd-pluginType: database
>     nsslapd-pluginType: extendedop
>     nsslapd-pluginType: internalpreoperation
>     nsslapd-pluginType: matchingRule
>     nsslapd-pluginType: object
>     nsslapd-pluginType: preoperation
>     nsslapd-pluginType: pwdstoragescheme
>     nsslapd-pluginType: reverpwdstoragescheme
>     nsslapd-pluginType: syntax
> 
> The reason why slapi_register_plugin and slapi_register_plugin_ext were 
> implemented was:
> 
>     /*
>       * Allows a plugin to register a plugin.
>       * This was added so that 'object' plugins could register all
>       * the plugin interfaces that it supports.
>       */
> 
> On the other hand, MEP has this type.
> 
>     nsslapd-pluginType: betxnpreoperation
> 
> The type is not "object", but the MEP plug-in is implemented as having 
> the type.  Originally, it might have been "object"...  Then, we 
> introduced the support for "betxn".  To make the transition to "betxn" 
> smoothly, we put the code to check "betxn" is in the type. If there is 
> "betxn" as in "betxnpreoperation", call the plug-in in betxn, otherwise 
> call them outside of the transaction.  Having the switch in the 
> configuration, we could go back to the original position without 
> rebuilding the plug-in.
> 
> Since we do not go back to pre-betxn era, the switch may not be too 
> important.  But keeping it would be a good idea for the consistency with 
> the other plug-ins.
> 
> Does this answer you question?  Please feel free to let us know if it 
> does not.

That answers some of my question. I guess the larger part of the question is how
the plugin subsystem treats each pluginType differently and the value of having
a plugin register to more than one pluginType. Are there some documents you can
point me to about this?

Additionally, with betxn, this seems quite black-or-white. It's either on a ds
instance that has betxn support, so every update will be betxn capable, or it's
not on such a system so you fall back to other methods. Is this correct? With
new plugins is it even worth writing them without betxn support? 


> I'm sure our team is interested in your idea and work, so let me share 
> your test plug-in with them.

Sure. It's not really that useful, like I said, nsDS5ReplicaType already does
this job. It was just an exercise to get my head more into the framework before
I work on http://directory.fedoraproject.org/docs/389ds/design/mep-rework.html

Thanks for the review, and for answering my questions. Your advice has helped a
lot! 

Sincerely,