*.sql contrib files contain unresolvable MODULE_PATHNAME

On 10/12/2011 09:53 AM, Martin Pitt wrote:

Hello all,

In https://launchpad.net/bugs/835502 it was reported that the 9.1.1
contrib *.sql files contain the token "MODULE_PATHNAME", which is
unknown:

psql test < /usr/share/postgresql/9.1/extension/intarray--1.0.sql

This fails with a ton of errors about the file "MODULE_PATHNAME" not
existing.

When I replace this with "$libdir/_int", it works:

sed 's!MODULE_PATHNAME!$libdir/_int!g' /usr/share/postgresql/9.1/extension/intarray--1.0.sql | psql test

Is that something I'm doing wrong in the packaging, or should the
contrib Makefiles be fixed to do this substitution?

It doesn't only affect intarray, but pretty much all *.sql files.

uh - the reason is that contrib is now packaged as extensions and that
you are supposed to use "CREATE EXTENSION intarray;" on the SQL level
instead of manually loading sql-scripts through psql.

Stefan

On 12.10.2011 10:58, Stefan Kaltenbrunner wrote:

On 10/12/2011 09:53 AM, Martin Pitt wrote:

Hello all,

In https://launchpad.net/bugs/835502 it was reported that the 9.1.1
contrib *.sql files contain the token "MODULE_PATHNAME", which is
unknown:

psql test< /usr/share/postgresql/9.1/extension/intarray--1.0.sql

This fails with a ton of errors about the file "MODULE_PATHNAME" not
existing.

When I replace this with "$libdir/_int", it works:

sed 's!MODULE_PATHNAME!$libdir/_int!g' /usr/share/postgresql/9.1/extension/intarray--1.0.sql | psql test

Is that something I'm doing wrong in the packaging, or should the
contrib Makefiles be fixed to do this substitution?

It doesn't only affect intarray, but pretty much all *.sql files.

uh - the reason is that contrib is now packaged as extensions and that
you are supposed to use "CREATE EXTENSION intarray;" on the SQL level
instead of manually loading sql-scripts through psql.

9.1 has been out for only a couple of months, and we've seen a lot of
people trying to do that already. In hindsight, we probably should've
chosen a different filename extension for those files, to make it clear
that you can't just run them in psql. It's too late for that, but a
comment at the top of the .sql files would be good:

--- a/contrib/intarray/intarray--1.0.sql
+++ b/contrib/intarray/intarray--1.0.sql
@@ -1,4 +1,8 @@
-/* contrib/intarray/intarray--1.0.sql */
+/*
+ * contrib/intarray/intarray--1.0.sql
+ *
+ * Script file to be run by CREATE EXTENSION.
+ */

--
-- Create the user-defined type for the 1-D integer arrays (_int4)

The people trying to run these files with psql look inside the file when
they get the error, so mentioning "CREATE EXTENSION" should give a hint
on what to do.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Wed, Oct 12, 2011 at 10:39, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

On 12.10.2011 10:58, Stefan Kaltenbrunner wrote:

On 10/12/2011 09:53 AM, Martin Pitt wrote:

Hello all,

In https://launchpad.net/bugs/835502 it was reported that the 9.1.1
contrib *.sql files contain the token "MODULE_PATHNAME", which is
unknown:

  psql test<  /usr/share/postgresql/9.1/extension/intarray--1.0.sql

This fails with a ton of errors about the file "MODULE_PATHNAME" not
existing.

When I replace this with "$libdir/_int", it works:

  sed 's!MODULE_PATHNAME!$libdir/_int!g'
/usr/share/postgresql/9.1/extension/intarray--1.0.sql | psql test

Is that something I'm doing wrong in the packaging, or should the
contrib Makefiles be fixed to do this substitution?

It doesn't only affect intarray, but pretty much all *.sql files.

uh - the reason is that contrib is now packaged as extensions and that
you are supposed to use "CREATE EXTENSION intarray;" on the SQL level
instead of manually loading sql-scripts through psql.

9.1 has been out for only a couple of months, and we've seen a lot of people
trying to do that already. In hindsight, we probably should've chosen a
different filename extension for those files, to make it clear that you
can't just run them in psql. It's too late for that, but a comment at the
top of the .sql files would be good:

--- a/contrib/intarray/intarray--1.0.sql
+++ b/contrib/intarray/intarray--1.0.sql
@@ -1,4 +1,8 @@
-/* contrib/intarray/intarray--1.0.sql */
+/*
+ * contrib/intarray/intarray--1.0.sql
+ *
+ * Script file to be run by CREATE EXTENSION.
+ */

 --
 -- Create the user-defined type for the 1-D integer arrays (_int4)

The people trying to run these files with psql look inside the file when
they get the error, so mentioning "CREATE EXTENSION" should give a hint on
what to do.

Hmm. is there some way that we could make it do something that would
affect only psql? I guess not, because any kind of \-command would
actually break the CREATE EXTENSION running of the script, right?

But it would be useful to be able to inject something there that psql
would notice but the backend would ignore...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 10/12/2011 04:39 AM, Heikki Linnakangas wrote:

On 12.10.2011 10:58, Stefan Kaltenbrunner wrote:

On 10/12/2011 09:53 AM, Martin Pitt wrote:

Hello all,

In https://launchpad.net/bugs/835502 it was reported that the 9.1.1
contrib *.sql files contain the token "MODULE_PATHNAME", which is
unknown:

psql test< /usr/share/postgresql/9.1/extension/intarray--1.0.sql

This fails with a ton of errors about the file "MODULE_PATHNAME" not
existing.

When I replace this with "$libdir/_int", it works:

sed 's!MODULE_PATHNAME!$libdir/_int!g'
/usr/share/postgresql/9.1/extension/intarray--1.0.sql | psql test

Is that something I'm doing wrong in the packaging, or should the
contrib Makefiles be fixed to do this substitution?

It doesn't only affect intarray, but pretty much all *.sql files.

uh - the reason is that contrib is now packaged as extensions and that
you are supposed to use "CREATE EXTENSION intarray;" on the SQL level
instead of manually loading sql-scripts through psql.

9.1 has been out for only a couple of months, and we've seen a lot of
people trying to do that already. In hindsight, we probably should've
chosen a different filename extension for those files, to make it
clear that you can't just run them in psql. It's too late for that,
but a comment at the top of the .sql files would be good:

I've made this mistake myself in an unthinking moment. I suggest that we
deprecate calling them something.sql and add code ASAP providing for
some other suffix ( .xtn ?) with legacy support for falling back to
.sql. I'd almost be inclined to backpatch it while there are so few
third party extensions out there.

cheers

andrew

On Wed, Oct 12, 2011 at 4:42 AM, Magnus Hagander <magnus@hagander.net> wrote:

9.1 has been out for only a couple of months, and we've seen a lot of people
trying to do that already. In hindsight, we probably should've chosen a
different filename extension for those files, to make it clear that you
can't just run them in psql. It's too late for that, but a comment at the
top of the .sql files would be good:

--- a/contrib/intarray/intarray--1.0.sql
+++ b/contrib/intarray/intarray--1.0.sql
@@ -1,4 +1,8 @@
-/* contrib/intarray/intarray--1.0.sql */
+/*
+ * contrib/intarray/intarray--1.0.sql
+ *
+ * Script file to be run by CREATE EXTENSION.
+ */

 --
 -- Create the user-defined type for the 1-D integer arrays (_int4)

The people trying to run these files with psql look inside the file when
they get the error, so mentioning "CREATE EXTENSION" should give a hint on
what to do.

Hmm. is there some way that we could make it do something that would
affect only psql? I guess not, because any kind of \-command would
actually break the CREATE EXTENSION running of the script, right?

But it would be useful to be able to inject something there that psql
would notice but the backend would ignore...

We could do that, but I think Heikki's idea of adding a comment would
help a lot. When people try to run the file through psql and it fails
due to the MODULE_PATHNAME stuff, the next thing they're probably
going to do is look at the file contents. If they see something there
telling them to use CREATE EXTENSION, that will help.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

