389-ds-base

#50286 Ticket 50260 - Invalid cache flushing improvements
Closed by spichugi. Opened by mreynolds.
mreynolds/389-ds-base ticket50260  into  master

Download 50286.patch

Description:
The original version of the fix only checked if backend
transaction "post" operation plugins failed, but it did
not check for errors from the backend transaction "pre"
operation plugin. To address this we flush invalid
entries whenever any error occurs.

          We were also not flushing invalid cache entries when
          modrdn errors occurred.  Modrdns only make changes to
          the DN hashtable inside the entry cache, but we were only
          checking the ID hashtable.  So we also need to check the
          DN hashtable in the entry cache for invalid entries.

https://pagure.io/389-ds-base/issue/50260

rebased onto 423907e056b183631fc8e8e9e56040995ef5348c

rebased onto 8e9f0c74edbf52e21156d25d2a95099c9ce1f53a

Seems okay to me, @tbordaz ?

I agree with the testing in preop but are you sure we hit a cache corruption during a plugin preop ?
Which plugin updates an entry on a preop while it does not know the result of the operation ?

I think remove_it is not the appropriate flag. It is used for each entries in the previous loop. So if the last evaluated entry is "older" than the txn remove_it is set to 0 although the loop may have remove others entries.

Sorry I do not understand why this loop fix the new issue :(

The patch is about ENTRY cache. In entry cache, idtable and dntable hashtables are updated together and an backcommon entry is either present in both tables or missing in both. If in the previous loop the backcommon was older that the txn start time it will still be older in that loop. as far as I understand entrycache_remove/return will never be called.

Sorry I do not understand why this loop fix the new issue :(
The patch is about ENTRY cache. In entry cache, idtable and dntable hashtables are updated together and an backcommon entry is either present in both tables or missing in both. If in the previous loop the backcommon was older that the txn start time it will still be older in that loop. as far as I understand entrycache_remove/return will never be called.

The issue is when we check for entries to be removed... Previously we only checked the ID table, and not the DN table. But in the case of a failed modrdn in preop, the entry is only in the dn table, not in the ID table! So we were never flushing the invalid entry, and it was appearing in searches. So we need to check both the ID and DN tables for invalid entries.

I think remove_it is not the appropriate flag. It is used for each entries in the previous loop. So if the last evaluated entry is "older" than the txn remove_it is set to 0 although the loop may have remove others entries.

You are right I am not using it correctly, I will revise it...

I agree with the testing in preop but are you sure we hit a cache corruption during a plugin preop ?
Which plugin updates an entry on a preop while it does not know the result of the operation ?

This was a concern of mine as well. Should I just have checks for failures for be txn pre and post op plugins? Or, flush the cache on any failure? I was worried about other corner cases and thought it might be best to flush the cache on any failure (regardless of its origin).

Would you prefer I just have two checks in the backend functions: one for be txn pre op, the other for be txn post op?

rebased onto 20287294619bd1dbc63c027990c9bb6f7c5c6058

The patch looks good to me. ACK.

I think it is better to have a common function that flushes the cache whatever the origin (pre/post) of the failure. I am a bit concerned that we may an entry in dntable but not in idtable, but it is outside of the scope of the patch. If this condition exists your patch address it.

The two loops are almost doing the same thing, it can be more generic if we rely on 'e->ep_type' to know if it is entry/dn cache. But your patch looks more simple to read. Could you add a comment to make more clear that we are flushing taking into account idtable, then dntable.

Thanks. You have my ACK

The patch looks good to me. ACK.
I think it is better to have a common function that flushes the cache whatever the origin (pre/post) of the failure.

Well right now, it is generic. Any failure from a be txn plugin, or a non-be txn plugin, or any error condition, will trigger the cache flushing. I was wondering if you wanted me to just do the flushing if an error came from be txn pre/post plugins.

I am a bit concerned that we may an entry in dntable but not in idtable, but it is outside of the scope of the patch. If this condition exists your patch address it.

The current patch always checks the DN and ID hashtables in the entry cache. So even if we find entries to invalidate in the ID table, we will still check the DN table. So no matter what we always check both hashtables

The two loops are almost doing the same thing, it can be more generic if we rely on 'e->ep_type' to know if it is entry/dn cache. But your patch looks more simple to read. Could you add a comment to make more clear that we are flushing taking into account idtable, then dntable.

Yeah I will update the comments!

I think the fix should flush all entries updated by the initial write operation. So revert in error_return, like the patch is doing, looks good to me because at this point all updated db entries will be revert.

rebased onto 33fbced25277b88695bfba7262e606380e9d891f

Pull-Request has been merged by mreynolds

@mreynolds,
I can confirm that this patch fixed issue https://bugzilla.redhat.com/show_bug.cgi?id=1680245#c10

@mreynolds, you are nominated entry cache guru :)

@mreynolds, you are nominated entry cache guru :)

Why does that sound like a punishment and not a complement :-p

@mreynolds No good deed goes unpunished?

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3345

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.