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

postgresql: Fix use with extensions #17838

Merged
merged 1 commit into from
Sep 2, 2016
Merged

postgresql: Fix use with extensions #17838

merged 1 commit into from
Sep 2, 2016

Conversation

lsix
Copy link
Member

@lsix lsix commented Aug 19, 2016

Motivation for this change

Postgresql extension mechanism is broken in master as reported in #15512 and #16032

Things done
  • Tested using sandboxing
    (nix.useChroot on NixOS,
    or option build-use-chroot in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Fixes #15512 and #16032

@lsix lsix added the 9.needs: reporter feedback This issue needs the person who filed it to respond label Aug 19, 2016
@lsix lsix added this to the 16.09 milestone Aug 19, 2016
@mention-bot
Copy link

@lancelotsix, thanks for your PR! By analyzing the annotation information on this pull request, we identified @MarcWeber, @edolstra and @aszlig to be potential reviewers

@lsix lsix force-pushed the fix_postgis branch 2 times, most recently from 55f2db1 to 67be11b Compare August 29, 2016 21:38
@lsix
Copy link
Member Author

lsix commented Aug 29, 2016

Improved, rebased against master and repushed.

It appears that the other approach to fix #15512 and #16032 would be remove the multi-output of postgresql. This would avoid the env variable used to indicate postgresql where to find libs.

@lsix
Copy link
Member Author

lsix commented Aug 30, 2016

@vcunat could you have a look at that PR ? The problems mentioned above come from the moment where a `"lib" output have been introduced. postgres cannot find libdir and pkglibdir relative to its position.

This search is done within this function:

/*
 * make_relative_path - make a path relative to the actual binary location
 *
 * This function exists to support relocation of installation trees.
 *
 *  ret_path is the output area (must be of size MAXPGPATH)
 *  target_path is the compiled-in path to the directory we want to find
 *  bin_path is the compiled-in path to the directory of executables
 *  my_exec_path is the actual location of my executable
 *
 * We determine the common prefix of target_path and bin_path, then compare
 * the remainder of bin_path to the last directory component(s) of
 * my_exec_path.  If they match, build the result as the part of my_exec_path
 * preceding the match, joined to the remainder of target_path.  If no match,
 * return target_path as-is.
 *
 * For example:
 *      target_path  = '/usr/local/share/postgresql'
 *      bin_path     = '/usr/local/bin'
 *      my_exec_path = '/opt/pgsql/bin/postmaster'
 * Given these inputs, the common prefix is '/usr/local/', the tail of
 * bin_path is 'bin' which does match the last directory component of
 * my_exec_path, so we would return '/opt/pgsql/share/postgresql'
 */

but since we split out and lib into 2 different outputs, target_path and bin_path are not in the same derivation output when target_path is LIBDIR or PKGLIBDIR. With this PR, I force the target_path to be the correct.

Thanks

@jb55
Copy link
Contributor

jb55 commented Sep 1, 2016

Looking forward to this. I was holding off on a work project for now because I couldn't get extensions working :[

@domenkozar
Copy link
Member

@lancelotsix did you verify that postgis test passes?

@lsix
Copy link
Member Author

lsix commented Sep 2, 2016

@domenkozar

$ nix-build nixos/tests/postgresql.nix
/nix/store/5y3h5la10w0j44v1p0lwr8i2hhffqlps-vm-test-run-postgresql
$ nix-build nixos/tests/postgis.nix
/nix/store/mgj16drf58yk4ybbsgmnkp4vzhp9aqal-vm-test-run-postgresql

@domenkozar
Copy link
Member

If @vcunat doesn't have any objections, I'll merge this soonish.

Fixes NixOS#15512 and NixOS#16032

With the multi output, postgresql cannot find at runtime what is its
basedir when looking for libdir and pkglibdir. This commit fixes that.
@lsix
Copy link
Member Author

lsix commented Sep 2, 2016

I have just updated the name attribute of the postgis test to postgis

@vcunat
Copy link
Member

vcunat commented Sep 2, 2016

OK with me. It would be nicer to hard-code the paths directly in the executables, but that might be complicated if they use the same path for things that we split now...

@vcunat
Copy link
Member

vcunat commented Sep 2, 2016

I say OK but mean thanks! ;-)

@vcunat vcunat merged commit 5b8072f into NixOS:master Sep 2, 2016
@vcunat
Copy link
Member

vcunat commented Sep 2, 2016

@domenkozar: do you count on this for 16.09 or not?

vcunat added a commit that referenced this pull request Sep 2, 2016
@domenkozar
Copy link
Member

Yes

On Fri, Sep 2, 2016, 20:10 Vladimír Čunát notifications@github.com wrote:

@domenkozar https://github.com/domenkozar: do you count on this for
16.09 or not?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#17838 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHtgwBW-V5I5wzYzunuiXyvHze_PIz3ks5qmGaigaJpZM4JoNGO
.

vcunat added a commit that referenced this pull request Sep 2, 2016
@vcunat
Copy link
Member

vcunat commented Sep 2, 2016

Picked.

@lsix
Copy link
Member Author

lsix commented Sep 2, 2016

@vcunat we cannot know what will be the final path at compilation time. postgres will have to load from the same path both its core libraries and extensions (such as postgis). The derivation containing all those files is not known until we go thru buildEnv in the service file. Unless we patch the binary itself, we have to use this environment variable (or we could introduce a new entry in the configuration file, but this would be way more intrusive).

@vcunat
Copy link
Member

vcunat commented Sep 2, 2016

Ah, I see, external extensions... I did no detailed reading of this PR.

adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
Ma27 pushed a commit that referenced this pull request Aug 24, 2024
Dynamic modules are technically libraries, but are not used by other packages.
Instead they are loaded by PostgreSQL itself, e.g. as extensions. Those just
increased the size of the lib output without benefit.

This removes the NIX_PGLIBDIR hack introduced in #17838 and instead makes sure
that pg_config always returns the correct PGLIBDIR. The test for postgis
introduced in the same PR is still passing with the change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9.needs: reporter feedback This issue needs the person who filed it to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants