Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly configure SQLite on Linux and OS X #287

Closed
wants to merge 1 commit into from
Closed

Properly configure SQLite on Linux and OS X #287

wants to merge 1 commit into from

Conversation

jmmv
Copy link

@jmmv jmmv commented Mar 14, 2016

Add all necessary -DHAVE_* flags that result from a run of the configure
script on Linux and OS X so that we build SQLite properly. In particular,
this fixes mattn/go-sqlite3 #285 in which I spotted erratic backoff behavior
due to the lack of usleep support.

Add all necessary -DHAVE_* flags that result from a run of the configure
script on Linux and OS X so that we build SQLite properly.  In particular,
this fixes mattn/go-sqlite3 #285 in which I spotted erratic backoff behavior
due to the lack of usleep support.
@vovkasm
Copy link
Contributor

vovkasm commented Mar 14, 2016

@jmmv, are you sure that sqlite amalgamation required so many defines?
I looked for HAVE_DLFCN_H or HAVE_DECL_STRERROR_R in amalgamation sources and found nothing. Sure that most of them (except HAVE_USLEEP and couple others, about gmtime and localtime) not needed.

But HAVE_USLEEP required to be defined and set to true value, yes. And it explicitly specified in the documentation https://www.sqlite.org/howtocompile.html and https://www.sqlite.org/compile.html

@mattn
Copy link
Owner

mattn commented Mar 15, 2016

Did you check some of breaking compatibility, append of dependency, performance issue or etc?

@jmmv
Copy link
Author

jmmv commented Mar 15, 2016

@vovkasm Many of the defines should not be necessary, particularly those for headers that can be found "everywhere", but they come from standard configure runs. I thought keeping all of them is "safer": it's trivial to copy/paste the output of the script and add the flags (and they don't hurt), but it's tricky to manually curate them (because then you can make mistakes). But I like your point on the documentation, so if the docs say to just define HAVE_USLEEP, maybe we should just do that.

And now that you mention this... it's possible that some of the defines exist only for extra tools shipped with sqlite3 that are not part of the amalgamation distribution.

@mattn All of the defines added are for features that should be widely available on any major Linux distribution; they are nothing really special. Is there any specific platform you would like to validate? This will be much easier to reason about if we do as @vovkasm says by only define a minimal set of things.

What do you mean with "append of dependency"?

Regarding performance: well, the behavior of these bindings without the fix was really bad under contention and now they fly. I'm running this binding as the backend for a FUSE file system that is receiving significant load from various cores and they behave great now. (Yeah, yeah, I know, I don't have numbers to back these claims, but if you have something specific in mind, I could maybe check too.)

@jmmv
Copy link
Author

jmmv commented Mar 15, 2016

So:

$ grep -Eo HAVE_[A-Z0-9_]+ code/*.c | sort | uniq
HAVE_DOCID_GE
HAVE_DOCID_LE
HAVE_FDATASYNC
HAVE_FULLFSYNC
HAVE_GETHOSTUUID
HAVE_GMTIME_R
HAVE_INT16_T
HAVE_INT8_T
HAVE_INTTYPES_H
HAVE_ISNAN
HAVE_LANGID
HAVE_LOCALTIME_R
HAVE_LOCALTIME_S
HAVE_MALLOC_H
HAVE_MALLOC_USABLE_SIZE
HAVE_MINGW_H
HAVE_MREMAP
HAVE_OS_TRACE
HAVE_POSIX_FALLOCATE
HAVE_SQLITE_CONFIG_H
HAVE_STDINT_H
HAVE_STRCHRNUL
HAVE_STRERROR_R
HAVE_UINT16_T
HAVE_UINT32_T
HAVE_UINT8_T
HAVE_USLEEP
HAVE_UTIME
HAVE__MINGW_H

The code is checking for many more constants than just HAVE_USLEEP. In particular, I'm concerned about the _R ones: you'd really want SQLite to use the reentrant versions of the libc functions if present.

Based on this, I still think that just using the defines generated by the configure script is better than attempting to curate them by hand; less room for mistakes.

Or am I missing something?

@vovkasm
Copy link
Contributor

vovkasm commented Mar 15, 2016

Based on this, I still think that just using the defines generated by the configure script is better than attempting to curate them by hand; less room for mistakes.

I'am not sure about best solution for this exactly case. So it all for @mattn to judge ;-)
Are these defines propagates to other cgo code? Are cgo generated code depends on those constants? I don't know. If answers to above questions is No and No, then I fully support idea to use defines from configure script.

So I will only provide additional information that may help with solution.

  1. FTS3_HAVE_DOCID_GE, FTS3_HAVE_DOCKID_LE, FTS3_HAVE_LANGID defined in sqlite3.c itself. (No such constants as HAVE_DOCID_GE etc.)
  2. HAVE_FULLFSYNC defined in sqlite3.c itself (autodetected).
  3. HAVE_FDATASYNC described in Docs
  4. HAVE_GETHOSTUUID defined in sqlite3.c itself (autodetected)
  5. HAVE_GMTIME_R yes, used. But needed only if SQLITE_OMIT_DATETIME_FUNCS, not our case. Described in Docs
  6. HAVE_STDINT_H, HAVE_INTTYPES_H (with all HAVE_[U]INT{8,16,32}_T). Yes used, see below.
  7. HAVE_ISNAN yes, used. Described in Docs
  8. HAVE_LOCALTIME_R, HAVE_LOCALTIME_S. yes used, described in Docs, most probably should be used, see below.
  9. HAVE_MALLOC_H applicable only if HAVE_MALLOC_USABLE_SIZE used.
  10. _HAVE_MINGW_H and _HAVE__MINGW_H only for mingw.
  11. HAVE_MREMAP tunable, but autodetected in code.
  12. SQLITE_HAVE_OS_TRACE (no HAVE_OS_TRACE) autodetected in code.
  13. HAVE_POSIX_FALLOCATE used, not documented, but has backoff implementation.
  14. HAVE_SQLITE_CONFIG_H documented in Docs
  15. HAVE_STRCHRNUL used, documented in Docs, not sure about profit to use it... Used in one place in implementation of sprintf like function to find next '%' char.
  16. HAVE_STRERROR_R used, not documented, but used only in logging errors.
  17. HAVE_USLEEP used, documented.

From above I can say, that

  1. HAVE_USLEEP need for us, but it will not resolve multiaccess errors from many threads, only make timeouts cheaper. I was trying some time ago to enable this option, but tests still fails. To really resolve multithreaded access to database with shared cache, someone probably need to deal with sqlite3_unlock_notify.
  2. HAVE_LOCALTIME_R good to have, but it only used in 'localtime' modifier in 'date' function
  3. HAVE_STDINT_H and other type size definitions... It would be good to use. In additional, sized types can be defined without stdint.h types, as -DUINT32_TYPE=, -DUINT16_TYPE= etc...
    But, sqlite defaults for desktop platforms tat Go supports is correct. For arm, I not so sure. But in my own project (compiled for 'arm64','arm7' for iOS, 'i386' and 'x86_64' for MacOS X) sqlite3 built with only -DSQLITE_ENABLE_FTS3 -DSQLITE_ENABLE_FTS3_PARENTHESIS and works well from single thread.

@gjrtimmer
Copy link
Collaborator

Most options already implemented, case outdated.

@mattn Please close.

@gjrtimmer gjrtimmer closed this May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants