Extensions vs PGXS' MODULE_PATHNAME handling

Tom Lane <tgl@sss.pgh.pa.us> writes:

pgxs.mk will substitute for MODULE_PATHNAME in it is
"$libdir/hstore-1.0" ... not exactly what's wanted. This is because the
transformation rule depends on $*, ie the base name of the input file.

[...]

On balance #3 seems the least bad, but I wonder if anyone sees this
choice differently or has another solution that I didn't think of.

A though that is occurring to me here would be to add a shlib property
in the control file and have the SQL script use $libdir/$shlib, or even
$shlib maybe. That would only work for extensions scripts, and even
only for those containing a single .so.

But the only counter example I know of is PGQ, and its install script is
ran by its command line tools. So PGQ would now ship 2 or 3 extensions
with some dependencies, each with its own .so. Seems cleaner for me
anyway.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

pgxs.mk will substitute for MODULE_PATHNAME in it is
"$libdir/hstore-1.0" ... not exactly what's wanted. This is because the
transformation rule depends on $*, ie the base name of the input file.

A though that is occurring to me here would be to add a shlib property
in the control file and have the SQL script use $libdir/$shlib, or even
$shlib maybe. That would only work for extensions scripts, and even
only for those containing a single .so.

Right, the basic difficulty here is exactly that in a Makefile that's
building multiple shlibs, there is no easy way to decide which shlibs go
with which sql scripts. The existing implementation essentially relies
on the base name of the sql script matching the base name of the shlib.
Adding a single-valued shlib property wouldn't improve matters at all.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Right, the basic difficulty here is exactly that in a Makefile that's
building multiple shlibs, there is no easy way to decide which shlibs go
with which sql scripts. The existing implementation essentially relies
on the base name of the sql script matching the base name of the shlib.
Adding a single-valued shlib property wouldn't improve matters at all.

My take here is to way that in this case, the current (9.1) way to deal
with the situation is to have multiple extensions when you have multiple
shlibs. After all we know that multiple extensions from the same
Makefile works, thanks to contrib/spi (I mean extension/spi).

And we even have inter-extensions dependencies in 9.1, so that's
friendly enough I think.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

Right, the basic difficulty here is exactly that in a Makefile that's
building multiple shlibs, there is no easy way to decide which shlibs go
with which sql scripts. The existing implementation essentially relies
on the base name of the sql script matching the base name of the shlib.
Adding a single-valued shlib property wouldn't improve matters at all.

My take here is to way that in this case, the current (9.1) way to deal
with the situation is to have multiple extensions when you have multiple
shlibs. After all we know that multiple extensions from the same
Makefile works, thanks to contrib/spi (I mean extension/spi).

But contrib/spi is exactly the case where it *won't* work. We need to
somehow figure out that $libdir/autoinc is what to substitute in
autoinc-1.0.sql, $libdir/insert_username in insert_username-1.0.sql,
etc.

Also, I've been looking at the pg_available_extensions issue a bit.
I don't yet have a proposal for exactly how we ought to redefine it,
but I did notice that the existing code is terribly confused by
secondary control files: it doesn't realize that they're not primary
control files, so you get e.g. hstore and hstore-1.0 as separate
listings.

We could possibly work around that by giving secondary control files a
different extension, but I'm becoming more and more convinced that it's
just a bad idea to have a file naming rule in which it's ambiguous where
the extension name stops and the version name starts.

I did think of another idea besides forbidding dash in extension names:
what if we use double dash as the name/version separator, ie the naming
conventions are like
extension--version.control
extension--version.sql
extension--oldversion-newversion.sql
Then we'd only have to forbid double dash in extension names, which
seems unlikely to be a problem for anybody. (I think we might also have
to forbid empty version names to make this bulletproof, but that doesn't
bother me much either.)

Comments?

regards, tom lane

On Feb 12, 2011, at 2:29 PM, Tom Lane wrote:

