Extensions, patch v18 (merge against master, bitrot-only-fixes)

On Thu, Dec 16, 2010 at 7:49 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Please note that the SQL scripts seem to be encoded in latin9.

Seems like an odd choice. Why not UTF-8?

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

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Dec 16, 2010 at 7:49 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Please note that the SQL scripts seem to be encoded in latin9.

Seems like an odd choice. Why not UTF-8?

Not a choice, just what's already in…

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

On Thu, Dec 16, 2010 at 9:04 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Dec 16, 2010 at 7:49 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Please note that the SQL scripts seem to be encoded in latin9.

Seems like an odd choice.  Why not UTF-8?

Not a choice, just what's already in…

Sure, I get it. I'm guessing that many of the scripts will work in a
wide variety of encodings because they're a subset of ASCII. Should
we think about converting the others to UTF-8, or is that a bad idea?

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

2010/12/16 Robert Haas <robertmhaas@gmail.com>:

On Thu, Dec 16, 2010 at 7:49 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

Please note that the SQL scripts seem to be encoded in latin9.

Seems like an odd choice.  Why not UTF-8?

Latin 9 = ISO 8859-15 = a more modern version of Latin 1 (ISO 8859-1),
which includes the € symbol for example. I.e., it's not that weird. As
long as there are no non-ASCII characters, it seems even likely that
some tools might detect a simple ASCII text file as Latin 9 by
default.

Nicolas

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Dec 16, 2010 at 9:04 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Please note that the SQL scripts seem to be encoded in latin9.

Seems like an odd choice. �Why not UTF-8?

Not a choice, just what's already in�

Sure, I get it. I'm guessing that many of the scripts will work in a
wide variety of encodings because they're a subset of ASCII. Should
we think about converting the others to UTF-8, or is that a bad idea?

I would think that we want to establish the same policy as we have for
dictionary files: they're assumed to be UTF-8. I don't believe there
should be an encoding option at all. If we didn't need one for
dictionary files, there is *surely* no reason why we have to have one
for extension SQL files.

regards, tom lane

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

I would think that we want to establish the same policy as we have for
dictionary files: they're assumed to be UTF-8. I don't believe there
should be an encoding option at all. If we didn't need one for
dictionary files, there is *surely* no reason why we have to have one
for extension SQL files.

Brain-fart from me in the v18, that I've produced while being pressed by
other distractions. Fixed now in the attached, v19. Sorry about that, I
was too eager to produce the no-moving-parts "final" patch. Time to find
this quote about parallelism and time lost I guess.

So, attached patch fixes the v18 regression wrt to script file encoding
and establish UTF-8 as the default encoding to consider to read a script
file. Thanks for your comments.

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

On Dec 16, 2010, at 8:19 AM, Tom Lane wrote:

I would think that we want to establish the same policy as we have for
dictionary files: they're assumed to be UTF-8. I don't believe there
should be an encoding option at all. If we didn't need one for
dictionary files, there is *surely* no reason why we have to have one
for extension SQL files.

+1 KISS.

David

Excerpts from Dimitri Fontaine's message of jue dic 16 09:49:31 -0300 2010:

Hi,

Well $subject says about it all really. The bitrot of course comes from
the fact that the last in-commitfest-dependency has been commited in,
and I kept a version of pg_execute_sql_file() in the extension's patch.

I thought the suggestion of having "version = 9.1devel" line in each
contrib's module extension file was a joke. It is certainly not going
to be sustainable in the long run -- I don't think we want to be
modifying all control files each release. We need to find a better way
to fix this.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:

I thought the suggestion of having "version = 9.1devel" line in each
contrib's module extension file was a joke. It is certainly not going
to be sustainable in the long run -- I don't think we want to be
modifying all control files each release. We need to find a better way
to fix this.

+1.

Naively enough, getting this from the Makefile looked obvious to me.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:

