half-atomically rebuild databases

half-atomically rebuild databases

[Ingo Schwarze]

Hi,

when mandocdb(8) rebuilds a database, the old database is already
truncated during the build, so you can't use apropos(1) at that
time, and if mandocdb fails, you have lost your database.

Improve this by first building the database in temporary files,
then rename(2)ing them into place when they are ready.

This is not perfect because the mandocdb process might get -KILLed
between the two renames, but i don't see a syscall anywhere to
atomically rename *two* files.

Also, now it turns out why using mandoc_malloc and friends in
a library is maybe not that smart.  If the library exits,
we have no chance to unlink(2) the temporary files.  Well, we
could rely on atexit(3), but that's exceedingly ugly, and even
the atexit(3) manual itself strongly discourages its use.

All that said, i think the patch below is an improvement.
But it's surprisingly large, so i'd like having somebody
look it over before commit.

Thanks,
  Ingo


Index: mandocdb.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mandocdb.c,v
retrieving revision 1.33
diff -u -p -r1.33 mandocdb.c
--- mandocdb.c	28 Dec 2011 01:17:01 -0000	1.33
+++ mandocdb.c	1 Jan 2012 15:30:54 -0000
@@ -21,6 +21,7 @@
 #include <assert.h>
 #include <ctype.h>
 #include <dirent.h>
+#include <errno.h>
 #include <fcntl.h>
 #include <getopt.h>
 #include <stdio.h>
@@ -382,10 +383,6 @@ mandocdb(int argc, char *argv[])
 	buf.cp = mandoc_malloc(buf.size);
 	dbuf.cp = mandoc_malloc(dbuf.size);
 
-	flags = O_CREAT | O_RDWR;
-	if (OP_DEFAULT == op || OP_CONFFILE == op)
-		flags |= O_TRUNC;
-
 	if (OP_TEST == op) {
 		ofile_argbuild(argc, argv, &of);
 		if (NULL == of)
@@ -408,6 +405,7 @@ mandocdb(int argc, char *argv[])
 			exit((int)MANDOCLEVEL_BADARG);
 		}
 
+		flags = O_CREAT | O_RDWR;
 		mdb.db = dbopen(mdb.dbn, flags, 0644, DB_BTREE, &info);
 		mdb.idx = dbopen(mdb.idxn, flags, 0644, DB_RECNO, NULL);
 