I did think of another idea besides forbidding dash in extension names:
what if we use double dash as the name/version separator, ie the naming
conventions are like
extension--version.control
extension--version.sql
extension--oldversion-newversion.sql
Then we'd only have to forbid double dash in extension names, which
seems unlikely to be a problem for anybody. (I think we might also have
to forbid empty version names to make this bulletproof, but that doesn't
bother me much either.)

+1 You might even consider mandating a double-dash between versions, so that they could have dashes:

extension--oldversion--newversion.sql

We don't have to worry about the length of the file name, do we?

Best,

David

"David E. Wheeler" <david@kineticode.com> writes:

On Feb 12, 2011, at 2:29 PM, Tom Lane wrote:

I did think of another idea besides forbidding dash in extension names:
what if we use double dash as the name/version separator,

+1 You might even consider mandating a double-dash between versions, so that they could have dashes:
extension--oldversion--newversion.sql

Hm. I think we'd still have to disallow dash as the first or last
character in a version name to make that unambiguous. Not sure it's
worth the trouble.

regards, tom lane

On Feb 12, 2011, at 3:12 PM, Tom Lane wrote:

"David E. Wheeler" <david@kineticode.com> writes:

On Feb 12, 2011, at 2:29 PM, Tom Lane wrote:

I did think of another idea besides forbidding dash in extension names:
what if we use double dash as the name/version separator,

+1 You might even consider mandating a double-dash between versions, so that they could have dashes:
extension--oldversion--newversion.sql

Hm. I think we'd still have to disallow dash as the first or last
character in a version name to make that unambiguous. Not sure it's
worth the trouble.

How likely is *that*?

David

"David E. Wheeler" <david@kineticode.com> writes:

On Feb 12, 2011, at 3:12 PM, Tom Lane wrote:

Hm. I think we'd still have to disallow dash as the first or last
character in a version name to make that unambiguous. Not sure it's
worth the trouble.

How likely is *that*?

Not very, but the rules are getting a bit complicated ...

regards, tom lane

On Feb 12, 2011, at 3:37 PM, Tom Lane wrote:

How likely is *that*?

Not very, but the rules are getting a bit complicated ...

Doesn't seem complicated to me:

1. Use -- to separate extension name, old version, new version
2. Don't use - at the beginning or end of name or version number
3. Profit

How hard is that?

David

Tom Lane <tgl@sss.pgh.pa.us> writes:

My take here is to way that in this case, the current (9.1) way to deal
with the situation is to have multiple extensions when you have multiple
shlibs. After all we know that multiple extensions from the same
Makefile works, thanks to contrib/spi (I mean extension/spi).

But contrib/spi is exactly the case where it *won't* work. We need to
somehow figure out that $libdir/autoinc is what to substitute in
autoinc-1.0.sql, $libdir/insert_username in insert_username-1.0.sql,
etc.

Indeed. That's why I'm proposing to have that setup in the control
file, which is per extension, rather than in the common Makefile.

Also, I've been looking at the pg_available_extensions issue a bit.
I don't yet have a proposal for exactly how we ought to redefine it,
but I did notice that the existing code is terribly confused by
secondary control files: it doesn't realize that they're not primary
control files, so you get e.g. hstore and hstore-1.0 as separate
listings.

I'd think that's it's a good idea if dealt with "correctly" because now
that ALTER EXTENSION UPDATE can deal with more than one target VERSION
I expect the view to show each available update here.

If possible adding the "update chain sequence" information as computed
in the code would be great. Because we can't ask people to figure that
out all by themselves, the best way to check your upgrading setup is
fine would be to run SELECT * FROM pg_available_extensions; and read the
result.

We could possibly work around that by giving secondary control files a
different extension, but I'm becoming more and more convinced that it's
just a bad idea to have a file naming rule in which it's ambiguous where
the extension name stops and the version name starts.

Agreed.

I did think of another idea besides forbidding dash in extension names:
what if we use double dash as the name/version separator, ie the naming
conventions are like
extension--version.control
extension--version.sql
extension--oldversion-newversion.sql

Yeah, something like that would work, so would maybe using ':' and
forbidding one-letter extension names, but I'm not in a position to
check that this won't confuse the windows we support too much. I see
about no downside to the double dash proposal, that said.