I thought the suggestion of having "version = 9.1devel" line in each
contrib's module extension file was a joke. It is certainly not going
to be sustainable in the long run -- I don't think we want to be
modifying all control files each release. We need to find a better way
to fix this.

Consider also the use case where we only update contrib module minor
version when they do receive an update in the tree. If we get to do
that, maybe it's good enough with plain .control files.

Of course having some Makefile or other support doesn't prevent anything
here, it just allows some more flexibility. That was considered some
more complexity not solving any real world use-case on-list, though.

FWIW, it seems very likely that should we ship without the Make support
I'll have to add it to every extension I maintain, separately.

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

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

Alvaro Herrera <alvherre@commandprompt.com> writes:

I thought the suggestion of having "version = 9.1devel" line in each
contrib's module extension file was a joke. It is certainly not going
to be sustainable in the long run -- I don't think we want to be
modifying all control files each release. We need to find a better way
to fix this.

Naively enough, getting this from the Makefile looked obvious to me.

Putting those numbers in the Makefile instead of the control file surely
does nothing to alleviate Alvaro's maintenance concern.

However, the only way I can see to fix this "automatically" is to have
the makefiles propagate PG_VERSION_NUM (or one of the other values set
by configure) into generated control files. I don't think that's what
we want either. If we do that, then people are going to be forced to
go through an ALTER EXTENSION UPGRADE cycle whether or not anything
actually changed in the extension's SQL definitions. We really only
want the extension's SQL version to change when there was a meaningful
change in the SQL commands.

I'm not sure that I see a better answer than hand-maintained version
numbers in each extension SQL file. But if that's where we're going,
they should be in the SQL files, not in either the Makefiles or control
files.

regards, tom lane

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

However, the only way I can see to fix this "automatically" is to have
the makefiles propagate PG_VERSION_NUM (or one of the other values set
by configure) into generated control files.

Ah, somewhat like what I was asked to remove from the patch, right?

-EXTVERSION = $(VERSION)

I don't think that's what
we want either. If we do that, then people are going to be forced to
go through an ALTER EXTENSION UPGRADE cycle whether or not anything
actually changed in the extension's SQL definitions. We really only
want the extension's SQL version to change when there was a meaningful
change in the SQL commands.

Well that's just a facility, and very useful for people that have a Make
variable where the extension's version is already held. This problem of
having a VERSION that moves when the extension possibly didn't change
seems pretty specific to PostgreSQL, not typical for extensions authors.

I'm not sure that I see a better answer than hand-maintained version
numbers in each extension SQL file. But if that's where we're going,
they should be in the SQL files, not in either the Makefiles or control
files.

You lost me. Are you wanting extension authors to maintain meta data in
the SQL script, with some new syntax support, or maybe from calling a
function? How do you think this will play with upgrading?

That doesn't sound any simpler to me. The whole goal of the control file
is to be able to register the extension in the catalog and get an OID
there to manage the dependencies (so that we get a single command per
extension in pg_dump output). The first design proposed an SQL command
to fill in the catalog, but that command must be separate from what the
DBA will have to type in to have the server execute the script, so this
idea has been beaten by the control file idea (thanks goes to Heikki).

Now, we could go with the current patch for starters and add some
facilities around 9.2 when we have experienced the maintenance
burden. And extension authors will probably be asking for some more
facilities too, by then.

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

Excerpts from Tom Lane's message of jue dic 16 17:10:10 -0300 2010:

However, the only way I can see to fix this "automatically" is to have
the makefiles propagate PG_VERSION_NUM (or one of the other values set
by configure) into generated control files. I don't think that's what
we want either. If we do that, then people are going to be forced to
go through an ALTER EXTENSION UPGRADE cycle whether or not anything
actually changed in the extension's SQL definitions. We really only
want the extension's SQL version to change when there was a meaningful
change in the SQL commands.