We could do that, but I think Heikki's idea of adding a comment would
help a lot.

+1. Simple, easy, should help significantly.

Also, I disagree with the position that the files "aren't SQL files".
Sure they are. You'd want them treated as SQL by your editor, for
example. So changing the extension is just wrong.

regards, tom lane

On 10/12/2011 10:12 AM, Tom Lane wrote:

Robert Haas<robertmhaas@gmail.com> writes:

We could do that, but I think Heikki's idea of adding a comment would
help a lot.

+1. Simple, easy, should help significantly.

Also, I disagree with the position that the files "aren't SQL files".
Sure they are. You'd want them treated as SQL by your editor, for
example. So changing the extension is just wrong.

*shrug* ok. Another thought I had was to have the file raise an error
and have that filtered out by the extension mechanism. But I'm not sure
if it's worth the trouble.

cheers

andrew

On Wed, Oct 12, 2011 at 16:31, Andrew Dunstan <andrew@dunslane.net> wrote:

On 10/12/2011 10:12 AM, Tom Lane wrote:

Robert Haas<robertmhaas@gmail.com>  writes:

We could do that, but I think Heikki's idea of adding a comment would
help a lot.

+1.  Simple, easy, should help significantly.

Also, I disagree with the position that the files "aren't SQL files".
Sure they are.   You'd want them treated as SQL by your editor, for
example.  So changing the extension is just wrong.

*shrug* ok. Another thought I had was to have the file raise an error and
have that filtered out by the extension mechanism. But I'm not sure if it's
worth the trouble.

How about adding something like
-- \\psql_hates_this
-- rest of comment

and then at least have new versions of psql find that and stop
processing the file with a more useful error at that point? Or maybe
that's overengineering..

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Andrew Dunstan <andrew@dunslane.net> writes:

*shrug* ok. Another thought I had was to have the file raise an error
and have that filtered out by the extension mechanism. But I'm not sure
if it's worth the trouble.

Hmm ...

\echo You should use CREATE EXTENSION foo to load this file!

and teach CREATE EXTENSION to drop any line beginning with \echo?
The latter part seems easy enough, but I'm not quite sure about the
wording or placement of the \echo command. Putting it at the top
feels natural but the message might scroll offscreen due to errors...

regards, tom lane

On Wed, Oct 12, 2011 at 10:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

*shrug* ok. Another thought I had was to have the file raise an error
and have that filtered out by the extension mechanism. But I'm not sure
if it's worth the trouble.

Hmm ...

\echo You should use CREATE EXTENSION foo to load this file!

and teach CREATE EXTENSION to drop any line beginning with \echo?
The latter part seems easy enough, but I'm not quite sure about the
wording or placement of the \echo command.  Putting it at the top
feels natural but the message might scroll offscreen due to errors...

Decorate them with a marker like:
\extension <name> <version>

And make the CREATE EXTENSION skip (or verify) it?

It will make psql stop on the \extension command.

a.

--
Aidan Van Dyk                                             Create like a god,
aidan@highrise.ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

On 12.10.2011 17:33, Magnus Hagander wrote:

How about adding something like
-- \\psql_hates_this
-- rest of comment

and then at least have new versions of psql find that and stop
processing the file with a more useful error at that point? Or maybe
that's overengineering..

Overengineering IMHO. Besides, if a psql poison comment like that
exists, then we'd have to be careful not to emit one elsewhere. Think
pg_dump, if someone puts that comment in a function body...

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Aidan Van Dyk <aidan@highrise.ca> writes:

On Wed, Oct 12, 2011 at 10:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hmm ...
\echo You should use CREATE EXTENSION foo to load this file!

Decorate them with a marker like:
\extension <name> <version>
And make the CREATE EXTENSION skip (or verify) it?
It will make psql stop on the \extension command.

No, the point is not to stop or fail, it is to print out an unmistakable
user instruction. Otherwise we'll still be getting "cube.sql failed to
load for me" bug reports. So I think \echo is entirely sufficient,
and we should not rely on psql features that aren't there yet. Ideally
this should do what we want even in older psql releases. \echo has been
there at least since 7.0.

It strikes me that we could get rid of the error message clutter
I worried about before if we coded like this:

/* contrib/foo--1.0.sql */

\echo Use "CREATE EXTENSION foo" to load this file. \quit

... SQL commands here ...

The forced \quit is a bit unfriendly maybe but it will get the job done.
And again, this isn't making any assumptions about which psql version
you're using.

regards, tom lane

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

On 12.10.2011 17:33, Magnus Hagander wrote:

How about adding something like
-- \\psql_hates_this
-- rest of comment

and then at least have new versions of psql find that and stop
processing the file with a more useful error at that point? Or maybe
that's overengineering..

Overengineering IMHO. Besides, if a psql poison comment like that
exists, then we'd have to be careful not to emit one elsewhere. Think
pg_dump, if someone puts that comment in a function body...

Well, it can't be a comment, but what about a real psql command?
See my suggestion of using \echo.

regards, tom lane

On 10/12/2011 11:15 AM, Tom Lane wrote:

\echo Use "CREATE EXTENSION foo" to load this file. \quit

+1 for this.

cheers

andrew

On 12.10.2011 18:20, Tom Lane wrote:

Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes:

On 12.10.2011 17:33, Magnus Hagander wrote:

How about adding something like
-- \\psql_hates_this
-- rest of comment

and then at least have new versions of psql find that and stop
processing the file with a more useful error at that point? Or maybe
that's overengineering..

Overengineering IMHO. Besides, if a psql poison comment like that
exists, then we'd have to be careful not to emit one elsewhere. Think
pg_dump, if someone puts that comment in a function body...

Well, it can't be a comment, but what about a real psql command?
See my suggestion of using \echo.

Frankly I think a comment is sufficient. We can make it more complicated
later if people are still confused.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Wed, Oct 12, 2011 at 11:24 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Overengineering IMHO. Besides, if a psql poison comment like that
exists, then we'd have to be careful not to emit one elsewhere. Think
pg_dump, if someone puts that comment in a function body...

Well, it can't be a comment, but what about a real psql command?
See my suggestion of using \echo.

Frankly I think a comment is sufficient. We can make it more complicated
later if people are still confused.

+1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

On 12.10.2011 18:20, Tom Lane wrote:

Well, it can't be a comment, but what about a real psql command?
See my suggestion of using \echo.

Frankly I think a comment is sufficient. We can make it more complicated
later if people are still confused.

The thing is that this will be the third time we've gone back to try to
make it more apparent that you should use CREATE EXTENSION, and I no
longer believe that mere documentation is really going to get the job
done. Putting in a comment will only stop the bug reports from people
who bother to examine the script contents before filing a report, but
the kind of folks who don't read the release notes probably won't do
that either. In fact, if we just put in a comment, I confidently
predict we'll be coming back to revisit this issue again in future.

The only thing the \echo approach will cost us is a few more lines of C
code in execute_extension_script(), and I think it's more than worth
that given the evident scope of the problem.

regards, tom lane

On Wed, Oct 12, 2011 at 17:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

On 12.10.2011 18:20, Tom Lane wrote:

Well, it can't be a comment, but what about a real psql command?
See my suggestion of using \echo.

Frankly I think a comment is sufficient. We can make it more complicated
later if people are still confused.

The thing is that this will be the third time we've gone back to try to
make it more apparent that you should use CREATE EXTENSION, and I no
longer believe that mere documentation is really going to get the job
done.  Putting in a comment will only stop the bug reports from people
who bother to examine the script contents before filing a report, but
the kind of folks who don't read the release notes probably won't do
that either.  In fact, if we just put in a comment, I confidently
predict we'll be coming back to revisit this issue again in future.

That's exactly my concern - I strongly doubt those not bothering to
read that even for a major release, aren't going to review the source
of the SQL scrpit either.

The only thing the \echo approach will cost us is a few more lines of C
code in execute_extension_script(), and I think it's more than worth
that given the evident scope of the problem.

