Bug 241385
| Summary: | databases/sqlite3: enable OS features | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Ivan Rozhuk <rozhuk.im> | ||||||||
| Component: | Individual Port(s) | Assignee: | Kevin Bowling <kbowling> | ||||||||
| Status: | Closed FIXED | ||||||||||
| Severity: | Affects Many People | CC: | emaste, kbowling, mandree, pavelivolkov, rozhuk.im, tmunro, trasz | ||||||||
| Priority: | --- | Keywords: | buildisok | ||||||||
| Version: | Latest | Flags: | bugzilla:
maintainer-feedback?
(pavelivolkov) |
||||||||
| Hardware: | Any | ||||||||||
| OS: | Any | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Ivan Rozhuk
2019-10-20 23:19:35 UTC
Doesn't the configure pick these up, or is this a different build than standard autoconf things? autoconf does not detect these features, but CPPFLAGS passed to compiler and enable features. Are all flags appropriate for all versions/archs of FreeBSD? If not, they should be scoped accordingly IMHO there is no arch depend flags. Most of flags - enables syscals, not 100% sure but it should be available on 11.2+. I use it on 12.1 amd64. Build info is available at https://gitlab.com/swills/freebsd-ports/pipelines/93636806 (In reply to rozhuk.im from comment #0) Hello. You're right. These features (HAVE_PREAD, HAVE_PWRITE) are not detected during configuration. But your patch requires further study. For example, the flag SQLITE_MMAP_READWRITE is dangerous for many uses cases. Please read paragraph 4 in the "How Memory-Mapped I/O Works" at the link (https://www.sqlite.org/mmap.html). Many other options auto detected in the configure script. (In reply to Pavel Volkov from comment #6) SQLITE_MMAP_READWRITE - does not enables mmap() usage, it enables memcpy() to mmaped memory write instead of lseek()+write() / pwrite(). I agree that it can be dangerous, or at least I do not know it there any bugs/features in FreeBSD mmap() implementations. But mmap() io does not enabled by this patch. To enable mmaped io some one should add -DSQLITE_DEFAULT_MMAP_SIZE=0x7fff0000 before compile this port, to force all apps use mmap() or execute "PRAGMA mmap_size=0x7fff0000" in app. I do not insist on SQLITE_MMAP_READWRITE, but if our mmap() implementation works as sqlite3 authors expect, then we get more profit from this. Yes, ~half of options is detected by autoconf, but I prefer to define it, just to be sure that it set. Hello. You can set any compilation options before creating the package. sh/bash: CPPFLAGS='-DHAVE_PREAD=1 -DHAVE_PWRITE=1' make clean configure csh/tcsh: setenv CPPFLAGS '-DHAVE_PREAD=1 -DHAVE_PWRITE=1' && make clean configure Pavel, the proposal is to add the safe features to the port, for everyone's profit. Is there an automated halfway thorough test suite we can enable so as to trigger pkg-fallout@ mail on the Tier 2/3 architectures? Already in Makefile: -DHAVE_ISNAN=1 \ -DHAVE_MALLOC_USABLE_SIZE=1 \ -DHAVE_GMTIME_R=1 \ -DHAVE_LOCALTIME_R=1 \ -DHAVE_USLEEP=1 \ -DHAVE_STRCHRNUL=1 \ New to add: -DHAVE_DECL_STRERROR_R=1 \ Looks like it was removed from sqlite since last time I dig into it or used some where outside amalgama, removed from this patch. -DHAVE_FCHOWN=1 \ https://www.freebsd.org/cgi/man.cgi?query=fchown&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html The fchown() system call appeared in 4.2BSD. Defined for non VxWorks. -DHAVE_FDATASYNC=1 \ https://www.freebsd.org/cgi/man.cgi?query=fdatasync&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html The fdatasync() system call appeared in FreeBSD 11.1. -DHAVE_GETHOSTUUID=1 \ gethostuuid() - apple specific, removed from this patch. -DHAVE_LOCALTIME_S=1 \ localtime_s() - not used if HAVE_LOCALTIME_R looks like windows specific, removed from this patch. -DHAVE_LSTAT=1 \ https://www.freebsd.org/cgi/man.cgi?query=lstat&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html The lstat() system call appeared in 4.2BSD. Defined for non VxWorks. -DHAVE_POSIX_FALLOCATE=1 \ https://www.freebsd.org/cgi/man.cgi?query=posix_fallocate&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html The posix_fallocate() function appeared in FreeBSD 9.0. -DHAVE_PREAD=1 \ https://www.freebsd.org/cgi/man.cgi?query=pread&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html The pread() function appeared in AT&T System V Release 4 UNIX. -DHAVE_PWRITE=1 \ https://www.freebsd.org/cgi/man.cgi?query=pwrite&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html The pwrite() function appeared in AT&T System V Release 4 UNIX. -DHAVE_STRERROR_R=1 \ https://www.freebsd.org/cgi/man.cgi?query=strerror_r&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html strerror_r() function was implemented in FreeBSD 4.4 -DHAVE_READLINK=1 \ https://www.freebsd.org/cgi/man.cgi?query=readlink&apropos=0&sektion=2&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html The readlink() system call appeared in 4.2BSD. Defined for non VxWorks. -DSQLITE_MMAP_READWRITE=1 \ Compile time option, if enabled the mmap()+memcpy() used instead of read()/pread() write()/pwrite(). After this enabled, application must enable it: PRAGMA mmap_size=268435456; https://www.sqlite.org/mmap.html https://bugs.chromium.org/p/chromium/issues/detail?id=489784 https://www.phoronix.com/scan.php?page=news_item&px=MTM3NjQ -DSQLITE_OS_UNIX=1 ** After the following block of preprocess macros, all of SQLITE_OS_UNIX, ** SQLITE_OS_WIN, and SQLITE_OS_OTHER will defined to either 1 or 0. One of ** the three will be 1. The other two will be 0. We are not WIN and not OTHER anyway. I use this on desktops without any issues last half year. Minimum req: FreeBSD 11.1. Looks like mmap() work without issues on FreeBSD, and app must call "PRAGMA mmap_size=268435456" before use this feature, so SQLITE_MMAP_READWRITE looks safe to use. I use it in codelite IDE and this works. Created attachment 213454 [details]
patch
Also it will be nice to have this options in all ports that uses their own sqlite3 inside, like ports-mgmt/pkg* and other.
"The operating system must have a unified buffer cache in order for the memory-mapped I/O extension to work correctly, especially in situations where two processes are accessing the same database file and one process is using memory-mapped I/O while the other is not. Not all operating systems have a unified buffer cache. In some operating systems that claim to have a unified buffer cache, the implementation is buggy and can lead to corrupt databases." Note that FreeBSD with ZFS doesn't have a unified buffer cache (mmap'd files are duplicated in the ARC and regular kernel buffers). I don't know whether it's safe or not or how the performance compares to pwrite/pread, for sqlite3. Also, I noticed that other distributions like Debian are not enabling the mmap option, and are going with pread/pwrite despite their unified buffer cache. SQLITE_MMAP_READWRITE=1 This allow applications use this feature, application must exec: "PRAGMA mmap_size=268435456" to use this feature. There is another option: SQLITE_CONFIG_MMAP_SIZE (NOT in this patch) that force all application use mmap for read/write in sqlite3. I know only one app that uses mmap_size - CodeLite. (our pkg - does not, @bapt miss few options, IMHO) Ah, right, I see. Cool. Doesn't this patch give you some duplicates, some coming from configure and some from CPPFLAGS? I see -DHAVE_FDATASYNC=1 twice. (In reply to Thomas Munro from comment #14) Yes, there is some options duplicate things in autoconf, but on configure they show as cached, this reduces a bit configure time and it is more clean to see all options in one place. FWIW, according to bapt@, sqlite3 bundled with pkg already uses those options. (In reply to Edward Tomasz Napierala from comment #16) Does that mean we should just apply those on the assumption that they've seen enough testing in the form of pkg? (In reply to Edward Tomasz Napierala from comment #16) @bapt miss some flags in his commit. (In reply to Matthias Andree from comment #17) Yes, I believe so - those flags seem safe generally speaking, FreeBSD has supported those features for ages, and to me, their use for pkg(8) means they should be safe to flip. As for the flags the libsqlite3 copy bundled with pkg doesn't enable yet - perhaps we should commit those step by step, ie first turn on those used by pkg, wait for a couple of weeks to see if there's no fallout, then enable the missing ones in pkg build, and then also here? -DHAVE_POSIX_FALLOCATE=1 results in a test failure (at least on ZFS): ! chunksize-1.2 expected: [32768] ! chunksize-1.2 got: [2048] Time: chunksize.test 15 ms A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=f7e91e1e0fe3860f53a83a180b1643fa45ea2c33 commit f7e91e1e0fe3860f53a83a180b1643fa45ea2c33 Author: Rozhuk Ivan <rozhuk.im@gmail.com> AuthorDate: 2021-06-13 21:42:36 +0000 Commit: Kevin Bowling <kbowling@FreeBSD.org> CommitDate: 2021-06-13 23:17:44 +0000 databases/sqlite3: enable OS features Enable various FreeBSD API options and go-fasters The only one of these that raised concern is related to mmap, but this compile time flag will not enable mmap usage on its own. There is a test failure on ZFS with posix_fallocate(), leave that off for now. PR: 241385 Tested by: kbowling (test harness https://www.sqlite.org/testing.html) Approved by: maintainer timeout databases/sqlite3/Makefile | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) Thanks for your contribution! posix_fallocate() will need some kernel work in OpenZFS, so I left it out with a comment. Created attachment 225785 [details]
Test output from f7e91e1e0fe3860f53a83a180b1643fa45ea2c33
Add test output after f7e91e1e0fe3860f53a83a180b1643fa45ea2c33
After more consideration I disabled SQLITE_MMAP_READWRITE in a followup commit, that should at least be behind a port option or something as some users/devs may want to only mmap reads (to reduce sqlite pagecache size) but use the well tested (p)write path. There are more things that can go wrong in the mmap write path, and not everyone is ready to handle that. https://cgit.freebsd.org/ports/commit/?id=48df5fd0d9e307f159634df6d72dadc57568a563 (In reply to Kevin Bowling from comment #24) https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241385#c7 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241385#c13 (In reply to rozhuk.im from comment #25) Yes I carefully read and considered your comments before commiting the work and when making the followup change. The issue here is, mmap writes require catching a signal to properly handle errors. It's unlikely most application code does that correctly. The outsized benefit to mmap is on the read path by eliminating the need for much sqlite page cache. That usage should be pretty safe. Also ZFS is widely used on FreeBSD and still pretty naive at a lot of basic things, so I'd rather play it safe in case mmap application usage increases at some point in the future. I think it would be fair to submit another PR to add a few mmap-related options or a flavor, and allow people to experiment in their local environment, if you want to pursue this further. |
