openldap-bugs

openldap-bugs@openldap.org

1 participants
20960 discussions

Re: (ITS#7775) LMDB terminates Postfix daemon process without logfile record
by h.b.furuseth＠usit.uio.no 07 Jan '14

07 Jan '14

Pushed to mdb.master. void assert_callback(MDB_env *env, const char *msg) { void *ctx = mdb_env_get_userctx(env); ...; } mdb_env_create(&env); mdb_env_set_userctx(env, ctx); mdb_env_set_assert(env, assert_callback); -- Hallvard

1 0

Re: (ITS#7775) LMDB terminates Postfix daemon process without logfile record
by h.b.furuseth＠usit.uio.no 05 Jan '14

05 Jan '14

On 2014-01-05 20:03, hyc(a)symas.com wrote: > I made this comment to Hallvard as well. It should be: > > MDB_assert_func * > mdb_env_set_assert(MDB_env *env, MDB_assert_func *func, void *info); Yes, I missed that. Hadn't intended to take this ITS into such a detour. Maybe such a context argument can be useful outside asserts too though, like a Python wrapper which wants the Python element wrapping the MDB_env of a given txn. If so, instead of passing the context to mdb_env_set_assert(): int mdb_env_set_userctx(env, void *ctx); void *mdb_env_get_userctx(env); assert() could still pass it out to the assert fallback. -- Hallvard

1 0

Re: (ITS#7775) LMDB terminates Postfix daemon process without logfile record
by hyc＠symas.com 05 Jan '14

05 Jan '14

wietse(a)porcupine.org wrote: > Wietse Venema: >>> env can be NULL. mdb_env_set_assert(env,cb) also sets a static >>> variable with the callback for the asserts which do not know of an >>> env. Could throw away that part later. Currently it only applies >>> to one assert(). >>> >>> The branch also invalidates a transaction on any page allocation >>> error, since the callers do not always clean up afterwards. Too >>> aggressive, can be reverted after updating the callers. But for >>> now it means MDB_MAP_FULL & co won't let the user commit an >>> inconsistent transaction. >> >> Thanks, this is more improvement than I asked for. I don't want to >> slip my schedule, and to avoid last-minute surprises, I will postpone >> the review and impact test of these improvements after the stable >> Postfix release 2-3 weeks from today. > > It looks like you are switching from a stop-gap fix (bare-bones > call-back) to a more permanent solution. > > In that case I must mention again that I need to register not only > a call-back function pointer but also a void* pointer that is > returned to me at call-back time. > > Getting the MDB_env at call-back time does not help me at all, > because my programs open many databases (including non-LMDB). I made this comment to Hallvard as well. It should be: MDB_assert_func * mdb_env_set_assert(MDB_env *env, MDB_assert_func *func, void *info); with the info being passed thru to the callback. > To find out which database is affected I need my own pointer to my > own (DICT) object that encapsulates the LMDB database. Then I can > decide if the problem is with a disposable cache. in which case the > program can continue with degraded performance. > > Wietse > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Re: (ITS#7775) LMDB terminates Postfix daemon process without logfile record
by wietse＠porcupine.org 05 Jan '14

05 Jan '14

Wietse Venema: > > env can be NULL. mdb_env_set_assert(env,cb) also sets a static > > variable with the callback for the asserts which do not know of an > > env. Could throw away that part later. Currently it only applies > > to one assert(). > > > > The branch also invalidates a transaction on any page allocation > > error, since the callers do not always clean up afterwards. Too > > aggressive, can be reverted after updating the callers. But for > > now it means MDB_MAP_FULL & co won't let the user commit an > > inconsistent transaction. > > Thanks, this is more improvement than I asked for. I don't want to > slip my schedule, and to avoid last-minute surprises, I will postpone > the review and impact test of these improvements after the stable > Postfix release 2-3 weeks from today. It looks like you are switching from a stop-gap fix (bare-bones call-back) to a more permanent solution. In that case I must mention again that I need to register not only a call-back function pointer but also a void* pointer that is returned to me at call-back time. Getting the MDB_env at call-back time does not help me at all, because my programs open many databases (including non-LMDB). To find out which database is affected I need my own pointer to my own (DICT) object that encapsulates the LMDB database. Then I can decide if the problem is with a disposable cache. in which case the program can continue with degraded performance. Wietse

1 0

Re: (ITS#7775) LMDB terminates Postfix daemon process without logfile record
by wietse＠porcupine.org 05 Jan '14

05 Jan '14

Hallvard Breien Furuseth: > Drafted a branch "mdb/assert" in > <http://folk.uio.no/hbf/OpenLDAP/mdb.git/> and on Ada, based > roughly on Howard's original message to you. I didn't know about > that at first, which has caused some confusion. > > So the callback usually receives an env after all, and a NULL > context since I don't know what Howard intended there. > > Howard, do I push this? Feel free to grab it and tweak a bit:) > > env can be NULL. mdb_env_set_assert(env,cb) also sets a static > variable with the callback for the asserts which do not know of an > env. Could throw away that part later. Currently it only applies > to one assert(). > > The branch also invalidates a transaction on any page allocation > error, since the callers do not always clean up afterwards. Too > aggressive, can be reverted after updating the callers. But for > now it means MDB_MAP_FULL & co won't let the user commit an > inconsistent transaction. Thanks, this is more improvement than I asked for. I don't want to slip my schedule, and to avoid last-minute surprises, I will postpone the review and impact test of these improvements after the stable Postfix release 2-3 weeks from today. Wietse

1 0

Re: (ITS#7775) LMDB terminates Postfix daemon process without logfile record
by h.b.furuseth＠usit.uio.no 05 Jan '14

05 Jan '14

Drafted a branch "mdb/assert" in <http://folk.uio.no/hbf/OpenLDAP/mdb.git/> and on Ada, based roughly on Howard's original message to you. I didn't know about that at first, which has caused some confusion. So the callback usually receives an env after all, and a NULL context since I don't know what Howard intended there. Howard, do I push this? Feel free to grab it and tweak a bit:) env can be NULL. mdb_env_set_assert(env,cb) also sets a static variable with the callback for the asserts which do not know of an env. Could throw away that part later. Currently it only applies to one assert(). The branch also invalidates a transaction on any page allocation error, since the callers do not always clean up afterwards. Too aggressive, can be reverted after updating the callers. But for now it means MDB_MAP_FULL & co won't let the user commit an inconsistent transaction. -- Hallvard

1 0

Re: (ITS#7774) LMDB assertion failure during Postfix cache cleanup
by wietse＠porcupine.org 03 Jan '14

03 Jan '14

Hallvard Breien Furuseth: > I wrote: > >Wietse Venema writes: > >> I can confirm that with LMDB-0.9.10 + ITS#7756 patch, the assertion > >> failure goes away when I close the cursor (and its read transaction) > >> before changing the map or the database, and restore the cursor > >> afterwards. I guess this got broken when things were changed to use > >> external locks in order to avoid world-writable lockfiles. > > Does this mean we can close this ITS? You can close this ITS. Wietse

1 0

Re: (ITS#7775) LMDB terminates Postfix daemon process without logfile record
by wietse＠porcupine.org 03 Jan '14

03 Jan '14

Hallvard Breien Furuseth: > wietse(a)porcupine.org writes: > > Below is my "least painful" patch for both parties. What do you think? > > That's roughly what I wrote was thinking, yes. > > We haven't heard if it will go in at all, though:-) If this ITS > is a reaction to ITS#7774, then the motivation is not catching > an lmdb bug, but letting the user band-aid a user error. Still > makes sense to me, lmdb lets the user screw up in many ways. I agree that we're talking about a stop-gap API in the absence of better error reporting. And I am not talking about users violating the C language model with invalid pointers and such; that would rightfully deserve a core dump. > --- a/libraries/liblmdb/mdb.c > +++ b/libraries/liblmdb/mdb.c Based on visual inspection, that code would work for me. Now the question for me is whether this will be available by the time Postfix 2.11 is released near the end of this month. Code freeze begins in days. I can't distribute LMDB support when daemons fall out of the sky without logfile message, whether it's your mistake, mine, or some middleperson distro maintainer. Prior art: Postfix does not support GnuTLS for the same reason. That would be a support problem. Wietse

1 0

Re: (ITS#7774) LMDB assertion failure during Postfix cache cleanup
by h.b.furuseth＠usit.uio.no 03 Jan '14

03 Jan '14

I wrote: >Wietse Venema writes: >> I can confirm that with LMDB-0.9.10 + ITS#7756 patch, the assertion >> failure goes away when I close the cursor (and its read transaction) >> before changing the map or the database, and restore the cursor >> afterwards. I guess this got broken when things were changed to use >> external locks in order to avoid world-writable lockfiles. Does this mean we can close this ITS? I made a major typo here: > Yes. > > Ending the read-only transaction and starting a new one means you are > no longer seeing the same snapshot, though. So I hope the reason you > couldn't use single transaction which I suggested previously, was **not** > that > the reader transaction needed to keep seeing the same snapshot. -- Hallvard

1 0

Re: (ITS#7775) LMDB terminates Postfix daemon process without logfile record
by h.b.furuseth＠usit.uio.no 03 Jan '14

03 Jan '14

wietse(a)porcupine.org writes: > Below is my "least painful" patch for both parties. What do you think? That's roughly what I wrote was thinking, yes. We haven't heard if it will go in at all, though:-) If this ITS is a reaction to ITS#7774, then the motivation is not catching an lmdb bug, but letting the user band-aid a user error. Still makes sense to me, lmdb lets the user screw up in many ways. Anyway: Why drop the info parameter? If you don't need it after all, that's fine. But then it won't be coming back later. It doesn't fit in the varargs, they're reserved for the message. I ended up declaring a type for it in ldmb.h, to make future expansion a bit friendlier: /** Dummy definition for future extension of #mdb_assert_cb(). */ typedef struct MDB_assertion MDB_assertion; I dislike when a feature makes me #include <stdarg.h>, so I'd not make one myself. I'd build the message in a buffer. Also the code needs fleshing out a bit: Obey NDEBUG, and omit all this if something already #defined assert. See below. Finally, __func__ needs a wrapper. It can go in a renamed "compat" section: --- a/libraries/liblmdb/mdb.c +++ b/libraries/liblmdb/mdb.c @@ -151,3 +150,3 @@ */ -/** @defgroup compat Windows Compatibility Macros +/** @defgroup compat Compatibility Macros * A bunch of macros to minimize the amount of platform-specific ifdefs @@ -158,2 +157,13 @@ */ + + /** Wrapper around __func__, which is a C99 feature */ +#if __STDC_VERSION__ >= 199901L +# define mdb_func_ __func__ +#elif __GNUC__ >= 2 || _MSC_VER >= 1300 +# define mdb_func_ __FUNCTION__ +#else +/* If a debug message says <mdb_unknown>(), update the #if statements above */ +# define mdb_func_ "<mdb_unknown>" +#endif + #ifdef _WIN32 @@ -329,3 +339,3 @@ static txnid_t mdb_debug_start; # define DPRINTF0(fmt, ...) \ - fprintf(stderr, "%s:%d " fmt "\n", __func__, __LINE__, __VA_ARGS__) + fprintf(stderr, "%s:%d " fmt "\n", mdb_func_, __LINE__, __VA_ARGS__) #else Google found more compiler #ifdefs, but I'm not sure which of these are still relevant as non-C99 compilers. Could add them later at need: __func__: (defined(__SUNPRO_C) && defined(__C99FEATURES__)) __FUNC__: __BORLANDC__ >= 0x550 __FUNCTION__: __MWERKS__ >= 0x3000 || defined(__ghs__) || __DMC__ >= 0x810 || __IBMCPP__ >= 500 And the assert() itself: /** assert(3) replacement. * Omitted in programs that #include mdb.c and already define assert(). */ #ifndef assert # ifdef NDEBUG # define assert(expr) ((void) 0) # else # define assert(expr) ((expr) ? (void)0 : \ mdb_assert_fail(#expr, mdb_func_, __FILE__, __LINE__)) static void mdb_assert_fail(const char *expr, const char *func, const char *file, int line) { char buf[400]; sprintf(buf, "%.100s:%d: Assertion '%.200s' failed in %.40s()", file, line, expr, func); if (mdb_assert_cb) mdb_assert_cb(buf, NULL); fprintf(stderr, "%s\n", buf); abort(); } # endif /* NDEBUG */ #endif /* assert */ -- Hallvard

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

openldap-bugs