Then we'd only have to forbid double dash in extension names, which
seems unlikely to be a problem for anybody. (I think we might also have
to forbid empty version names to make this bulletproof, but that doesn't
bother me much either.)

Those look like sanity checks more than anything else, I'd welcome us
having them.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

But contrib/spi is exactly the case where it *won't* work. We need to
somehow figure out that $libdir/autoinc is what to substitute in
autoinc-1.0.sql, $libdir/insert_username in insert_username-1.0.sql,
etc.

Indeed. That's why I'm proposing to have that setup in the control
file, which is per extension, rather than in the common Makefile.

How's that help? In a makefile building more than one extension,
you'd still need a way to decide which extension the current script
file is associated with.

Or are you suggesting substituting for MODULE_PATHNAME during CREATE
EXTENSION, and not during "make" at all? That would work I guess.
I'm hesitant to have any substitutions that happen unconditionally,
but we could add a control parameter like
module_pathname = '$libdir/hstore'
and then things would be pretty clean.

I think we should still change the file naming conventions to use double
dashes, though, since there's more than one reason to want that. Will
work on that next.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Or are you suggesting substituting for MODULE_PATHNAME during CREATE
EXTENSION, and not during "make" at all? That would work I guess.

That's my idea, sorry not having made it clear enough. We have $libdir
which is expanded server-side AFAIUI, I though we would have $shlib
expanded the same way and taken from some backend variable like with
creating_extension.

I'm hesitant to have any substitutions that happen unconditionally,
but we could add a control parameter like
module_pathname = '$libdir/hstore'
and then things would be pretty clean.

Ok. Maybe the simpler would be to make the current control variable a
static backend variable so that EXT_CONTROL(module_pathname) is easy to
find out from anywhere (I see you got rid of some direct usage of static
variables with recordDependencyOnCurrentExtension() already).

I think we should still change the file naming conventions to use double
dashes, though, since there's more than one reason to want that. Will
work on that next.

Great!

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

I'm hesitant to have any substitutions that happen unconditionally,
but we could add a control parameter like
module_pathname = '$libdir/hstore'
and then things would be pretty clean.

Ok. Maybe the simpler would be to make the current control variable a
static backend variable so that EXT_CONTROL(module_pathname) is easy to
find out from anywhere (I see you got rid of some direct usage of static
variables with recordDependencyOnCurrentExtension() already).

I think it's better to keep it working as a textual substitution.
That poses the least risk of breaking scripts that work today ---
who's to say that somebody might not be relying on the substitution
happening someplace else than CREATE FUNCTION's shlib string?

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

I think it's better to keep it working as a textual substitution.
That poses the least risk of breaking scripts that work today ---
who's to say that somebody might not be relying on the substitution
happening someplace else than CREATE FUNCTION's shlib string?

Fair enough, I suppose. So +1 from me, FWIW.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

I think it's better to keep it working as a textual substitution.
That poses the least risk of breaking scripts that work today ---
who's to say that somebody might not be relying on the substitution
happening someplace else than CREATE FUNCTION's shlib string?

Fair enough, I suppose. So +1 from me, FWIW.

OK, so with that, attached is an example of the complete conversion diff
for a contrib module (hstore in particular). Although "git status"
reports hstore--1.0.sql as being a rename of hstore.sql.in, "git diff"
doesn't seem to be exceedingly bright about presenting it that way :-(.
But actually the change in that script other than renaming is just
removing the "set search_path" command and adjusting the header comment.

I've checked that regression tests pass and "create extension hstore
from unpackaged" successfully upgrades from a 9.0 dump. I don't have
the ability to check that it works on Windows too, but since we're not
hacking pgxs.mk I doubt that there's anything to do to the Windows build
process.

Barring objections, I'll press on with fixing the rest of them.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

I think it's better to keep it working as a textual substitution.

Thinking about this some more, it has the advantage that the effects of
the control file settings are kept within the script file processing and
pg_extension catalog. The only backend impact is the dependency
tracking.

