Review: create extension default_full_version - Mailing list pgsql-hackers

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@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