Issue 6430 - implement read-only bdb #6431

progier389 · 2024-12-03T12:50:38Z

This PR:

implement librobdb as derivated work from rpm lib
add configure flag to allow 389ds to use librobdb with some glue code interfacing bdb api toward the read only functions.
Modify spec file to build 389ds with that librairy if libdb is not available

Issue: #6430

Reviewed by: @tbordaz and @vashirov (Thanks)

tbordaz

The overall looks good. Few comments

tbordaz · 2024-12-04T10:15:40Z

ldap/servers/slapd/back-ldbm/db-bdb/bdb_bdbreader_glue.c

+#include "bdb_layer.h"
+#include <robdb.h>
+
+#define DEBUG 1


Shouldn't it be 0 by default

should not even be defined at all (since we are using #ifdef)

tbordaz · 2024-12-04T10:31:22Z

ldap/servers/slapd/back-ldbm/db-bdb/bdb_bdbreader_glue.c

+{
+    int rc = bdbreader_cur_lookup(db->cur->impl, key->data, key->size);
+    if (rc == DB_SUCCESS) {
+        rc = dbc_get(db->cur, key, data, flags);


Shouldn't it call db->cur->c_get than dbc_get ?

db->cur->c_get == dbc_get so it does not really matter.
But to respect the layered approach since we are in the glue code that maps bdb API to robdb API,
bdb_bdbreader_glue.c code should use either internal or librobdb function rather than the bdb API
(So dbc_get looks good.)

tbordaz · 2024-12-04T10:32:30Z

ldap/servers/slapd/back-ldbm/dbimpl.c

@@ -397,6 +397,7 @@ const char *dblayer_op2str(dbi_op_t op)
    return str[idx];
 }

+<<<<<<< HEAD


Probably a leftover from a merge.

Indeed: bad conflict resolution when rebasing to main ...

tbordaz · 2024-12-04T10:48:05Z

ldap/servers/slapd/back-ldbm/db-bdb/bdb_bdbreader_glue.c

+
+int dbc_close(DBC *dbc)
+{
+     bdbreader_bdb_close(dbc->impl);


Did you mean bdbreader_cur_close ?

progier389 · 2024-12-04T14:45:27Z

Fixed review cooments about DEBUG, bdbreader_cur_close, and rebase conflict issue

vashirov · 2024-12-04T14:40:45Z

rpm/389-ds-base.spec.in

@@ -540,11 +582,11 @@ if ! make DESTDIR="$RPM_BUILD_ROOT" check; then cat ./test-suite.log && false; f

 %post
 if [ -n "$DEBUGPOSTTRANS" ] ; then
-    output=$DEBUGPOSTTRANS


Indentation was removed here and below, is this intentional?

Good catch!

vashirov · 2024-12-04T14:41:41Z

lib/librobdb/robdb.spec

+Summary:        Provide basic functions to search and read Berkeley Database records
+
+License:        GPL-2.0-or-later
+URL:            https://github.com/389ds/389-ds-base/lib/librobdb


That is expected:
Right now, lib/librobdb does not exists yet on main branch, but it will exist once the PR get merged.

Then it would be something like this (note the tree and main branch in the URL):
https://github.com/389ds/389-ds-base/tree/main/lib/librobdb

lib/librobdb/robdb.spec

vashirov · 2024-12-04T14:47:44Z

lib/librobdb/robdb.spec

+%doc %{_defaultdocdir}/%{name}-devel/README.md
+%{_libdir}/*.so
+%{_includedir}/*
+


Since there is test.c for robdb, maybe we should run it as part of %check section?

Added a make localtest to be able to run the test without having to install the rpm first
and added a %check section to use it

progier389 · 2024-12-04T15:39:28Z

Pushed commit about Viktor's review comments

progier389 · 2024-12-05T14:16:49Z

Pushed new commit to fix the URL

tbordaz

Impressive patch. Copyrights may be set to 2025 (rather than 2024)
LGTM
(could you restart the (failing) tests in the PR before pushing)

vashirov

I can't build the package without robdb, configure picks it up anyway:

checking for db.h... using lib/librobdb/lib/robdb.h

And then fails with

error: Installed (but unpackaged) file(s) found:
   /usr/lib/debug/usr/lib64/dirsrv/librobdb.so-3.1.1.202501071235git0db4461d6-1.fc41.aarch64.debug
   /usr/lib64/dirsrv/librobdb.so

RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/lib/debug/usr/lib64/dirsrv/librobdb.so-3.1.1.202501071235git0db4461d6-1.fc41.aarch64.debug
   /usr/lib64/dirsrv/librobdb.so
make: *** [rpm.mk:178: rpms] Error 1

I provided some suggestions to the spec file, but I need to test this more.

rpm/389-ds-base.spec.in

vashirov · 2025-01-07T12:29:58Z

rpm.mk

+ifeq ($(BUNDLE_LIBDB), 1)
+BUNDLE_BDBREADERS ?= 0
+else
+ifeq (,$(wildcard /usr/include/db.h#))


Is # a typo? With this I'm always getting robdb-libs package generated.

Indeed! I think that I added the # for some testing and forgot to remove it afterwards 😉

rpm.mk

rpm/389-ds-base.spec.in

vashirov · 2025-01-07T12:44:04Z

Makefile.am

@@ -325,7 +332,7 @@ bin_PROGRAMS = dbscan \
 # based on defines
 # ----------------------------------------------------------------------------------------

-server_LTLIBRARIES = libslapd.la libldaputil.la libns-dshttpd.la librewriters.la
+server_LTLIBRARIES = libslapd.la libldaputil.la libns-dshttpd.la librewriters.la librobdb.la


Shouldn't this be also behind a condition?

Indeed! I will fix it.

Co-authored-by: Viktor Ashirov <[email protected]>

Issue 6430 - implement read-only bdb

172c87f

progier389 force-pushed the i0_bdb_ro branch from 94576bf to 172c87f Compare December 3, 2024 12:56

progier389 linked an issue Dec 3, 2024 that may be closed by this pull request

Solution to be able to upgrade existing instance when bdb will be no more available #6430

Open

tbordaz reviewed Dec 4, 2024

View reviewed changes

vashirov reviewed Dec 4, 2024

View reviewed changes

issue 6430 - Fix review comments

875f395

progier389 force-pushed the i0_bdb_ro branch from e38e67e to 875f395 Compare December 4, 2024 15:35

Issue 6430 - Fix spec file URL

0db4461

tbordaz approved these changes Jan 7, 2025

View reviewed changes

vashirov requested changes Jan 7, 2025

View reviewed changes

Apply Viktor's suggestions from code review

d633be9

Co-authored-by: Viktor Ashirov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 6430 - implement read-only bdb #6431

Issue 6430 - implement read-only bdb #6431

progier389 commented Dec 3, 2024 •

edited

Loading

tbordaz left a comment

tbordaz Dec 4, 2024

progier389 Dec 4, 2024

tbordaz Dec 4, 2024

progier389 Dec 4, 2024

tbordaz Dec 4, 2024

progier389 Dec 4, 2024

tbordaz Dec 4, 2024

progier389 commented Dec 4, 2024

vashirov Dec 4, 2024

progier389 Dec 4, 2024

vashirov Dec 4, 2024

progier389 Dec 4, 2024

vashirov Dec 5, 2024

vashirov Dec 4, 2024

progier389 Dec 4, 2024

progier389 commented Dec 4, 2024

progier389 commented Dec 5, 2024

tbordaz left a comment •

edited

Loading

vashirov left a comment

vashirov Jan 7, 2025

progier389 Jan 9, 2025

vashirov Jan 7, 2025

progier389 Jan 9, 2025

Issue 6430 - implement read-only bdb #6431

Are you sure you want to change the base?

Issue 6430 - implement read-only bdb #6431

Conversation

progier389 commented Dec 3, 2024 • edited Loading

tbordaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progier389 commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progier389 commented Dec 4, 2024

progier389 commented Dec 5, 2024

tbordaz left a comment • edited Loading

Choose a reason for hiding this comment

vashirov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progier389 commented Dec 3, 2024 •

edited

Loading

tbordaz left a comment •

edited

Loading