@@ -464,62 +462,89 @@ mandocdb(int argc, char *argv[])
 		manpath_parse(&dirs, dir, NULL, NULL);
 
 	for (i = 0; i < dirs.sz; i++) {
-		mdb.idxn[0] = mdb.dbn[0] = '\0';
-
-		strlcat(mdb.dbn, dirs.paths[i], MAXPATHLEN);
-		strlcat(mdb.dbn, "/", MAXPATHLEN);
-		sz1 = strlcat(mdb.dbn, MANDOC_DB, MAXPATHLEN);
 
-		strlcat(mdb.idxn, dirs.paths[i], MAXPATHLEN);
-		strlcat(mdb.idxn, "/", MAXPATHLEN);
-		sz2 = strlcat(mdb.idxn, MANDOC_IDX, MAXPATHLEN);
+		/*
+		 * Go to the root of the respective manual tree.
+		 * This must work or no manuals may be found:
+		 * They are indexed relative to the root.
+		 */
 
-		if (sz1 >= MAXPATHLEN || sz2 >= MAXPATHLEN) {
-			fprintf(stderr, "%s: path too long\n",
-					dirs.paths[i]);
-			exit((int)MANDOCLEVEL_BADARG);
+		if (-1 == chdir(dirs.paths[i])) {
+			perror(dirs.paths[i]);
+			exit((int)MANDOCLEVEL_SYSERR);
 		}
 
-		if (mdb.db)
-			(*mdb.db->close)(mdb.db);
-		if (mdb.idx)
-			(*mdb.idx->close)(mdb.idx);
+		/* Create a new database in two temporary files. */
 
-		mdb.db = dbopen(mdb.dbn, flags, 0644, DB_BTREE, &info);
-		mdb.idx = dbopen(mdb.idxn, flags, 0644, DB_RECNO, NULL);
-
-		if (NULL == mdb.db) {
-			perror(mdb.dbn);
-			exit((int)MANDOCLEVEL_SYSERR);
-		} else if (NULL == mdb.idx) {
-			perror(mdb.idxn);
-			exit((int)MANDOCLEVEL_SYSERR);
+		flags = O_CREAT | O_EXCL | O_RDWR;
+		while (NULL == mdb.db) {
+			strlcpy(mdb.dbn, MANDOC_DB, MAXPATHLEN);
+			strlcat(mdb.dbn, ".XXXXXXXXXX", MAXPATHLEN);
+			if (NULL == mktemp(mdb.dbn)) {
+				perror(mdb.dbn);
+				exit((int)MANDOCLEVEL_SYSERR);
+			}
+			mdb.db = dbopen(mdb.dbn, flags, 0644,
+					DB_BTREE, &info);
+			if (NULL == mdb.db && EEXIST != errno) {
+				perror(mdb.dbn);
+				exit((int)MANDOCLEVEL_SYSERR);
+			}
 		}
+		while (NULL == mdb.idx) {
+			strlcpy(mdb.idxn, MANDOC_IDX, MAXPATHLEN);
+			strlcat(mdb.idxn, ".XXXXXXXXXX", MAXPATHLEN);
+			if (NULL == mktemp(mdb.idxn)) {
+				perror(mdb.idxn);
+				unlink(mdb.dbn);
+				exit((int)MANDOCLEVEL_SYSERR);
+			}
+			mdb.idx = dbopen(mdb.idxn, flags, 0644,
+					DB_RECNO, NULL);
+			if (NULL == mdb.idx && EEXIST != errno) {
+				perror(mdb.idxn);
+				unlink(mdb.dbn);
+				exit((int)MANDOCLEVEL_SYSERR);
+			}
+		}
+
+		/*
+		 * Search for manuals and fill the new database.
+		 */
 
-		ofile_free(of);
-		of = NULL;
+	       	ofile_dirbuild(".", "", "", 0, &of);
 
-		if (-1 == chdir(dirs.paths[i])) {
-			perror(dirs.paths[i]);
-			exit((int)MANDOCLEVEL_SYSERR);
+		if (NULL != of) {
+			index_merge(of, mp, &dbuf, &buf, hash,
+			     &mdb, &recs);
+			ofile_free(of);
+			of = NULL;
 		}
 
-	       	ofile_dirbuild(".", "", "", 0, &of);
-		if (NULL == of)
-			continue;
+		(*mdb.db->close)(mdb.db);
+		(*mdb.idx->close)(mdb.idx);
+		mdb.db = NULL;
+		mdb.idx = NULL;
 
 		/*
-		 * Go to the root of the respective manual tree.
-		 * This must work or no manuals may be found (they're
-		 * indexed relative to the root).
+		 * Replace the old database with the new one.
+		 * This is not perfectly atomic,
+		 * but i cannot think of a better way.
 		 */
 
-		if (-1 == chdir(dirs.paths[i])) {
-			perror(dirs.paths[i]);
+		if (-1 == rename(mdb.dbn, MANDOC_DB)) {
+			perror(MANDOC_DB);
+			unlink(mdb.dbn);
+			unlink(mdb.idxn);
+			exit((int)MANDOCLEVEL_SYSERR);
+		}
+		if (-1 == rename(mdb.idxn, MANDOC_IDX)) {
+			perror(MANDOC_IDX);
+			unlink(MANDOC_DB);
+			unlink(MANDOC_IDX);
+			unlink(mdb.idxn);
 			exit((int)MANDOCLEVEL_SYSERR);
 		}
-
-		index_merge(of, mp, &dbuf, &buf, hash, &mdb, &recs);
 	}
 
 out:
@@ -737,6 +762,8 @@ index_merge(const struct of *of, struct 
 		}
 		if (ch < 0) {
 			perror("hash");
+			unlink(mdb->dbn);
+			unlink(mdb->idxn);
 			exit((int)MANDOCLEVEL_SYSERR);
 		}
 
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Joerg Sonnenberger]

On Sun, Jan 01, 2012 at 04:45:38PM +0100, Ingo Schwarze wrote:
> This is not perfect because the mandocdb process might get -KILLed
> between the two renames, but i don't see a syscall anywhere to
> atomically rename *two* files.

Why do you have to? You can create the new file under temporary name in
the same directory and rename it to the normal file name.

Joerg
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Ingo Schwarze]

Hi Joerg,

Joerg Sonnenberger wrote on Sun, Jan 01, 2012 at 04:55:08PM +0100:
> On Sun, Jan 01, 2012 at 04:45:38PM +0100, Ingo Schwarze wrote:

>> This is not perfect because the mandocdb process might get -KILLed
>> between the two renames, but i don't see a syscall anywhere to
>> atomically rename *two* files.

> Why do you have to?

There are two files to rename(2):  whatis.{db,index}.

> You can create the new file under temporary name in
> the same directory and rename it to the normal file name.

That's exactly what the patch i sent does:
Creating both files under temporary names in the same directory
and rename both to the normal file names when both are ready.

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Kristaps Dzonsons]

> Joerg Sonnenberger wrote on Sun, Jan 01, 2012 at 04:55:08PM +0100:
>> On Sun, Jan 01, 2012 at 04:45:38PM +0100, Ingo Schwarze wrote:
>
>>> This is not perfect because the mandocdb process might get -KILLed
>>> between the two renames, but i don't see a syscall anywhere to
>>> atomically rename *two* files.
>
>> Why do you have to?
>
> There are two files to rename(2):  whatis.{db,index}.
>
>> You can create the new file under temporary name in
>> the same directory and rename it to the normal file name.
>
> That's exactly what the patch i sent does:
> Creating both files under temporary names in the same directory
> and rename both to the normal file names when both are ready.

Hi Ingo,

I don't like this: no matter how we splice it, at some point the files 
will be inconsistent.  I also don't think it's unreasonable to have a 
"bogus" database for a few seconds when they're being rebuilt so long as 
this is documented.

However, if this is a problem scenario, we can just use flock(2) and be 
upfront that the database is inconsistent (apropos_db.c and mandocdb.c 
would need to sync open order) instead of crossing our fingers and 
hoping that they don't dbopen(3) between rename(2)s.

Thoughts?

Kristaps
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Joerg Sonnenberger]

