Bug 232490 - databases/postgresql11-server: add LLVM JIT
Summary: databases/postgresql11-server: add LLVM JIT
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: pgsql
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-20 22:47 UTC by Greg V
Modified: 2019-02-15 20:21 UTC (History)
6 users (show)

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


Attachments
postgresql11-llvm.patch (37.73 KB, patch)
2018-10-20 22:47 UTC, Greg V
no flags Details | Diff
Alternative: LLVMJIT option on postgresql11-server (45.11 KB, patch)
2018-10-21 19:18 UTC, Christoph Moench-Tegeder
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg V 2018-10-20 22:47:02 UTC
Created attachment 198420 [details]
postgresql11-llvm.patch

The LLVM JIT can be built as a separate component port (see patch).

But it should be built with the same options as the server — you can see that the bitcode file for the OpenSSL is commented out in the plist because it wasn't built.

What do you think?
Comment 1 Christoph Moench-Tegeder freebsd_committer 2018-10-21 19:18:48 UTC
Why not make this an OPTION? This results in less problems with flag mismatch or mismatching packages.
In fact, I've an patch for that...
Comment 2 Christoph Moench-Tegeder freebsd_committer 2018-10-21 19:18:56 UTC
Created attachment 198443 [details]
Alternative: LLVMJIT option on postgresql11-server

