Review: create extension default_full_version

Hi,

I looked at the discussion for this patch and the patch itself. Here
are my comments and observations about the patch.
What I got from the discussion is that the patch tries to implement a
mechanism to install extension from series of SQL scripts from
base/full version e.g. if a user wants to create an extension "1.1",
system should run v1.0 script followed by 1.0--1.1 script. In that
case we need to know about the base or full version which in the above
case is v1.0. So the patch added a defualt_full_version option in
extension control file.

Here are my comments about the patch

* Note: Patch does not apply cleanly on latest code base. You probably
need to re-base the code

ibrar(at)ibrar-laptop:~/work/postgresql$ patch -p1
<extension-default-full-version.v0.patch
patching file contrib/hstore/Makefile
Hunk #1 FAILED at 5.
1 out of 1 hunk FAILED -- saving rejects to file contrib/hstore/Makefile.rej
patching file contrib/hstore/hstore--1.1.sql
patching file contrib/hstore/hstore.control
patching file src/backend/commands/extension.c
Hunk #1 succeeded at 68 (offset 2 lines).
Hunk #2 succeeded at 508 (offset 2 lines).
Hunk #3 succeeded at 1295 (offset 2 lines).
Hunk #4 succeeded at 1316 (offset 2 lines).
Hunk #5 succeeded at 1473 (offset 3 lines).

* This is a user visible change so documentation change is required here.

* Also, You need to update the comment, because this code is now
handling default_full_version as well.

/*
* Determine the (unpackaged) version to update from, if any, and then
* figure out what sequence of update scripts we need to apply.
*/
if ((d_old_version && d_old_version->arg) || pcontrol->default_full_version)

* In case the "default_full_version" and VERSION in SQL are same then
we are getting a misleading error message i.e.

comment = 'data type for storing sets of (key, value) pairs'
default_version = '1.1'
default_full_version = '1.0'
module_pathname = '$libdir/hstore'
relocatable = true

postgres=# create EXTENSION hstore version '1.0';
ERROR: FROM version must be different from installation target version "1.0"

Error message is complaining about "FROM" clause which is not used in
the query. I think EXTENSION creation should not be blocked in case
"default_full_version" and VERSION are same. But if we want to block
that; error message should be meaningful.

* I noticed another issue with the patch.

In case we do not specify the default_full_version in control file
then this is the sequence of sql scripts.

postgres=# CREATE EXTENSION hstore version '1.3' from '1.0';
WARNING: /usr/local/pgsql/share/extension/hstore--1.0--1.1.sql
WARNING: /usr/local/pgsql/share/extension/hstore--1.1--1.2.sql
WARNING: /usr/local/pgsql/share/extension/hstore--1.2--1.3.sql
CREATE EXTENSION

But in case default_full_version = 1.0, then we are getting "ERROR:
could not stat file..." error message.

postgres=# CREATE EXTENSION hstore version '1.3' from '1.0';
WARNING: SCRIPT = /usr/local/pgsql/share/extension/hstore--1.0.sql
WARNING: SCRIPT =
/usr/local/pgsql/share/extension/hstore--1.0--1.2.sql
<<--- Why not 1.0--1.1
ERROR: could not stat file
"/usr/local/pgsql/share/extension/hstore--1.0--1.2.sql": No such file
or directory

This is because of missing version number i.e. first we're executing
1.0 followed by 1.0--1.2 not 1.0--1.1 but I think following should be
the right sequence

postgres=# CREATE EXTENSION hstore version '1.3' from '1.0';
WARNING: /usr/local/pgsql/share/extension/hstore--1.0.sql
WARNING: /usr/local/pgsql/share/extension/hstore--1.0--1.1.sql
WARNING: /usr/local/pgsql/share/extension/hstore--1.1--1.2.sql
WARNING: /usr/local/pgsql/share/extension/hstore--1.2--1.3.sql
CREATE EXTENSION

PS: I modified the code to get this result.

- IMHO there should be an SQL option along with the default_full_version; like.

postgres=# create EXTENSION hstore VERSION '1.1' FULL_VERSION '1.0';

- hstore regression is also failing.

-----------

Ibrar Ahmed