On Sun, Jan 01, 2012 at 05:29:58PM +0100, Ingo Schwarze wrote:
> Hi Joerg,
> 
> Joerg Sonnenberger wrote on Sun, Jan 01, 2012 at 04:55:08PM +0100:
> > On Sun, Jan 01, 2012 at 04:45:38PM +0100, Ingo Schwarze wrote:
> 
> >> This is not perfect because the mandocdb process might get -KILLed
> >> between the two renames, but i don't see a syscall anywhere to
> >> atomically rename *two* files.
> 
> > Why do you have to?
> 
> There are two files to rename(2):  whatis.{db,index}.

Well, let me repeat the question, that is the point of having two
databases.

Joerg
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Kristaps Dzonsons]

On 01/03/12 13:45, Joerg Sonnenberger wrote:
> On Sun, Jan 01, 2012 at 05:29:58PM +0100, Ingo Schwarze wrote:
>> Hi Joerg,
>>
>> Joerg Sonnenberger wrote on Sun, Jan 01, 2012 at 04:55:08PM +0100:
>>> On Sun, Jan 01, 2012 at 04:45:38PM +0100, Ingo Schwarze wrote:
>>
>>>> This is not perfect because the mandocdb process might get -KILLed
>>>> between the two renames, but i don't see a syscall anywhere to
>>>> atomically rename *two* files.
>>
>>> Why do you have to?
>>
>> There are two files to rename(2):  whatis.{db,index}.
>
> Well, let me repeat the question, that is the point of having two
> databases.

Joerg---you mean to ask why I originally used two separate database files?
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Joerg Sonnenberger]

On Tue, Jan 03, 2012 at 02:07:48PM +0100, Kristaps Dzonsons wrote:
> On 01/03/12 13:45, Joerg Sonnenberger wrote:
> >On Sun, Jan 01, 2012 at 05:29:58PM +0100, Ingo Schwarze wrote:
> >>Hi Joerg,
> >>
> >>Joerg Sonnenberger wrote on Sun, Jan 01, 2012 at 04:55:08PM +0100:
> >>>On Sun, Jan 01, 2012 at 04:45:38PM +0100, Ingo Schwarze wrote:
> >>
> >>>>This is not perfect because the mandocdb process might get -KILLed
> >>>>between the two renames, but i don't see a syscall anywhere to
> >>>>atomically rename *two* files.
> >>
> >>>Why do you have to?
> >>
> >>There are two files to rename(2):  whatis.{db,index}.
> >
> >Well, let me repeat the question, that is the point of having two
> >databases.
> 
> Joerg---you mean to ask why I originally used two separate database files?

Yes.

Joerg
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Kristaps Dzonsons]

On 01/03/12 14:19, Joerg Sonnenberger wrote:
> On Tue, Jan 03, 2012 at 02:07:48PM +0100, Kristaps Dzonsons wrote:
>> On 01/03/12 13:45, Joerg Sonnenberger wrote:
>>> On Sun, Jan 01, 2012 at 05:29:58PM +0100, Ingo Schwarze wrote:
>>>> Hi Joerg,
>>>>
>>>> Joerg Sonnenberger wrote on Sun, Jan 01, 2012 at 04:55:08PM +0100:
>>>>> On Sun, Jan 01, 2012 at 04:45:38PM +0100, Ingo Schwarze wrote:
>>>>
>>>>>> This is not perfect because the mandocdb process might get -KILLed
>>>>>> between the two renames, but i don't see a syscall anywhere to
>>>>>> atomically rename *two* files.
>>>>
>>>>> Why do you have to?
>>>>
>>>> There are two files to rename(2):  whatis.{db,index}.
>>>
>>> Well, let me repeat the question, that is the point of having two
>>> databases.
>>
>> Joerg---you mean to ask why I originally used two separate database files?
>
> Yes.