OK, so with that, attached is an example of the complete conversion diff
for a contrib module (hstore in particular). Although "git status"

I see you're not using the @extschema@ placeholder in the upgrade
script. It is intentional? It's been common wisdom and practice to
edit the SQL file of any contrib or third party module to have it
installed in your preferred schema…

reports hstore--1.0.sql as being a rename of hstore.sql.in, "git diff"
doesn't seem to be exceedingly bright about presenting it that way :-(.
But actually the change in that script other than renaming is just
removing the "set search_path" command and adjusting the header comment.

And we don't have to rely on hstore.sql.in file anymore as the change is
done by the backend side of things. That's a very good point for the
windows build system I think.

Barring objections, I'll press on with fixing the rest of them.

I think you'd be interested into this reworked SQL query. It should be
providing exactly the script file you need as an upgrade from unpackaged.

I took the time to finish this query (filter out array types, some
replacement in operator classes and families descriptions) because I
think it would be nice to offer it in the docs. It could even be
proposed as a function :)

I hope you'll find it useful, but it could well be you finished the
search&replace of all contribs already (ah, emacs keyboard macros).

CREATE EXTENSION hstore;

CREATE SCHEMA empty_place;
SET search_path TO empty_place;

WITH objs AS (
SELECT classid, 'ALTER EXTENSION ' || E.extname || ' ADD '
|| replace(pg_describe_object(classid, objid, 0),
N.nspname, '@extschema@')
|| ';' as sql
FROM pg_depend D
JOIN pg_extension E ON D.refobjid = E.oid
AND D.refclassid = E.tableoid
JOIN pg_namespace N ON E.extnamespace = N.oid
WHERE CASE WHEN classid = 'pg_catalog.pg_type'::regclass
THEN (SELECT typarray FROM pg_type WHERE oid=objid) != 0
ELSE true
END
AND deptype = 'e' AND E.extname = 'hstore'
)
SELECT
CASE WHEN classid IN ('pg_catalog.pg_opclass'::regclass,
'pg_catalog.pg_opfamily'::regclass)
THEN replace(sql, 'for access method', 'using')
ELSE sql
END
FROM objs;

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

OK, so with that, attached is an example of the complete conversion diff
for a contrib module (hstore in particular). Although "git status"

I see you're not using the @extschema@ placeholder in the upgrade
script. It is intentional?

Yes, it should be unnecessary given the search_path setup done by
execute_extension_script(). Also, I think that a relocatable
extension's script should not be subject to @extschema@ substitution,
no matter what.

I think you'd be interested into this reworked SQL query. It should be
providing exactly the script file you need as an upgrade from unpackaged.

This seems overly complicated. I have a version of it that I'll publish
as soon as I've tested it on all the contrib modules ...

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Yes, it should be unnecessary given the search_path setup done by
execute_extension_script(). Also, I think that a relocatable
extension's script should not be subject to @extschema@ substitution,
no matter what.

Oh I'm just realizing that my reasoning predates the search_path strong
guarantees at CREATE EXTENSION time.

I think you'd be interested into this reworked SQL query. It should be
providing exactly the script file you need as an upgrade from unpackaged.

This seems overly complicated. I have a version of it that I'll publish
as soon as I've tested it on all the contrib modules ...

Nice. I confess I worked out mine from my last patch where I still have
the INTERNAL dependencies setup etc, so maybe that makes it more complex
that it needs to be.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Feb 13, 2011, at 11:34 AM, Tom Lane wrote:

OK, so with that, attached is an example of the complete conversion diff
for a contrib module (hstore in particular). Although "git status"
reports hstore--1.0.sql as being a rename of hstore.sql.in, "git diff"
doesn't seem to be exceedingly bright about presenting it that way :-(.
But actually the change in that script other than renaming is just
removing the "set search_path" command and adjusting the header comment.

I sure would like it if the install script with no version in it corresponded to the latest version. Otherwise, one must rename the file every time one does a release. And as you're noting, you lose Git history that way.

Best,

David