Hello Peter & Co. On Dec 5, 2024, at 06:07, Peter Eisentraut <pe...@eisentraut.org> wrote:
> I've made a bit of progress on this patch, filled in some documentation and
> resolved some TODO markers. Also:
Finally getting around to reviewing this patch. Should it be considered part of
the previous patch[1] for purposes of commitfest tracking?
I’ve also created a GitHub PR[2] for anyone who’d prefer to look it over that
way.
>> Some open problems or discussion points:
>> - You can install extensions into alternative directories using PGXS like
>> make install datadir=/else/where/share pkglibdir=/else/where/lib
>> This works. I was hoping it would work to use
>> make install prefix=/else/where
>> but that doesn't because of some details in Makefile.global. I think we can
>> tweak that a little bit to make that work too.
>
> This works now.
I tried `prefix` with semver[3] and it did not work:
``` console
❯ make PG_CONFIG=~/dev/misc/postgres/pgsql-devel/bin/pg_config
prefix=/Users/david/pgsql-test
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement
-Werror=vla -Werror=unguarded-availability-new -Wendif-labels
-Wmissing-format-attribute -Wcast-function-type -Wformat-security
-Wmissing-variable-declarations -fno-strict-aliasing -fwrapv
-fexcess-precision=standard -Wno-unused-command-line-argument
-Wno-compound-token-split-by-macro -Wno-cast-function-type-strict
-I/opt/homebrew/opt/readline/include -I/opt/homebrew/opt/openssl/include
-I/opt/homebrew/opt/libxml2/include -I/opt/homebrew/opt/icu4c/include
-fvisibility=hidden -I. -I./ -I/Users/david/pgsql-test/include/server
-I/Users/david/pgsql-test/include/internal
-I/opt/homebrew/Cellar/icu4c@76/76.1_1/include -isysroot
/Library/Developer/CommandLineTools/SDKs/MacOSX15.1.sdk
-I/opt/homebrew/opt/readline/include -I/opt/homebrew/opt/openssl/include
-I/opt/homebrew/opt/libxml2/include -I/opt/homebrew/opt/icu4c/include -c -o
src/semver.o src/semver.c
src/semver.c:13:10: fatal error: 'postgres.h' file not found
13 | #include "postgres.h"
| ^~~~~~~~~~~~
1 error generated.
make: *** [src/semver.o] Error 1
```
It works fine without `prefix` to install into the default directories as
before. It also installs fine with `datadir` and `pkglibdir`:
``` console
❯ make PG_CONFIG=~/dev/misc/postgres/pgsql-devel/bin/pg_config
datadir=/Users/david/pgsql-test/share pkglibdir=/Users/david/pgsql-test/lib
install
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/extension'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/semver'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/lib'
/opt/homebrew/bin/gmkdir -p
'/Users/david/dev/misc/postgres/pgsql-devel/share/doc/semver'
/opt/homebrew/bin/ginstall -c -m 644 .//semver.control
'/Users/david/pgsql-test/share/extension/'
/opt/homebrew/bin/ginstall -c -m 644 .//sql/semver--0.10.0--0.11.0.sql
.//sql/semver--0.11.0--0.12.0.sql .//sql/semver--0.12.0--0.13.0.sql
.//sql/semver--0.13.0--0.15.0.sql .//sql/semver--0.15.0--0.16.0.sql
.//sql/semver--0.16.0--0.17.0.sql .//sql/semver--0.17.0--0.20.0.sql
.//sql/semver--0.2.1--0.2.4.sql .//sql/semver--0.2.4--0.3.0.sql
.//sql/semver--0.20.0--0.21.0.sql .//sql/semver--0.21.0--0.22.0.sql
.//sql/semver--0.22.0--0.30.0.sql .//sql/semver--0.3.0--0.4.0.sql
.//sql/semver--0.30.0--0.31.0.sql .//sql/semver--0.31.0--0.31.1.sql
.//sql/semver--0.31.1--0.31.2.sql .//sql/semver--0.31.2--0.32.0.sql
.//sql/semver--0.5.0--0.10.0.sql .//sql/semver--unpackaged--0.2.1.sql
'/Users/david/pgsql-test/share/semver/'
/opt/homebrew/bin/ginstall -c -m 755 src/semver.dylib
'/Users/david/pgsql-test/lib/'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/lib/bitcode/src/semver'
/opt/homebrew/bin/gmkdir -p
'/Users/david/pgsql-test/lib/bitcode'/src/semver/src/
/opt/homebrew/bin/ginstall -c -m 644 src/semver.bc
'/Users/david/pgsql-test/lib/bitcode'/src/semver/src/
cd '/Users/david/pgsql-test/lib/bitcode' &&
/opt/homebrew/Cellar/llvm/19.1.6/bin/llvm-lto -thinlto -thinlto-action=thinlink
-o src/semver.index.bc src/semver/src/semver.bc
/opt/homebrew/bin/ginstall -c -m 644 .//doc/semver.mmd
'/Users/david/dev/misc/postgres/pgsql-devel/share/doc/semver/'
```
But then it won’t load:
```psql
postgres=# create extension semver;
ERROR: extension "semver" has no installation script nor update path for
version “0.40.0"
```
Since `semver` uses the directory parameter[4], I decided to try another C
extension that doesn’t use it, envvar[5], which worked:
``` psql
postgres=# create extension envvar;
CREATE EXTENSION
```
So I suspect the issue is that, when looking for SQL files, the patch needs to
use the directory parameter[4] when it’s set --- and it can be an absolute
path! Honestly I think there’s a case to be made for eliminating that parameter.
The `prefix` param works with a pure SQL extension like pair[6]:
```console
❯ make PG_CONFIG=~/dev/misc/postgres/pgsql-devel/bin/pg_config
prefix=/Users/david/pgsql-test
cp sql/pair.sql sql/pair--0.1.2.sql
❯ make PG_CONFIG=~/dev/misc/postgres/pgsql-devel/bin/pg_config
prefix=/Users/david/pgsql-test install
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/extension'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/extension'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/doc//extension'
/opt/homebrew/bin/ginstall -c -m 644 .//pair.control
'/Users/david/pgsql-test/share/extension/'
/opt/homebrew/bin/ginstall -c -m 644 .//sql/pair--0.1.2.sql
.//sql/pair--unpackaged--0.1.2.sql '/Users/david/pgsql-test/share/extension/'
/opt/homebrew/bin/ginstall -c -m 644 .//doc/pair.md
'/Users/david/pgsql-test/share/doc//extension/‘
```
I thought it would put the files into /Users/david/pgsql-test/extension, not
/Users/david/pgsql-test/share/extension? I guess that makes sense; but then
perhaps the search path should be for prefixes, in which case I’d use a config
like:
``` ini
extension_control_path = '/Users/david/pgsql-test:$system'
dynamic_library_path = '/Users/david/pgsql-test/lib:$libdir'
```
And the search path would append `share/extension` to each path. But then it
varies from the behavior of `dynamic_library_path`. :-(
Not at all sure where the doc files should go unless, again, prefix is truly
used as a prefix for all the things, which frankly seems reasonable.
I’m wondering whether there should be formal documentation of prefix, datadir,
pkglibdir, etc. I haven’t noticed them in the PGXS docs[7].
>> This disables the use of dynamic_library_path, so this whole idea of
>> installing an extension elsewhere won't work that way. The obvious solution
>> is that extensions change this to just 'foo'. But this will require a lot
>> updating work for many extensions, or a lot of patching by packagers.
>
>
> I have solved this by just stripping off "$libdir/" from the front of the
> filename. This works for now. We can think about other ways to tweak this,
> perhaps, but I don't see any drawback to this in practice.
I think this makes perfect sense. I presume it’s stripped out *before*
replacing the MODULE_PATHNAME string in SQL files, yes?
# Patch Review
Compiles fine. All tests pass. Some comments on the docs:
> <primary><varname>extension_control_path</varname> configuration
> parameter</primary>
Although the current path explicitly searches for control files, I’d like to
suggest a more generic name, since the point is to find extensions; control
files are just the (current) method for identifying them. I suggest
`extension_path`. Although given the above, maybe it should be all about
prefixes.
> The value for <varname>extension_control_path</varname> must be a
> list of absolute directory paths separated by colons (or semi-colons
> on Windows). If a list element starts
> with the special string <literal>$system</literal>, the
I like this idea, though I quibble with the term `$system`, which seems like it
would identify SYSCONFDIR, not SHAREDIR/extension. How about `$extdir` or, if
using prefixes, `$coreprefix` or something.
> Note that the path elements should typically end in
> <literal>extension</literal> if the normal installation layouts are
> followed. (The value for <literal>$system</literal> already includes
> the <literal>extension</literal> suffix.)
In fact, if we’re using `prefix` as currently implemented, it should end in
`share/extension`, no?
> Note that if you set this parameter to be able to load extensions from
> nonstandard locations, you will most likely also need to set <xref
> linkend="guc-dynamic-library-path"/> to a correspondent location, for
s/correspondent/corresponding/
> example,
> <programlisting>
> extension_control_path = '/usr/local/share/postgresql/extension:$system'
> dynamic_library_path = '/usr/local/lib/postgresql:$libdir'
> </programlisting>
This makes sense in the context of this patch, but I sure would like to see
these features decoupled as much as possible.
<digression type="brief">
Hence my proposal that extensions each have their own directory. For example,
via Slack Gabriele sent Peter and me a POC for loading individual extensions as
mounted volumes in an OCI container. The Kubernetes config looks like this:
``` yaml
postgresql:
shared_preload_libraries:
- pg_squeeze
parameters:
extension_control_path:
'/extensions/pgvector/18/share:/extensions/pgsqueeze/18/share:$system'
dynamic_library_path:
'/extensions/pgvector/18/lib:/extensions/pgsqueeze/18/lib:$libdir'
extensions:
- name: pgvector
image:
reference: ghcr.io/cloudnative-pg/pgvector-testing:0.8.0
- name: pgsqueeze
image:
reference: ghcr.io/cloudnative-pg/pgsqueeze-testing:1.7
```
Which works! But it also means that new directories need to be appended to
`extension_control_path` and, often `dynamic_library_path` for every extension
added. It would be much nicer to just have 2-3 paths that contain extensions
identified by a directory name. Then, to add a new extension, one just installs
it to the appropriate prefix. (This is also yet another reason to eliminate the
directory parameter[4].)
This is only additional wish I had for this feature, but I also believe we can
iterate in that direction based on your patch.
</digression>
> the installation's <literal>SHAREDIR</literal> directory. By default,
> the script files are looked for in the same directory where the
> control file was found.
This could use a pointer to the impact of the directory parameter[4]. Bit of a
wildcard for DBAs who want to use this feature, TBH.
Best,
David
[1]: https://commitfest.postgresql.org/51/4913/
[2]: https://github.com/theory/postgres/pull/9/files
[3]: https://github.com/theory/pg-semver
[4]:
https://www.postgresql.org/docs/current/extend-extensions.html#EXTEND-EXTENSIONS-FILES-DIRECTORY
[5]: https://github.com/theory/pg-envvar
[6]: https://github.com/theory/kv-pair
[7]: https://www.postgresql.org/docs/current/extend-pgxs.html
signature.asc
Description: Message signed with OpenPGP