Let's have a LLVMJIT option on postgresql11-server to enable building the LLVM_JIT-bytecode and support module. Keep this option OFF by default - it nearly doubles package size and pulls in a heavy build dependency (devel/llvm60).
Comment 3 Greg V 2018-10-22 09:35:19 UTC
(In reply to Christoph Moench-Tegeder from comment #2)

hmm! Can we make flavors? postgresql11-server@jit and @nojit

(I think it's important to make the JIT pkg-installable instead of forcing everyone to build from source.)
Comment 4 Palle Girgensohn freebsd_committer 2018-10-22 09:36:47 UTC
Yes, my plan is to add them as flavors. Just haven't had the time. Patches are welcome. :)
Comment 5 cedric 2018-10-23 13:24:53 UTC
Why not make the JIT simply an option that is ON by default?
If someone is bothered by the huge build dependency, then they can simply turn it off.
Long term, I assumes most people will want it.
Comment 6 Torsten Zuehlsdorff freebsd_committer 2019-01-15 12:12:15 UTC
Is there any progress here? How can i help to get this issue resolved?
Comment 7 Palle Girgensohn freebsd_committer 2019-01-15 13:03:48 UTC
Sorry for the delay, I'll look into this in the next few days.

Palle
Comment 8 Torsten Zuehlsdorff freebsd_committer 2019-02-06 11:43:09 UTC
How can we help you Palle? Did you already decide if you want to go with an option (including a flavor or not)?

If you decide, we can provide a patch you only need to test. 

If you are completely out of time, i can test and commit, if you want.
Comment 9 Palle Girgensohn freebsd_committer 2019-02-06 12:12:27 UTC
Thanks for the reminder about this one.

I could use some help testing it. I'll try adding a flavor, and will post a patch.

Palle
Comment 10 Palle Girgensohn freebsd_committer 2019-02-06 13:18:34 UTC
How about this one?

https://reviews.freebsd.org/differential/diff/53622/

Palle
Comment 11 Torsten Zuehlsdorff freebsd_committer 2019-02-08 11:21:22 UTC
Mh, i cant download the patch. Can you add me as tester or provide the patch here?

At first glance it looks good to me! :)
Comment 12 Palle Girgensohn freebsd_committer 2019-02-08 12:14:04 UTC
(In reply to Torsten Zuehlsdorff from comment #11)
added you as reviewer
Comment 13 commit-hook freebsd_committer 2019-02-15 11:03:00 UTC
A commit references this bug:

Author: girgen
Date: Fri Feb 15 11:02:24 UTC 2019
New revision: 492989
URL: https://svnweb.freebsd.org/changeset/ports/492989

Log:
  The PostgreSQL Global Development Group has released an update to all
  supported versions of our database system, including 11.2, 10.7, 9.6.12,
  9.5.16, and 9.4.21. This release changes the behavior in how PostgreSQL
  interfaces with `fsync()` and includes fixes for partitioning and over
  70 other bugs that were reported over the past three months.

  Users should plan to apply this update at the next scheduled downtime.

  FreeBSD port adds OPTIONS knob to support LLVM JIT. [1]

  Highlight: Change in behavior with fsync()
  ------------------------------------------

  When available in an operating system and enabled in the configuration
  file (which it is by default), PostgreSQL uses the kernel function
  `fsync()` to help ensure that data is written to a disk. In some
  operating systems that provide `fsync()`, when the kernel is unable to
  write out the data, it returns a failure and flushes the data that was
  supposed to be written from its data buffers.

  This flushing operation has an unfortunate side-effect for PostgreSQL:
  if PostgreSQL tries again to write the data to disk by again calling
  `fsync()`, `fsync()` will report back that it succeeded, but the data
  that PostgreSQL believed to be saved to the disk would not actually be
  written. This presents a possible data corruption scenario.

  This update modifies how PostgreSQL handles a `fsync()` failure:
  PostgreSQL will no longer retry calling `fsync()` but instead will
  panic. In this case, PostgreSQL can then replay the data from the
  write-ahead log (WAL) to help ensure the data is written. While this may
  appear to be a suboptimal solution, there are presently few alternatives
  and, based on reports, the problem case occurs extremely rarely.

  A new server parameter `data_sync_retry` has been added to manage this
  behavior. If you are certain that your kernel does not discard dirty
  data buffers in such scenarios, you can set `data_sync_retry` to `on` to
  restore the old behavior.

  Release Notes:	https://www.postgresql.org/about/news/1920/
  PR:		232490 [1]

Changes:
  head/databases/postgresql10-client/Makefile
  head/databases/postgresql10-contrib/Makefile
  head/databases/postgresql10-pgtcl/Makefile
  head/databases/postgresql10-server/Makefile
  head/databases/postgresql10-server/distinfo
  head/databases/postgresql10-server/pkg-plist-client
  head/databases/postgresql10-server/pkg-plist-server
  head/databases/postgresql11-server/Makefile
  head/databases/postgresql11-server/distinfo
  head/databases/postgresql11-server/pkg-plist-client
  head/databases/postgresql11-server/pkg-plist-server
  head/databases/postgresql94-client/Makefile
  head/databases/postgresql94-contrib/Makefile
  head/databases/postgresql94-pgtcl/Makefile
  head/databases/postgresql94-server/Makefile
  head/databases/postgresql94-server/distinfo
  head/databases/postgresql94-server/pkg-plist-server
  head/databases/postgresql95-client/Makefile
  head/databases/postgresql95-contrib/Makefile
  head/databases/postgresql95-pgtcl/Makefile
  head/databases/postgresql95-plperl/Makefile
  head/databases/postgresql95-plpython/Makefile
  head/databases/postgresql95-server/Makefile
  head/databases/postgresql95-server/distinfo
  head/databases/postgresql95-server/pkg-plist-server
  head/databases/postgresql96-client/Makefile
  head/databases/postgresql96-contrib/Makefile
  head/databases/postgresql96-docs/Makefile
  head/databases/postgresql96-pgtcl/Makefile
  head/databases/postgresql96-pltcl/Makefile
  head/databases/postgresql96-server/Makefile
  head/databases/postgresql96-server/distinfo
  head/databases/postgresql96-server/pkg-plist-client
  head/databases/postgresql96-server/pkg-plist-server
Comment 14 Torsten Zuehlsdorff freebsd_committer 2019-02-15 11:06:58 UTC
Thanks for the option! :)
Comment 15 Palle Girgensohn freebsd_committer 2019-02-15 11:50:13 UTC
Committed the option.

I'm also having a dialogue with portsmgr@ about adding a flavour for llvm.
Comment 16 Torsten Zuehlsdorff freebsd_committer 2019-02-15 11:54:20 UTC
That is great.

Do you really should MFH this, since it contains important bugfixes!

Greetings,
Torsten
Comment 17 Palle Girgensohn freebsd_committer 2019-02-15 12:10:45 UTC
(In reply to Torsten Zuehlsdorff from comment #16)

Reasonable, will do so in a day or two, if no problems arise.
Comment 18 Charlie Li 2019-02-15 20:21:41 UTC
Shouldn't the LLVM option be using ${LLVM_DEFAULT} from bsd.default-versions.mk rather than hardcoding llvm60?