Bug 241385 - databases/sqlite3: enable OS features
Summary: databases/sqlite3: enable OS features
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Kevin Bowling
Keywords: buildisok
Depends on:
Reported: 2019-10-20 23:19 UTC by rozhuk.im
Modified: 2021-06-14 15:55 UTC (History)
7 users (show)

See Also:
bugzilla: maintainer-feedback? (pavelivolkov)

patch (956 bytes, patch)
2019-10-20 23:19 UTC, rozhuk.im
no flags Details | Diff
patch (874 bytes, patch)
2020-04-16 07:09 UTC, rozhuk.im
rozhuk.im: maintainer-approval?
Details | Diff
Test output from f7e91e1e0fe3860f53a83a180b1643fa45ea2c33 (27.49 KB, text/plain)
2021-06-13 23:21 UTC, Kevin Bowling
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description rozhuk.im 2019-10-20 23:19:35 UTC
Created attachment 208474 [details]

Make script from sqlite3 a bit outdated and does not detect all OS features that can be used by sqlite3.
For example it does not detect pread()/pwrite() and sqlite3 use lseek()+read() instead of pread().
I walk trough all code and collect all HAVE_* switches that available on FreeBSD.

PS: also
+		-DSQLITE_MAX_MMAP_SIZE=0x7fff0000 \
can be set to force use mmap() instead of pread()/pwrite() but is not safe.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-21 06:18:44 UTC
Doesn't the configure pick these up, or is this a different build than standard autoconf things?
Comment 2 rozhuk.im 2019-10-21 06:40:40 UTC
autoconf does not detect these features, but CPPFLAGS passed to compiler and enable features.
Comment 3 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-21 08:11:43 UTC
Are all flags appropriate for all versions/archs of FreeBSD? If not, they should be scoped accordingly
Comment 4 rozhuk.im 2019-10-21 10:01:48 UTC
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.
Comment 5 Automation User 2019-11-05 00:06:16 UTC
Build info is available at https://gitlab.com/swills/freebsd-ports/pipelines/93636806
Comment 6 Pavel Volkov 2019-11-05 02:51:39 UTC
(In reply to rozhuk.im from comment #0)
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.
Comment 7 rozhuk.im 2019-11-05 03:07:35 UTC
(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.
Comment 8 Pavel Volkov 2019-11-17 12:10:42 UTC
You can set any compilation options before creating the package.

CPPFLAGS='-DHAVE_PREAD=1 -DHAVE_PWRITE=1' make clean configure

setenv CPPFLAGS '-DHAVE_PREAD=1 -DHAVE_PWRITE=1' && make clean configure
Comment 9 Matthias Andree freebsd_committer 2020-02-22 15:19:13 UTC
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?
Comment 10 rozhuk.im 2020-04-16 07:05:47 UTC
Already in Makefile:

New to add:

Looks like it was removed from sqlite since last time I dig into it or used some where outside amalgama, removed from this patch.

The fchown() system call appeared in 4.2BSD.
Defined for non VxWorks.

The fdatasync() system call appeared in FreeBSD 11.1.

gethostuuid() - apple specific, removed from this patch.

localtime_s() - not used if HAVE_LOCALTIME_R
looks like windows specific, removed from this patch.

The lstat() system call appeared in 4.2BSD.
Defined for non VxWorks.

The posix_fallocate() function appeared in FreeBSD 9.0.

The pread() function appeared in AT&T System V Release 4 UNIX.

The pwrite() function appeared in AT&T System V Release 4 UNIX.

strerror_r() function was implemented in FreeBSD 4.4

The readlink() system call appeared in 4.2BSD.
Defined for non VxWorks.

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;


** 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.
Comment 11 rozhuk.im 2020-04-16 07:09:28 UTC
Created attachment 213454 [details]

Also it will be nice to have this options in all ports that uses their own sqlite3 inside, like ports-mgmt/pkg* and other.
Comment 12 Thomas Munro freebsd_committer 2020-05-27 21:26:00 UTC
"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.
Comment 13 rozhuk.im 2020-05-27 22:23:00 UTC
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)
Comment 14 Thomas Munro freebsd_committer 2020-05-28 00:25:28 UTC
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.
Comment 15 rozhuk.im 2020-05-28 01:50:01 UTC
(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.
Comment 16 Edward Tomasz Napierala freebsd_committer 2020-12-11 12:47:48 UTC
FWIW, according to bapt@, sqlite3 bundled with pkg already uses those options.
Comment 17 Matthias Andree freebsd_committer 2020-12-11 13:33:26 UTC
(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?
Comment 18 rozhuk.im 2020-12-11 18:49:02 UTC
(In reply to Edward Tomasz Napierala from comment #16)

@bapt miss some flags in his commit.
Comment 19 Edward Tomasz Napierala freebsd_committer 2020-12-15 12:35:05 UTC
(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?
Comment 20 Kevin Bowling freebsd_committer 2021-06-13 23:02:58 UTC
-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
Comment 21 commit-hook freebsd_committer 2021-06-13 23:19:03 UTC
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(-)
Comment 22 Kevin Bowling freebsd_committer 2021-06-13 23:20:19 UTC
Thanks for your contribution!

posix_fallocate() will need some kernel work in OpenZFS, so I left it out with a comment.
Comment 23 Kevin Bowling freebsd_committer 2021-06-13 23:21:10 UTC
Created attachment 225785 [details]
Test output from f7e91e1e0fe3860f53a83a180b1643fa45ea2c33

Add test output after f7e91e1e0fe3860f53a83a180b1643fa45ea2c33
Comment 24 Kevin Bowling freebsd_committer 2021-06-14 07:13:43 UTC
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.

Comment 26 Kevin Bowling freebsd_committer 2021-06-14 15:55:39 UTC
(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.