+1.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Oct 12, 2011 at 17:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The only thing the \echo approach will cost us is a few more lines of C
code in execute_extension_script(), and I think it's more than worth
that given the evident scope of the problem.

+1.

PFA, a sample patch for this --- I've only bothered to change one script
file here, but will of course do the rest if there are no further
objections. The technique actually works even better than I expected,
because of the seemingly nowhere documented fact that \quit in a script
file doesn't terminate psql, only processing of the script. So what
I get is

regression=# \i ~/postgres/share/extension/cube--1.0.sql
Use "CREATE EXTENSION cube" to load this file.
regression=#

which is about as good as one could hope for.

(Looks like a patch to the psql docs is upcoming, too.)

regards, tom lane

On Wed, Oct 12, 2011 at 19:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Oct 12, 2011 at 17:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The only thing the \echo approach will cost us is a few more lines of C
code in execute_extension_script(), and I think it's more than worth
that given the evident scope of the problem.

+1.

PFA, a sample patch for this --- I've only bothered to change one script
file here, but will of course do the rest if there are no further
objections.  The technique actually works even better than I expected,
because of the seemingly nowhere documented fact that \quit in a script
file doesn't terminate psql, only processing of the script.  So what
I get is

regression=# \i ~/postgres/share/extension/cube--1.0.sql
Use "CREATE EXTENSION cube" to load this file.
regression=#

which is about as good as one could hope for.

Looks great to me.

I guess the failure scenario is if someone has an extension from 9.1.2
and tries to load it into 9.1.1 or earlier, in which case they will
get a syntax error or somehing when trying to run the CREATE EXTENSION
command, right? I doubt that's something worth dealing with - it's a
lot less likely to happen.

We might want to document this for other third-party extension
developers to use as a trick as well?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Oct 12, 2011 at 19:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:

PFA, a sample patch for this --- I've only bothered to change one script
file here, but will of course do the rest if there are no further
objections.

I guess the failure scenario is if someone has an extension from 9.1.2
and tries to load it into 9.1.1 or earlier, in which case they will
get a syntax error or somehing when trying to run the CREATE EXTENSION
command, right? I doubt that's something worth dealing with - it's a
lot less likely to happen.

Hmm, yeah, you're right. But it doesn't seem like a big problem to me,
certainly not as big as the problem we're trying to solve.

We might want to document this for other third-party extension
developers to use as a trick as well?

Yes, I will add something to the docs.

regards, tom lane

On 10/12/2011 02:21 PM, Magnus Hagander wrote:

On Wed, Oct 12, 2011 at 19:36, Tom Lane<tgl@sss.pgh.pa.us> wrote:

regression=# \i ~/postgres/share/extension/cube--1.0.sql
Use "CREATE EXTENSION cube" to load this file.
regression=#

which is about as good as one could hope for.

Looks great to me.

Yes, me too.

I guess the failure scenario is if someone has an extension from 9.1.2
and tries to load it into 9.1.1 or earlier, in which case they will
get a syntax error or somehing when trying to run the CREATE EXTENSION
command, right? I doubt that's something worth dealing with - it's a
lot less likely to happen.

As long as we are going to apply it for 9.1 and not wait for 9.2 I don't
think there will be much problem. I think this is one of the rare cases
where we should apply a change to the stable release.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

On 10/12/2011 02:21 PM, Magnus Hagander wrote:

I guess the failure scenario is if someone has an extension from 9.1.2
and tries to load it into 9.1.1 or earlier, in which case they will
get a syntax error or somehing when trying to run the CREATE EXTENSION
command, right? I doubt that's something worth dealing with - it's a
lot less likely to happen.

As long as we are going to apply it for 9.1 and not wait for 9.2 I don't
think there will be much problem. I think this is one of the rare cases
where we should apply a change to the stable release.

By 9.2 doing this would be rather pointless, likely. Also, the earlier
we get it in the easier it will be for third-party devs to rely on it
working.

regards, tom lane

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

regression=# \i ~/postgres/share/extension/cube--1.0.sql
Use "CREATE EXTENSION cube" to load this file.
regression=#

Great work, thank you!
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support