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
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
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
> 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
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
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
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
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
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
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
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
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