Joerg,

Duplication.  By having the keyword database point to the index database 
entries, I avoided having duplicate index data everywhere---and this 
data can be huge since it includes titles, descriptions, and so on. 
Considering that the number of keywords is expected to grow as we add 
more values (full-text search?), this is quite important.  Since libdb 
has only one table per file...

It's still debatable whether using recno(3) for the index is a good 
idea.  The general usage is to hit the btree(3) then look up in the 
recno(3) (btree(3) is harmless, I think, but useless: we don't do 
lexicographic lookup as I'd originally thought).

However, look-up is slow in recno(3).  I can definately see a case for 
using btree(3) or even hash(3) in place of recno(3).

Make sense?

Kristaps
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Joerg Sonnenberger]

On Tue, Jan 03, 2012 at 02:45:04PM +0100, Kristaps Dzonsons wrote:
> Duplication.  By having the keyword database point to the index
> database entries, I avoided having duplicate index data
> everywhere---and this data can be huge since it includes titles,
> descriptions, and so on. Considering that the number of keywords is
> expected to grow as we add more values (full-text search?), this is
> quite important.  Since libdb has only one table per file...

You can easily have more than one key space in a db(3) file by prefixing
the key with a tag. That's e.g. what the services(5) database does.

Joerg
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Ingo Schwarze]

Hi Joerg,

Joerg Sonnenberger wrote on Tue, Jan 03, 2012 at 02:49:38PM +0100:
> On Tue, Jan 03, 2012 at 02:45:04PM +0100, Kristaps Dzonsons wrote:

> You can easily have more than one key space in a db(3) file by prefixing
> the key with a tag. That's e.g. what the services(5) database does.

I don't doubt that there would have been different options to choose
the layout of the database than those Kristaps has implemented, and in
the long run, checking what is worth improving might make sense.

But now is not the right time to start over, unless we want to miss
the 5.1 release.  What we have is working, and i'd say it performs
reasonably well.  I hope to enable it in -current one of the next
days.

The basic architecture should not be changed right now,
whereas now is a good time for non-intrusive polishing.

Thanks,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Ingo Schwarze]

Hi Kristaps,

> It's still debatable whether using recno(3) for the index is a good
> idea.  The general usage is to hit the btree(3) then look up in the
> recno(3)

Right, so typically, we do a full linear search through the
whole large btree(3), doing a substring or even regex match for
each entry, and after that typically a single or a few recno(3)
lookups in the much smaller index.

I'd *guess* almost the whole time is spent in the btree,
so i'm not sure optimizing the recno will buy us anything.
I'd discourage optimizing the recno part without first
confirming that any relevant part of the time is spent there.

In any case, please don't optimize the database format right now.  ;-)

> (btree(3) is harmless, I think, but useless: we don't do
> lexicographic lookup as I'd originally thought).

That statement makes sense to me.

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: half-atomically rebuild databases

[Ingo Schwarze]

Hi Kristaps,

Kristaps Dzonsons wrote on Tue, Jan 03, 2012 at 01:39:28PM +0100:
> Ingo Schwarze wrote:

>> Creating both files under temporary names in the same directory
>> and rename both to the normal file names when both are ready.

> I don't like this: no matter how we splice it, at some point the
> files will be inconsistent.

Agreed.  There is no way to atomically replace two existing files
with two new files, and combining both into a single file as suggested
by Joerg in not an option right now.

> I also don't think it's unreasonable to
> have a "bogus" database for a few seconds when they're being rebuilt
> so long as this is documented.

Well, with my patch, it's not a few seconds, but maybe a few
milliseconds, or even less.

On the other hand, without my patch, it is more than a few seconds.
On one of my old i386 servers (still in production use),
it is about 45 seconds.  On my sparc64, 4 minutes 40 seconds.
On the VAX or hp300, i have no idea, except that it will be *much*
worse.  During that time, apropos(1) is out of service.
And then, if the job crashes near the end, you are left without
any database, for good.

> However, if this is a problem scenario,

I don't think the rename-rename race is a problem scenario.
But i do think being without any database for several minutes,
or more likely many hours on the VAX, is a problem.

> we can just use flock(2) and
> be upfront that the database is inconsistent (apropos_db.c and
> mandocdb.c would need to sync open order) instead of crossing our
> fingers and hoping that they don't dbopen(3) between rename(2)s.

I don't object to that, but don't consider it an urgent matter
either.  It can easily be added later, on top of my patch.

> Thoughts?

I'm aware that my patch doesn't *completely* solve the issue,
but it solves it for almost all practical purposes.
So, what do you mean by "I don't like this":
Do you mean to say that you like the current situation better
than the one with the patch?
If so, why?

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv