[WIP] Make indexes more robust in failure cases.
Gary Lockyer
gary at catalyst.net.nz
Fri Mar 8 00:55:58 UTC 2019
A series of patches to ensure that the indexes are consistent in the
event of a failure during a modify or rename operation.
Extra tests to come, and I need to verify the memory management it very
probably has leaks.
I'm off on leave next week, so may not respond immediately to any comments.
CI currently underway at
https://gitlab.com/samba-team/devel/samba/pipelines/50847618
Ngā mihi
Gary
-------------- next part --------------
From 47820794b0a9f212a0422f6acd798b9459e13f10 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Wed, 6 Mar 2019 15:28:45 +1300
Subject: [PATCH 1/7] lib ldb tests: Test for index corruption in modify
Add test to test for index corruption in a failed modify.
Pair-Programmed-With: Andrew Bartlett <abartlet at samba.org>
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
lib/ldb/tests/python/api.py | 68 ++++++++++++++++++++
selftest/knownfail.d/test_modify_transaction | 3 +
2 files changed, 71 insertions(+)
create mode 100644 selftest/knownfail.d/test_modify_transaction
diff --git a/lib/ldb/tests/python/api.py b/lib/ldb/tests/python/api.py
index 0c4e269239b..ca267ae6758 100755
--- a/lib/ldb/tests/python/api.py
+++ b/lib/ldb/tests/python/api.py
@@ -2071,6 +2071,62 @@ class BadIndexTests(LdbBaseTest):
# https://bugzilla.samba.org/show_bug.cgi?id=13361
self.assertEquals(len(res), 4)
+ def test_modify_transaction(self):
+ self.ldb.add({"dn": "x=y,dc=samba,dc=org",
+ "objectUUID": b"0123456789abcde1",
+ "y": "2",
+ "z": "2"})
+
+ res = self.ldb.search(expression="(y=2)",
+ base="dc=samba,dc=org")
+ self.assertEqual(len(res), 1)
+
+ self.ldb.add({"dn": "@ATTRIBUTES",
+ "y": "UNIQUE_INDEX"})
+
+ self.ldb.transaction_start()
+
+ m = ldb.Message()
+ m.dn = ldb.Dn(self.ldb, "x=y,dc=samba,dc=org")
+ m["0"] = ldb.MessageElement([], ldb.FLAG_MOD_DELETE, "y")
+ m["1"] = ldb.MessageElement([], ldb.FLAG_MOD_DELETE, "not-here")
+
+ try:
+ self.ldb.modify(m)
+ self.fail()
+
+ except ldb.LdbError as err:
+ enum = err.args[0]
+ self.assertEqual(enum, ldb.ERR_NO_SUCH_ATTRIBUTE)
+
+ try:
+ self.ldb.transaction_commit()
+ # We should fail here, but we want to be sure
+ # we fail below
+
+ except ldb.LdbError as err:
+ enum = err.args[0]
+ self.assertEqual(enum, ldb.ERR_OPERATIONS_ERROR)
+
+ # The index should still be pointing to x=y
+ res = self.ldb.search(expression="(y=2)",
+ base="dc=samba,dc=org")
+ self.assertEqual(len(res), 1)
+
+ try:
+ self.ldb.add({"dn": "x=y2,dc=samba,dc=org",
+ "objectUUID": b"0123456789abcde2",
+ "y": "2"})
+ self.fail("Added unique attribute twice")
+ except ldb.LdbError as err:
+ enum = err.args[0]
+ self.assertEqual(enum, ldb.ERR_CONSTRAINT_VIOLATION)
+
+ res = self.ldb.search(expression="(y=2)",
+ base="dc=samba,dc=org")
+ self.assertEqual(len(res), 1)
+ self.assertEqual(str(res[0].dn), "x=y,dc=samba,dc=org")
+
def tearDown(self):
super(BadIndexTests, self).tearDown()
@@ -2084,6 +2140,18 @@ class GUIDBadIndexTests(BadIndexTests):
super(GUIDBadIndexTests, self).setUp()
+class GUIDBadIndexTestsLmdb(BadIndexTests):
+
+ def setUp(self):
+ self.prefix = MDB_PREFIX
+ self.index = MDB_INDEX_OBJ
+ self.IDXGUID = True
+ super(GUIDBadIndexTestsLmdb, self).setUp()
+
+ def tearDown(self):
+ super(GUIDBadIndexTestsLmdb, self).tearDown()
+
+
class DnTests(TestCase):
def setUp(self):
diff --git a/selftest/knownfail.d/test_modify_transaction b/selftest/knownfail.d/test_modify_transaction
new file mode 100644
index 00000000000..d53070a664b
--- /dev/null
+++ b/selftest/knownfail.d/test_modify_transaction
@@ -0,0 +1,3 @@
+^ldb.python.api.BadIndexTests.test_modify_transaction
+^ldb.python.api.GUIDBadIndexTests.test_modify_transaction
+^ldb.python.api.GUIDBadIndexTestsLmdb.test_modify_transaction
--
2.17.1
From 094c3e310e72b2169ec19a8f4aa39ff95bb98644 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Wed, 6 Mar 2019 15:32:08 +1300
Subject: [PATCH 2/7] lib ldb tests: Test nested transactions
Add a test to document that ldb does not currently support nested
transactions.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
lib/ldb/tests/python/api.py | 80 ++++++++++++++++++++++++
selftest/knownfail.d/nested_transactions | 8 +++
2 files changed, 88 insertions(+)
create mode 100644 selftest/knownfail.d/nested_transactions
diff --git a/lib/ldb/tests/python/api.py b/lib/ldb/tests/python/api.py
index ca267ae6758..e4feec5e68b 100755
--- a/lib/ldb/tests/python/api.py
+++ b/lib/ldb/tests/python/api.py
@@ -3097,6 +3097,86 @@ class VersionTests(TestCase):
def test_version(self):
self.assertTrue(isinstance(ldb.__version__, str))
+class NestedTransactionTests(LdbBaseTest):
+ def setUp(self):
+ super(NestedTransactionTests, self).setUp()
+ self.testdir = tempdir()
+ self.filename = os.path.join(self.testdir, "test.ldb")
+ self.ldb = ldb.Ldb(self.url(), flags=self.flags())
+ self.ldb.add({"dn": "@INDEXLIST",
+ "@IDXATTR": [b"x", b"y", b"ou"],
+ "@IDXGUID": [b"objectUUID"],
+ "@IDX_DN_GUID": [b"GUID"]})
+
+ super(NestedTransactionTests, self).setUp()
+
+ def test_nested_transactions(self):
+
+ self.ldb.transaction_start()
+
+ self.ldb.add({"dn": "x=x1,dc=samba,dc=org",
+ "objectUUID": b"0123456789abcde1"})
+ res = self.ldb.search(expression="(objectUUID=0123456789abcde1)",
+ base="dc=samba,dc=org")
+ self.assertEquals(len(res), 1)
+
+ self.ldb.add({"dn": "x=x2,dc=samba,dc=org",
+ "objectUUID": b"0123456789abcde2"})
+ res = self.ldb.search(expression="(objectUUID=0123456789abcde2)",
+ base="dc=samba,dc=org")
+ self.assertEquals(len(res), 1)
+
+ self.ldb.transaction_start()
+ self.ldb.add({"dn": "x=x3,dc=samba,dc=org",
+ "objectUUID": b"0123456789abcde3"})
+ res = self.ldb.search(expression="(objectUUID=0123456789abcde3)",
+ base="dc=samba,dc=org")
+ self.assertEquals(len(res), 1)
+ self.ldb.transaction_cancel()
+ #
+ # Check that we can not see the record added by the cancelled
+ # transaction.
+ # Currently this fails as ldb does not support true nested
+ # transactions, and only the outer commits and cancels have an effect
+ #
+ res = self.ldb.search(expression="(objectUUID=0123456789abcde3)",
+ base="dc=samba,dc=org")
+ self.assertEquals(len(res), 0)
+
+ #
+ # Commit the outer transaction
+ #
+ self.ldb.transaction_commit()
+ #
+ # Now check we can still see the records added in the outer
+ # transaction.
+ #
+ res = self.ldb.search(expression="(objectUUID=0123456789abcde1)",
+ base="dc=samba,dc=org")
+ self.assertEquals(len(res), 1)
+ res = self.ldb.search(expression="(objectUUID=0123456789abcde2)",
+ base="dc=samba,dc=org")
+ self.assertEquals(len(res), 1)
+ #
+ # And that we can't see the records added by the nested transaction.
+ #
+ res = self.ldb.search(expression="(objectUUID=0123456789abcde3)",
+ base="dc=samba,dc=org")
+ self.assertEquals(len(res), 0)
+
+ def tearDown(self):
+ super(NestedTransactionTests, self).tearDown()
+
+class LmdbNestedTransactionTests(NestedTransactionTests):
+
+ def setUp(self):
+ self.prefix = MDB_PREFIX
+ self.index = MDB_INDEX_OBJ
+ super(LmdbNestedTransactionTests, self).setUp()
+
+ def tearDown(self):
+ super(LmdbNestedTransactionTests, self).tearDown()
+
if __name__ == '__main__':
import unittest
diff --git a/selftest/knownfail.d/nested_transactions b/selftest/knownfail.d/nested_transactions
new file mode 100644
index 00000000000..aa96037f0d3
--- /dev/null
+++ b/selftest/knownfail.d/nested_transactions
@@ -0,0 +1,8 @@
+#
+# Currently LDB fakes nested transaction support by maintaining a count of
+# the number of nested transactions open.
+#
+# These failing tests serve to document this behaviour
+#
+^ldb.python.api.LmdbNestedTransactionTests.test_nested_transactions
+^ldb.python.api.NestedTransactionTests.test_nested_transactions
--
2.17.1
From 9af27ccd8609e3a6e9e3f124bdaf73eecfc3f6f0 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Wed, 6 Mar 2019 15:45:54 +1300
Subject: [PATCH 3/7] lib ldb tests: remove deprecation warning from api.py
Remove the "DeprecationWarning: Please use assertEqual instead."
warnings from api.py
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
lib/ldb/tests/python/api.py | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/lib/ldb/tests/python/api.py b/lib/ldb/tests/python/api.py
index e4feec5e68b..e8b98f014c8 100755
--- a/lib/ldb/tests/python/api.py
+++ b/lib/ldb/tests/python/api.py
@@ -1954,7 +1954,7 @@ class BadIndexTests(LdbBaseTest):
res = self.ldb.search(expression="(y=1)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 3)
+ self.assertEqual(len(res), 3)
# Now set this to unique index, but forget to check the result
try:
@@ -1967,7 +1967,7 @@ class BadIndexTests(LdbBaseTest):
# We must still have a working index
res = self.ldb.search(expression="(y=1)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 3)
+ self.assertEqual(len(res), 3)
def test_unique_transaction(self):
self.ldb.add({"dn": "x=x,dc=samba,dc=org",
@@ -1982,7 +1982,7 @@ class BadIndexTests(LdbBaseTest):
res = self.ldb.search(expression="(y=1)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 3)
+ self.assertEqual(len(res), 3)
self.ldb.transaction_start()
@@ -2005,7 +2005,7 @@ class BadIndexTests(LdbBaseTest):
res = self.ldb.search(expression="(y=1)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 3)
+ self.assertEqual(len(res), 3)
def test_casefold(self):
self.ldb.add({"dn": "x=x,dc=samba,dc=org",
@@ -2020,7 +2020,7 @@ class BadIndexTests(LdbBaseTest):
res = self.ldb.search(expression="(y=a)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 2)
+ self.assertEqual(len(res), 2)
self.ldb.add({"dn": "@ATTRIBUTES",
"y": "CASE_INSENSITIVE"})
@@ -2030,12 +2030,12 @@ class BadIndexTests(LdbBaseTest):
base="dc=samba,dc=org")
if hasattr(self, 'IDXGUID'):
- self.assertEquals(len(res), 3)
+ self.assertEqual(len(res), 3)
else:
# We should not return this entry twice, but sadly
# we have not yet fixed
# https://bugzilla.samba.org/show_bug.cgi?id=13361
- self.assertEquals(len(res), 4)
+ self.assertEqual(len(res), 4)
def test_casefold_transaction(self):
self.ldb.add({"dn": "x=x,dc=samba,dc=org",
@@ -2050,7 +2050,7 @@ class BadIndexTests(LdbBaseTest):
res = self.ldb.search(expression="(y=a)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 2)
+ self.assertEqual(len(res), 2)
self.ldb.transaction_start()
@@ -2064,12 +2064,12 @@ class BadIndexTests(LdbBaseTest):
base="dc=samba,dc=org")
if hasattr(self, 'IDXGUID'):
- self.assertEquals(len(res), 3)
+ self.assertEqual(len(res), 3)
else:
# We should not return this entry twice, but sadly
# we have not yet fixed
# https://bugzilla.samba.org/show_bug.cgi?id=13361
- self.assertEquals(len(res), 4)
+ self.assertEqual(len(res), 4)
def test_modify_transaction(self):
self.ldb.add({"dn": "x=y,dc=samba,dc=org",
@@ -3118,20 +3118,20 @@ class NestedTransactionTests(LdbBaseTest):
"objectUUID": b"0123456789abcde1"})
res = self.ldb.search(expression="(objectUUID=0123456789abcde1)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 1)
+ self.assertEqual(len(res), 1)
self.ldb.add({"dn": "x=x2,dc=samba,dc=org",
"objectUUID": b"0123456789abcde2"})
res = self.ldb.search(expression="(objectUUID=0123456789abcde2)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 1)
+ self.assertEqual(len(res), 1)
self.ldb.transaction_start()
self.ldb.add({"dn": "x=x3,dc=samba,dc=org",
"objectUUID": b"0123456789abcde3"})
res = self.ldb.search(expression="(objectUUID=0123456789abcde3)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 1)
+ self.assertEqual(len(res), 1)
self.ldb.transaction_cancel()
#
# Check that we can not see the record added by the cancelled
@@ -3141,7 +3141,7 @@ class NestedTransactionTests(LdbBaseTest):
#
res = self.ldb.search(expression="(objectUUID=0123456789abcde3)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 0)
+ self.assertEqual(len(res), 0)
#
# Commit the outer transaction
@@ -3153,16 +3153,16 @@ class NestedTransactionTests(LdbBaseTest):
#
res = self.ldb.search(expression="(objectUUID=0123456789abcde1)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 1)
+ self.assertEqual(len(res), 1)
res = self.ldb.search(expression="(objectUUID=0123456789abcde2)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 1)
+ self.assertEqual(len(res), 1)
#
# And that we can't see the records added by the nested transaction.
#
res = self.ldb.search(expression="(objectUUID=0123456789abcde3)",
base="dc=samba,dc=org")
- self.assertEquals(len(res), 0)
+ self.assertEqual(len(res), 0)
def tearDown(self):
super(NestedTransactionTests, self).tearDown()
--
2.17.1
From 0db4dce49e9802558a584ef66081623c41a1cce0 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu, 7 Mar 2019 10:18:00 +1300
Subject: [PATCH 4/7] lib ldb key value: Add limited nested transaction support
Add limited nested transaction support to the back ends to make the key value
operations atomic (for those back ends that support nested transactions).
Note: that only the lmdb backend currently supports nested transactions.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
lib/ldb/ldb_key_value/ldb_kv.h | 3 +++
lib/ldb/ldb_mdb/ldb_mdb.c | 34 ++++++++++++++++++++++++++++++++++
lib/ldb/ldb_tdb/ldb_tdb.c | 32 ++++++++++++++++++++++++++++++++
3 files changed, 69 insertions(+)
diff --git a/lib/ldb/ldb_key_value/ldb_kv.h b/lib/ldb/ldb_key_value/ldb_kv.h
index 5070a588c00..e3642ced792 100644
--- a/lib/ldb/ldb_key_value/ldb_kv.h
+++ b/lib/ldb/ldb_key_value/ldb_kv.h
@@ -41,6 +41,9 @@ struct kv_db_ops {
const char *(*name)(struct ldb_kv_private *ldb_kv);
bool (*has_changed)(struct ldb_kv_private *ldb_kv);
bool (*transaction_active)(struct ldb_kv_private *ldb_kv);
+ int (*begin_nested_write)(struct ldb_kv_private *);
+ int (*finish_nested_write)(struct ldb_kv_private *);
+ int (*abort_nested_write)(struct ldb_kv_private *);
};
/* this private structure is used by the key value backends in the
diff --git a/lib/ldb/ldb_mdb/ldb_mdb.c b/lib/ldb/ldb_mdb/ldb_mdb.c
index 646a67c554c..7104ee83928 100644
--- a/lib/ldb/ldb_mdb/ldb_mdb.c
+++ b/lib/ldb/ldb_mdb/ldb_mdb.c
@@ -576,6 +576,37 @@ static bool lmdb_changed(struct ldb_kv_private *ldb_kv)
return true;
}
+/*
+ * Start a sub transaction
+ * As lmdb supports nested transactions we can start a new transaction
+ */
+static int lmdb_nested_transaction_start(struct ldb_kv_private *ldb_kv)
+{
+ int ret = lmdb_transaction_start(ldb_kv);
+ return ret;
+}
+
+/*
+ * Commit a sub transaction
+ * As lmdb supports nested transactions we can commit the nested transaction
+ */
+static int lmdb_nested_transaction_commit(struct ldb_kv_private *ldb_kv)
+{
+ int ret = lmdb_transaction_commit(ldb_kv);
+ return ret;
+}
+
+/*
+ * Cancel a sub transaction
+ * As lmdb supports nested transactions we can cancel the nested transaction
+ */
+static int lmdb_nested_transaction_cancel(struct ldb_kv_private *ldb_kv)
+{
+ int ret = lmdb_transaction_cancel(ldb_kv);
+ return ret;
+}
+
+
static struct kv_db_ops lmdb_key_value_ops = {
.store = lmdb_store,
.delete = lmdb_delete,
@@ -593,6 +624,9 @@ static struct kv_db_ops lmdb_key_value_ops = {
.name = lmdb_name,
.has_changed = lmdb_changed,
.transaction_active = lmdb_transaction_active,
+ .begin_nested_write = lmdb_nested_transaction_start,
+ .finish_nested_write = lmdb_nested_transaction_commit,
+ .abort_nested_write = lmdb_nested_transaction_cancel,
};
static const char *lmdb_get_path(const char *url)
diff --git a/lib/ldb/ldb_tdb/ldb_tdb.c b/lib/ldb/ldb_tdb/ldb_tdb.c
index 812ddd3e389..fdcac0c7f7c 100644
--- a/lib/ldb/ldb_tdb/ldb_tdb.c
+++ b/lib/ldb/ldb_tdb/ldb_tdb.c
@@ -400,6 +400,35 @@ static bool ltdb_transaction_active(struct ldb_kv_private *ldb_kv)
return tdb_transaction_active(ldb_kv->tdb);
}
+/*
+ * Start a sub transaction
+ * As TDB does not currently support nested transactions, we do nothing and
+ * return LDB_SUCCESS
+ */
+static int ltdb_nested_transaction_start(struct ldb_kv_private *ldb_kv)
+{
+ return LDB_SUCCESS;
+}
+
+/*
+ * Commit a sub transaction
+ * As TDB does not currently support nested transactions, we do nothing and
+ * return LDB_SUCCESS
+ */
+static int ltdb_nested_transaction_commit(struct ldb_kv_private *ldb_kv)
+{
+ return LDB_SUCCESS;
+}
+
+/*
+ * Cancel a sub transaction
+ * As TDB does not currently support nested transactions, we do nothing and
+ * return LDB_SUCCESS
+ */
+static int ltdb_nested_transaction_cancel(struct ldb_kv_private *ldb_kv)
+{
+ return LDB_SUCCESS;
+}
static const struct kv_db_ops key_value_ops = {
.store = ltdb_store,
.delete = ltdb_delete,
@@ -417,6 +446,9 @@ static const struct kv_db_ops key_value_ops = {
.name = ltdb_name,
.has_changed = ltdb_changed,
.transaction_active = ltdb_transaction_active,
+ .begin_nested_write = ltdb_nested_transaction_start,
+ .finish_nested_write = ltdb_nested_transaction_commit,
+ .abort_nested_write = ltdb_nested_transaction_cancel,
};
/*
--
2.17.1
From 66c674bb0a5cbe1207ab1f5475ddec92e5bb9a16 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu, 7 Mar 2019 10:37:18 +1300
Subject: [PATCH 5/7] lid ldb key value: add nested transaction support.
Use the nested transaction support added to the key value back ends to
make key value operations atomic. This will ensure that rename
operation failures, which delete the original record and add a new
record, leave the database in a consistent state.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
lib/ldb/ldb_key_value/ldb_kv.c | 75 ++++++++++++++++++++++++++++++++++
lib/ldb/ldb_key_value/ldb_kv.h | 3 ++
2 files changed, 78 insertions(+)
diff --git a/lib/ldb/ldb_key_value/ldb_kv.c b/lib/ldb/ldb_key_value/ldb_kv.c
index d4f896736a2..db6506bbcff 100644
--- a/lib/ldb/ldb_key_value/ldb_kv.c
+++ b/lib/ldb/ldb_key_value/ldb_kv.c
@@ -432,6 +432,81 @@ static bool ldb_kv_single_valued(const struct ldb_schema_attribute *a,
return false;
}
+
+/*
+ * Place holder actual implementation to be added in subsequent commits
+ */
+int ldb_kv_index_sub_transaction_start(struct ldb_kv_private *ldb_kv)
+{
+ return LDB_SUCCESS;
+}
+/*
+ * Starts a sub transaction if they are supported by the backend
+ */
+static int ldb_kv_sub_transaction_start(struct ldb_kv_private *ldb_kv)
+{
+ int ret = LDB_SUCCESS;
+
+ ret = ldb_kv->kv_ops->begin_nested_write(ldb_kv);
+ if (ret == LDB_SUCCESS) {
+ ret = ldb_kv_index_sub_transaction_start(ldb_kv);
+ }
+ return ret;
+}
+
+/*
+ * Place holder actual implementation to be added in subsequent commits
+ */
+int ldb_kv_index_sub_transaction_commit(struct ldb_kv_private *ldb_kv)
+{
+ return LDB_SUCCESS;
+}
+/*
+ * Commits a sub transaction if they are supported by the backend
+ */
+static int ldb_kv_sub_transaction_commit(struct ldb_kv_private *ldb_kv)
+{
+ int ret = LDB_SUCCESS;
+
+ ret = ldb_kv_index_sub_transaction_commit(ldb_kv);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+ ret = ldb_kv->kv_ops->finish_nested_write(ldb_kv);
+ return ret;
+}
+
+/*
+ * Place holder actual implementation to be added in subsequent commits
+ */
+int ldb_kv_index_sub_transaction_cancel(struct ldb_kv_private *ldb_kv)
+{
+ return LDB_SUCCESS;
+}
+/*
+ * Cancels a sub transaction if they are supported by the backend
+ */
+static int ldb_kv_sub_transaction_cancel(struct ldb_kv_private *ldb_kv)
+{
+ int ret = LDB_SUCCESS;
+
+ ret = ldb_kv_index_sub_transaction_cancel(ldb_kv);
+ if (ret != LDB_SUCCESS) {
+ struct ldb_context *ldb = ldb_module_get_ctx(ldb_kv->module);
+ /*
+ * In the event of a failure we log the failure and continue
+ * as we need to cancel the database transaction.
+ */
+ ldb_debug(ldb,
+ LDB_DEBUG_ERROR,
+ __location__": ldb_kv_index_sub_transaction_cancel "
+ "failed: %s",
+ ldb_errstring(ldb));
+ }
+ ret = ldb_kv->kv_ops->abort_nested_write(ldb_kv);
+ return ret;
+}
+
static int ldb_kv_add_internal(struct ldb_module *module,
struct ldb_kv_private *ldb_kv,
const struct ldb_message *msg,
diff --git a/lib/ldb/ldb_key_value/ldb_kv.h b/lib/ldb/ldb_key_value/ldb_kv.h
index e3642ced792..3c1adbed23a 100644
--- a/lib/ldb/ldb_key_value/ldb_kv.h
+++ b/lib/ldb/ldb_key_value/ldb_kv.h
@@ -266,3 +266,6 @@ int ldb_kv_init_store(struct ldb_kv_private *ldb_kv,
struct ldb_context *ldb,
const char *options[],
struct ldb_module **_module);
+int ldb_kv_index_sub_transaction_start(struct ldb_kv_private *ldb_kv);
+int ldb_kv_index_sub_transaction_cancel(struct ldb_kv_private *ldb_kv);
+int ldb_kv_index_sub_transaction_commit(struct ldb_kv_private *ldb_kv);
--
2.17.1
From f83dbda3c0b58615a252ff3f08de86b44255a357 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu, 7 Mar 2019 10:40:59 +1300
Subject: [PATCH 6/7] lib ldb_key value: wrap operations
Wrap the key value operations in nested transactions to maintain
database consistency.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
lib/ldb/ldb_key_value/ldb_kv.c | 117 ++++++++++++++++++++++++++++-----
1 file changed, 101 insertions(+), 16 deletions(-)
diff --git a/lib/ldb/ldb_key_value/ldb_kv.c b/lib/ldb/ldb_key_value/ldb_kv.c
index db6506bbcff..1d2b51b3fb3 100644
--- a/lib/ldb/ldb_key_value/ldb_kv.c
+++ b/lib/ldb/ldb_key_value/ldb_kv.c
@@ -653,7 +653,23 @@ static int ldb_kv_add(struct ldb_kv_context *ctx)
return LDB_ERR_OPERATIONS_ERROR;
}
+ ret = ldb_kv_sub_transaction_start(ldb_kv);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
ret = ldb_kv_add_internal(module, ldb_kv, req->op.add.message, true);
+ if (ret != LDB_SUCCESS) {
+ int r = ldb_kv_sub_transaction_cancel(ldb_kv);
+ if (r != LDB_SUCCESS) {
+ ldb_debug(
+ ldb_module_get_ctx(module),
+ LDB_DEBUG_FATAL,
+ __location__
+ ": Unable to roll back sub transaction");
+ }
+ return ret;
+ }
+ ret = ldb_kv_sub_transaction_commit(ldb_kv);
return ret;
}
@@ -744,6 +760,9 @@ static int ldb_kv_delete(struct ldb_kv_context *ctx)
{
struct ldb_module *module = ctx->module;
struct ldb_request *req = ctx->req;
+ void *data = ldb_module_get_private(module);
+ struct ldb_kv_private *ldb_kv =
+ talloc_get_type(data, struct ldb_kv_private);
int ret = LDB_SUCCESS;
ldb_request_set_state(req, LDB_ASYNC_PENDING);
@@ -752,7 +771,23 @@ static int ldb_kv_delete(struct ldb_kv_context *ctx)
return LDB_ERR_OPERATIONS_ERROR;
}
+ ret = ldb_kv_sub_transaction_start(ldb_kv);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
ret = ldb_kv_delete_internal(module, req->op.del.dn);
+ if (ret != LDB_SUCCESS) {
+ int r = ldb_kv_sub_transaction_cancel(ldb_kv);
+ if (r != LDB_SUCCESS) {
+ ldb_debug(
+ ldb_module_get_ctx(module),
+ LDB_DEBUG_FATAL,
+ __location__
+ ": Unable to roll back sub transaction");
+ }
+ return ret;
+ }
+ ret = ldb_kv_sub_transaction_commit(ldb_kv);
return ret;
}
@@ -1282,6 +1317,9 @@ static int ldb_kv_modify(struct ldb_kv_context *ctx)
{
struct ldb_module *module = ctx->module;
struct ldb_request *req = ctx->req;
+ void *data = ldb_module_get_private(module);
+ struct ldb_kv_private *ldb_kv =
+ talloc_get_type(data, struct ldb_kv_private);
int ret = LDB_SUCCESS;
ret = ldb_kv_check_special_dn(module, req->op.mod.message);
@@ -1295,7 +1333,56 @@ static int ldb_kv_modify(struct ldb_kv_context *ctx)
return LDB_ERR_OPERATIONS_ERROR;
}
+ ret = ldb_kv_sub_transaction_start(ldb_kv);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
ret = ldb_kv_modify_internal(module, req->op.mod.message, req);
+ if (ret != LDB_SUCCESS) {
+ int r = ldb_kv_sub_transaction_cancel(ldb_kv);
+ if (r != LDB_SUCCESS) {
+ ldb_debug(
+ ldb_module_get_ctx(module),
+ LDB_DEBUG_FATAL,
+ __location__
+ ": Unable to roll back sub transaction");
+ }
+ return ret;
+ }
+ ret = ldb_kv_sub_transaction_commit(ldb_kv);
+
+
+ return ret;
+}
+
+static int ldb_kv_rename_internal(struct ldb_module *module,
+ struct ldb_request *req,
+ struct ldb_message *msg)
+{
+ void *data = ldb_module_get_private(module);
+ struct ldb_kv_private *ldb_kv =
+ talloc_get_type(data, struct ldb_kv_private);
+ int ret = LDB_SUCCESS;
+
+ /* Always delete first then add, to avoid conflicts with
+ * unique indexes. We rely on the transaction to make this
+ * atomic
+ */
+ ret = ldb_kv_delete_internal(module, msg->dn);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+
+ msg->dn = ldb_dn_copy(msg, req->op.rename.newdn);
+ if (msg->dn == NULL) {
+ return LDB_ERR_OPERATIONS_ERROR;
+ }
+
+ /* We don't check single value as we can have more than 1 with
+ * deleted attributes. We could go through all elements but that's
+ * maybe not the most efficient way
+ */
+ ret = ldb_kv_add_internal(module, ldb_kv, msg, false);
return ret;
}
@@ -1403,28 +1490,26 @@ static int ldb_kv_rename(struct ldb_kv_context *ctx)
talloc_free(key_old.data);
talloc_free(key.data);
- /* Always delete first then add, to avoid conflicts with
- * unique indexes. We rely on the transaction to make this
- * atomic
- */
- ret = ldb_kv_delete_internal(module, msg->dn);
+
+ ret = ldb_kv_sub_transaction_start(ldb_kv);
if (ret != LDB_SUCCESS) {
talloc_free(msg);
return ret;
}
-
- msg->dn = ldb_dn_copy(msg, req->op.rename.newdn);
- if (msg->dn == NULL) {
+ ret = ldb_kv_rename_internal(module, req, msg);
+ if (ret != LDB_SUCCESS) {
+ int r = ldb_kv_sub_transaction_cancel(ldb_kv);
+ if (r != LDB_SUCCESS) {
+ ldb_debug(
+ ldb_module_get_ctx(module),
+ LDB_DEBUG_FATAL,
+ __location__
+ ": Unable to roll back sub transaction");
+ }
talloc_free(msg);
- return LDB_ERR_OPERATIONS_ERROR;
+ return ret;
}
-
- /* We don't check single value as we can have more than 1 with
- * deleted attributes. We could go through all elements but that's
- * maybe not the most efficient way
- */
- ret = ldb_kv_add_internal(module, ldb_kv, msg, false);
-
+ ret = ldb_kv_sub_transaction_commit(ldb_kv);
talloc_free(msg);
return ret;
--
2.17.1
From 39192946b73f2e7204b875e9712440b4da17986e Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu, 7 Mar 2019 10:40:10 +1300
Subject: [PATCH 7/7] lib ldb key value: fix index buffering
As a performance enhancement the key value layer maintains a cache of
the index records, which is written to disk as part of a prepare commit.
This patch adds an extra cache at the operation layer to ensure that the
cached indexes remain consistent in the event of an operation failing.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
lib/ldb/ldb_key_value/ldb_kv.c | 22 ---
lib/ldb/ldb_key_value/ldb_kv.h | 12 ++
lib/ldb/ldb_key_value/ldb_kv_index.c | 161 ++++++++++++++++++-
selftest/knownfail.d/test_modify_transaction | 3 -
4 files changed, 166 insertions(+), 32 deletions(-)
delete mode 100644 selftest/knownfail.d/test_modify_transaction
diff --git a/lib/ldb/ldb_key_value/ldb_kv.c b/lib/ldb/ldb_key_value/ldb_kv.c
index 1d2b51b3fb3..d22b62999a5 100644
--- a/lib/ldb/ldb_key_value/ldb_kv.c
+++ b/lib/ldb/ldb_key_value/ldb_kv.c
@@ -432,14 +432,6 @@ static bool ldb_kv_single_valued(const struct ldb_schema_attribute *a,
return false;
}
-
-/*
- * Place holder actual implementation to be added in subsequent commits
- */
-int ldb_kv_index_sub_transaction_start(struct ldb_kv_private *ldb_kv)
-{
- return LDB_SUCCESS;
-}
/*
* Starts a sub transaction if they are supported by the backend
*/
@@ -454,13 +446,6 @@ static int ldb_kv_sub_transaction_start(struct ldb_kv_private *ldb_kv)
return ret;
}
-/*
- * Place holder actual implementation to be added in subsequent commits
- */
-int ldb_kv_index_sub_transaction_commit(struct ldb_kv_private *ldb_kv)
-{
- return LDB_SUCCESS;
-}
/*
* Commits a sub transaction if they are supported by the backend
*/
@@ -476,13 +461,6 @@ static int ldb_kv_sub_transaction_commit(struct ldb_kv_private *ldb_kv)
return ret;
}
-/*
- * Place holder actual implementation to be added in subsequent commits
- */
-int ldb_kv_index_sub_transaction_cancel(struct ldb_kv_private *ldb_kv)
-{
- return LDB_SUCCESS;
-}
/*
* Cancels a sub transaction if they are supported by the backend
*/
diff --git a/lib/ldb/ldb_key_value/ldb_kv.h b/lib/ldb/ldb_key_value/ldb_kv.h
index 3c1adbed23a..c643ae87367 100644
--- a/lib/ldb/ldb_key_value/ldb_kv.h
+++ b/lib/ldb/ldb_key_value/ldb_kv.h
@@ -72,7 +72,19 @@ struct ldb_kv_private {
bool check_base;
bool disallow_dn_filter;
+ /*
+ * To improve the performance of batch operations we maintain a cache
+ * of index records, these entries get written to disk in the
+ * prepare_commit phase.
+ */
struct ldb_kv_idxptr *idxptr;
+ /*
+ * To ensure that the indexes in idxptr are consistent we cache any
+ * index updates during an operation, i.e. ldb_kv_add, ldb_kv_delete ...
+ * Once the changes to the data record have been commited to disk
+ * the contents of this cache are copied to idxptr
+ */
+ struct ldb_kv_idxptr *nested_idx_ptr;
bool prepared_commit;
int read_lock_count;
diff --git a/lib/ldb/ldb_key_value/ldb_kv_index.c b/lib/ldb/ldb_key_value/ldb_kv_index.c
index 6d02c91a597..5b767e39d36 100644
--- a/lib/ldb/ldb_key_value/ldb_kv_index.c
+++ b/lib/ldb/ldb_key_value/ldb_kv_index.c
@@ -162,6 +162,11 @@ struct dn_list {
};
struct ldb_kv_idxptr {
+ /*
+ * In memory tdb to cache the index updates performed during a
+ * transaction. This improves the performance of operations like
+ * re-index and join
+ */
struct tdb_context *itdb;
int error;
};
@@ -353,7 +358,7 @@ static int ldb_kv_dn_list_load(struct ldb_module *module,
struct ldb_message *msg;
int ret, version;
struct ldb_message_element *el;
- TDB_DATA rec;
+ TDB_DATA rec = {0};
struct dn_list *list2;
TDB_DATA key;
@@ -368,7 +373,12 @@ static int ldb_kv_dn_list_load(struct ldb_module *module,
key.dptr = discard_const_p(unsigned char, ldb_dn_get_linearized(dn));
key.dsize = strlen((char *)key.dptr);
- rec = tdb_fetch(ldb_kv->idxptr->itdb, key);
+ if (ldb_kv->nested_idx_ptr->itdb != NULL) {
+ rec = tdb_fetch(ldb_kv->nested_idx_ptr->itdb, key);
+ }
+ if (rec.dptr == NULL) {
+ rec = tdb_fetch(ldb_kv->idxptr->itdb, key);
+ }
if (rec.dptr == NULL) {
goto normal_index;
}
@@ -709,14 +719,23 @@ static int ldb_kv_dn_list_store(struct ldb_module *module,
{
struct ldb_kv_private *ldb_kv = talloc_get_type(
ldb_module_get_private(module), struct ldb_kv_private);
- TDB_DATA rec, key;
- int ret;
+ TDB_DATA rec = {0};
+ TDB_DATA key = {0};
+
+ int ret = LDB_SUCCESS;
struct dn_list *list2;
if (ldb_kv->idxptr == NULL) {
return ldb_kv_dn_list_store_full(module, ldb_kv, dn, list);
}
+ if (ldb_kv->nested_idx_ptr->itdb == NULL) {
+ ldb_kv->nested_idx_ptr->itdb =
+ tdb_open(NULL, 10, TDB_INTERNAL, O_RDWR, 0);
+ if (ldb_kv->nested_idx_ptr->itdb == NULL) {
+ return LDB_ERR_OPERATIONS_ERROR;
+ }
+ }
if (ldb_kv->idxptr->itdb == NULL) {
ldb_kv->idxptr->itdb =
tdb_open(NULL, 1000, TDB_INTERNAL, O_RDWR, 0);
@@ -731,7 +750,12 @@ static int ldb_kv_dn_list_store(struct ldb_module *module,
}
key.dsize = strlen((char *)key.dptr);
- rec = tdb_fetch(ldb_kv->idxptr->itdb, key);
+ if( ldb_kv->nested_idx_ptr != NULL) {
+ rec = tdb_fetch(ldb_kv->nested_idx_ptr->itdb, key);
+ }
+ if (rec.dptr == NULL) {
+ rec = tdb_fetch(ldb_kv->idxptr->itdb, key);
+ }
if (rec.dptr != NULL) {
list2 = ldb_kv_index_idxptr(module, rec, false);
if (list2 == NULL) {
@@ -759,9 +783,13 @@ static int ldb_kv_dn_list_store(struct ldb_module *module,
* This is not a store into the main DB, but into an in-memory
* TDB, so we don't need a guard on ltdb->read_only
*/
- ret = tdb_store(ldb_kv->idxptr->itdb, key, rec, TDB_INSERT);
+ ret = tdb_store(ldb_kv->nested_idx_ptr->itdb,
+ key,
+ rec,
+ TDB_INSERT);
if (ret != 0) {
- return ltdb_err_map(tdb_error(ldb_kv->idxptr->itdb));
+ return ltdb_err_map(
+ tdb_error(ldb_kv->nested_idx_ptr->itdb));
}
return LDB_SUCCESS;
}
@@ -3169,3 +3197,122 @@ int ldb_kv_reindex(struct ldb_module *module)
}
return LDB_SUCCESS;
}
+
+/*
+ */
+static int ldb_kv_sub_transaction_traverse(struct tdb_context *tdb,
+ TDB_DATA key,
+ TDB_DATA data,
+ void *state)
+{
+ struct ldb_module *module = state;
+ struct ldb_dn *dn;
+ struct ldb_context *ldb = ldb_module_get_ctx(module);
+ struct ldb_kv_private *ldb_kv = talloc_get_type(
+ ldb_module_get_private(module), struct ldb_kv_private);
+ struct ldb_val v;
+ TDB_DATA rec;
+ struct dn_list *list;
+ struct dn_list *list2;
+ int ret;
+
+ list = ldb_kv_index_idxptr(module, data, true);
+ if (list == NULL) {
+ ldb_kv->idxptr->error = LDB_ERR_OPERATIONS_ERROR;
+ return -1;
+ }
+
+ v.data = key.dptr;
+ v.length = strnlen((char *)key.dptr, key.dsize);
+
+ dn = ldb_dn_from_ldb_val(module, ldb, &v);
+ if (dn == NULL) {
+ ldb_asprintf_errstring(
+ ldb,
+ "Failed to parse index key %*.*s as an LDB DN",
+ (int)v.length,
+ (int)v.length,
+ (const char *)v.data);
+ ldb_kv->nested_idx_ptr->error = LDB_ERR_OPERATIONS_ERROR;
+ return -1;
+ }
+
+ list2 = talloc(ldb_kv->idxptr, struct dn_list);
+ if (list2 == NULL) {
+ return LDB_ERR_OPERATIONS_ERROR;
+ }
+ list2->dn = talloc_steal(list2, list->dn);
+ list2->count = list->count;
+
+ rec.dptr = (uint8_t *)&list2;
+ rec.dsize = sizeof(void *);
+
+
+ /*
+ * This is not a store into the main DB, but into an in-memory
+ * TDB, so we don't need a guard on ltdb->read_only
+ */
+ ret = tdb_store(ldb_kv->idxptr->itdb, key, rec, TDB_INSERT);
+ talloc_free(dn);
+ if (ret != 0) {
+ ldb_kv->idxptr->error = ltdb_err_map(
+ tdb_error(ldb_kv->idxptr->itdb));
+ return -1;
+ }
+ return 0;
+}
+
+/*
+ * Initialise the index cache for a sub transaction.
+ */
+int ldb_kv_index_sub_transaction_start(struct ldb_kv_private *ldb_kv)
+{
+ ldb_kv->nested_idx_ptr = talloc_zero(ldb_kv, struct ldb_kv_idxptr);
+ if (ldb_kv->nested_idx_ptr == NULL) {
+ return LDB_ERR_OPERATIONS_ERROR;
+ }
+ return LDB_SUCCESS;
+}
+/* copy the contents of the index operation cache to the transaction
+ * index cache
+ */
+int ldb_kv_index_sub_transaction_cancel(struct ldb_kv_private *ldb_kv)
+{
+ if (ldb_kv->nested_idx_ptr->itdb != NULL) {
+ tdb_close(ldb_kv->nested_idx_ptr->itdb);
+ ldb_kv->nested_idx_ptr = NULL;
+ }
+ TALLOC_FREE(ldb_kv->nested_idx_ptr);
+ return LDB_SUCCESS;
+}
+
+/* copy the contents of the index operation cache to the transaction
+ * index cache
+ */
+int ldb_kv_index_sub_transaction_commit(struct ldb_kv_private *ldb_kv)
+{
+ int ret;
+
+ if (ldb_kv->nested_idx_ptr->itdb) {
+ tdb_traverse(
+ ldb_kv->nested_idx_ptr->itdb,
+ ldb_kv_sub_transaction_traverse,
+ ldb_kv->module);
+ tdb_close(ldb_kv->nested_idx_ptr->itdb);
+ ldb_kv->nested_idx_ptr->itdb = NULL;
+ }
+
+ ret = ldb_kv->nested_idx_ptr->error;
+ if (ret != LDB_SUCCESS) {
+ struct ldb_context *ldb = ldb_module_get_ctx(ldb_kv->module);
+ if (!ldb_errstring(ldb)) {
+ ldb_set_errstring(ldb, ldb_strerror(ret));
+ }
+ ldb_asprintf_errstring(
+ ldb,
+ __location__": Failed to update index records in "
+ "sub transaction commit: %s",
+ ldb_errstring(ldb));
+ }
+ return ret;
+}
diff --git a/selftest/knownfail.d/test_modify_transaction b/selftest/knownfail.d/test_modify_transaction
deleted file mode 100644
index d53070a664b..00000000000
--- a/selftest/knownfail.d/test_modify_transaction
+++ /dev/null
@@ -1,3 +0,0 @@
-^ldb.python.api.BadIndexTests.test_modify_transaction
-^ldb.python.api.GUIDBadIndexTests.test_modify_transaction
-^ldb.python.api.GUIDBadIndexTestsLmdb.test_modify_transaction
--
2.17.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190308/cb90e6bb/signature.sig>
More information about the samba-technical
mailing list