Fix #78808: [LMDB] MDB_MAP_FULL: Environment mapsize limit reached

[cmb69]

We implement support for a fifth parameter, which allows to specify the
mapsize. The parameter defaults to zero, in which case the compiled in
default mapsize (usually 1048576) will be used. The mapsize should be
a multiple of the page size of the OS.

---

I'm not really happy with this solution. I would have preferred to introduce an option array, but all driver specific parameters are converted to string (why?), so that's not possible, even though it might bite us when we may want to add support for further mdb_env_set_*() functions.

Furthermore, I'm not sure which version to target. I think not being able to specify the mapsize is a bug, because the default mapsize is pretty small, but am uncomfortable to fix it this late for PHP 7.2, so I've targeted 7.3.

[saippuakauppias]

mapsize may be equal 0

[cmb69]

mapsize==0 is supposed to be the default (which also may be passed explicitly), in which case no warning should be emitted. Do you suggest to change the default to -1, and let the user pass 0 to have a mapsize of zero?

[saippuakauppias]

No, I thought it was a mistake :)

[saippuakauppias]

mapsize greater 0

[cmb69]

Yes, that's deliberate. If mapsize==0 that means use the default mapsize, so there is no need to call mdb_env_set_mapsize().

[saippuakauppias]

Need add for lmdb:

@unlink($db_filename.'-lock');

[cmb69]

That should be handled by the --CLEAN-- section.

[saippuakauppias]

For the future or discussion: in multi-thread read database I was caught an exception: "MDB_READERS_FULL: Environment maxreaders limit reached".

By default MDB_READERS_FULL=126, but it can be changed through environment export MDB_READERS_FULL=1024.

I think, in future maybe cool to change mdb_env_set_maxreaders() in dba_open() args.

PS: I wanted to send a bug about it, but could not make a test script.

[cmb69]

@saippuakauppias, yes, I've seen that change in your lmdb patch, but I'm really unsure how to go about that. Having more and more optional parameters doesn't look desireable, but we can't pass an option array due to the mentioned string conversion. It might be sensible to change this in PHP 8, but we would need to assess the impact an other drivers.

[cmb69]

[…], but we would need to assess the impact an other drivers.

The optional driver parameters are now documented. TL;DR: some drivers don't support any params, and the others all support a single $filemode param. So switching to an option array doesn't seem to be an issue (actually, the fact that $filemode is passed as string might be confusing, since $filemode is usually expressed as octal number, which would loose its expected meaning when given as string).

Anyhow, since this PR would introduce an optional parameter in a patch revision, I'd like to get some feedback of others.

Thanks.

[saippuakauppias]

Yes, it does not look elegant ...
It might be worth rewriting method dba_open and doing something like:

dba_open ( string $path , string $mode [, string $handler [, string $filemode [, array $params]]] ) : resource

?

I understand that this requires additional work, but maybe it’s better to do well right away?

LMDB is a really fast and cool database! I am using it in production now and it seems that this is the best solution for key=>value that I have met!

I also want to test fork libmdbx - it seems that it should be even better and more stable. But I don’t understand yet - will it work with PHP if I just replace library files or do I need to write my own extension?

[cmb69]

I'm afraid we can't change the signature of dba_open() during PHP 7; even if none of the bundled drivers use further arguments, custom code might expect such to be passed as strings.

To use another database with DBA, you would have to implement the driver (see, for instance, https://github.com/php/php-src/blob/php-7.4.1/ext/dba/dba_lmdb.c). Probably no need to fork the actual implementation library.

Anyhow, if there are no objections, I'll merge this in a week.

[cmb69]

Applied as c05a069. Thanks.