Bug 241385 - databases/sqlite3: enable OS features
Summary: databases/sqlite3: enable OS features
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-ports-bugs (Nobody)
URL:
Keywords: buildisok
Depends on:
Blocks:
 
Reported: 2019-10-20 23:19 UTC by rozhuk.im
Modified: 2020-05-28 01:50 UTC (History)
5 users (show)

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


Attachments
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

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

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 \
+		-DSQLITE_DEFAULT_MMAP_SIZE=0x7fff0000 \
can be set to force use mmap() instead of pread()/pwrite() but is not safe.
https://www.sqlite.org/mmap.html
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)
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.
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
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
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:
-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.
Comment 11 rozhuk.im 2020-04-16 07:09:28 UTC
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.
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
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)
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.