I've been wondering if we should stop thinking that each contrib's
module version is the same as the backend version. What if we declare
them all 1.0.0 for the 9.1 release, and increment the version manually
each time there's a change? So a module that stays unchanged for 9.2
would still be 1.0.0. (The .so file would still need to be recompiled
for the new server version, because of PG_MODULE_MAGIC.)

In that case, having hand-maintained version numbers in control or
wherever is not so much of a problem; the committer that touches the
module needs to ensure that the version number is incremented too.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Tom Lane's message of jue dic 16 17:10:10 -0300 2010:

However, the only way I can see to fix this "automatically" is to have
the makefiles propagate PG_VERSION_NUM (or one of the other values set
by configure) into generated control files. I don't think that's what
we want either. If we do that, then people are going to be forced to
go through an ALTER EXTENSION UPGRADE cycle whether or not anything
actually changed in the extension's SQL definitions. We really only
want the extension's SQL version to change when there was a meaningful
change in the SQL commands.

I've been wondering if we should stop thinking that each contrib's
module version is the same as the backend version.

Right, they would have to be decoupled if you believe what I said above.

In that case, having hand-maintained version numbers in control or
wherever is not so much of a problem; the committer that touches the
module needs to ensure that the version number is incremented too.

It would be tolerable, if perhaps error-prone. But we've seldom blown
it on catversion, and this would be a comparable requirement.

regards, tom lane

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

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

However, the only way I can see to fix this "automatically" is to have
the makefiles propagate PG_VERSION_NUM (or one of the other values set
by configure) into generated control files.

Ah, somewhat like what I was asked to remove from the patch, right?

I've been pointed off-list that this message ain't conveying the meaning
I'm attaching it, sorry about that. What I mean is that should we change
our opinion again the code to do that has already been written.

Allow me to insist on "we": it's not like I feel forced into changing
the design again and again, I realise I'm acting eagerly upon group
decision making steps before to ensure it's the final step. Well, we all
have been reading over-stressed exchanges on this list before, right? :)

Will now step back on the topic, or try to...

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

On Fri, Dec 17, 2010 at 02:00, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

So, attached patch fixes the v18 regression wrt to script file encoding
and establish UTF-8 as the default encoding to consider to read a script
file. Thanks for your comments.

You probably compared wrong versions of source trees. The patch contains
many diffs not related to EXTENSION. It cannot be applied cleanly.

BTW, only earthdistance.sql.in has @extschema@.
Didn't you forget to remove it?

--- b/contrib/earthdistance/earthdistance.sql.in
! SET search_path = @extschema@;

I've not read the patch well yet, but I'd like to make sure of
two specs in the patch:

#1. I found the patch specifies "version" in each control file. We will
need to maintain them manually, but I assume it was a conclusion in the
discussion for v18 patch. So, no more changes are required here.

--- b/contrib/XXX/XXX.control
+ version = '9.1devel'

#2. The patch replaces "\i XXX.sql" to "CREATE EXTENSION XXX". They are a
bit different because CREATE EXTENSION uses the installed sql files instead
of the source directory. But I think this is the correct behavior. We should
have used only installed files because they are used in "make *installcheck*".

*** a/contrib/XXX/sql/XXX.sql
***************
  SET client_min_messages = warning;
! \set ECHO none
! \i XXX.sql
! \set ECHO all
  RESET client_min_messages;
--- 7,13 ----
  SET client_min_messages = warning;
! CREATE EXTENSION XXX;
  RESET client_min_messages;

--
Itagaki Takahiro

Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:

You probably compared wrong versions of source trees. The patch contains
many diffs not related to EXTENSION. It cannot be applied cleanly.

Ouch, sorry about that, will revise soon'ish. Meanwhile the git repo is
still available here:

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

BTW, only earthdistance.sql.in has @extschema@.
Didn't you forget to remove it?

No. It so happens that earthdistance depends on cube and that we agreed
on not implement dependencies between extensions for the first patch.
The problem here is that ALTER EXTENSION earthdistance SET SCHEMA foo;
would relocate cube objects too, because there's nothing to stop the
recursive walking at the right spot. So earthdistance.control states
that earthdistance is not relocatable for now.

And a non relocatable extension will use @extschema@ so that users still
get a say in where to install it…

I've not read the patch well yet, but I'd like to make sure of
two specs in the patch:

#1. I found the patch specifies "version" in each control file. We will
need to maintain them manually, but I assume it was a conclusion in the
discussion for v18 patch. So, no more changes are required here.

Exactly, the consensus seems to be that we *want* to maintain separate
versions number for each contrib.

#2. The patch replaces "\i XXX.sql" to "CREATE EXTENSION XXX". They are a
bit different because CREATE EXTENSION uses the installed sql files instead
of the source directory. But I think this is the correct behavior. We should
have used only installed files because they are used in "make *installcheck*".

Yeah.

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

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

Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:

You probably compared wrong versions of source trees. The patch contains
many diffs not related to EXTENSION. It cannot be applied cleanly.

Ouch, sorry about that, will revise soon'ish. Meanwhile the git repo is
still available here:

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

Here it is, attached. The problem was of course due to git branches and
missing local pulling and merging. Going gradually from 7 to 2 branches
causes that, sometimes.

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

On Sat, Dec 18, 2010 at 2:39 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Here it is, attached. The problem was of course due to git branches and
missing local pulling and merging. Going gradually from 7 to 2 branches
causes that, sometimes.

I spent a little time looking at this tonight. I'm going to give you
the same general advice that I've given other people who have
submitted very large patches of this type: it'll be a lot easier to
get this committed if we can break it into smaller pieces. A pretty
easy thing to do would be to separate the changes that turn the
existing contrib modules into extensions into its own patch. A
possibly more controversial idea is to actually remove the notion of a
relocatable extension from this patch, and all the supporting
machinery, and make that a follow-on patch. I think it was arranged
like that before and somehow got collapsed, but maybe the notion is
worth revisiting, because this is a lot of code:

224 files changed, 4713 insertions(+), 293 deletions(-)

That issue aside, I think there is still a fair amount of cleanup and
code review that needs to happen here before this can be considered
for commit. Here are a few things I noticed on a first read-through:

- pg_execute_sql_string() and pg_execute_sql_file() are documented,
but are not actually part of the patch.

- The description of the NO USER DATA option is almost content-free.
It basically says "this option is here because it wouldn't work if we
didn't have this option".

- listDependentObjects() appears to be largely cut-and-pasted from
some other function. It calls AcquireDeletionLock() and the comments
mention constructing a list of objects to delete. It sure doesn't
seem right to be acquiring AccessExclusiveLocks on lots of things in a
function that's there to support the pg_extension_objects SRF.

- This bit certainly violates our message style guidelines:

+       elog(NOTICE,
+                "Installing extension '%s' from '%s', in schema %s,
%s user data",
+                stmt->extname,
+                get_extension_absolute_path(control->script),
+                schema ? schema : "public",
+                create_extension_with_user_data ? "with" : "without");

User-facing messages need to be ereport, not elog, so they can be
translated, and mustn't be assembled from pieces, as you've done with
"%s user data".

- Has the issue of changing custom_variable_classes from PGC_SIGHUP to
PGC_SUSET been discussed? I am not sure whether that's an OK thing to
do. If it is OK, then the documentation also needs updating.

- This comment looks like leftovers:

+       /* FIXME: add PGC_EXTENSION so that we don't abuse PGC_SIGHUP here? */
+       SetConfigOption("custom_variable_classes",
+                                       newval, PGC_SIGHUP, PGC_S_EXTENSION);

Apologies if I missed the previous discussion of this, but why are we
adding a new GUC context?

- Did we decide to ditch the encoding parameter for extension scripts
and mandate UTF-8?

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