Re: [HACKERS] Review: create extension default_full_version
Ibrar Ahmed ibrar.ah...@gmail.com writes: * In case we have hstore--1.3.sql file and want to install that file, but failed because of default_full_version. That's now fixed, please see the Extension Templates patch at http://www.postgresql.org/message-id/m21uc8l4j8@2ndquadrant.fr Where you will even find regression tests for that problem. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: create extension default_full_version
Now it works in most of the cases, here is one more point about the patch. * In case we have hstore--1.3.sql file and want to install that file, but failed because of default_full_version. No default_full_version specified --- postgres=# CREATE EXTENSION hstore version '1.3'; CREATE EXTENSION default_full_version = 1.2 postgres=# CREATE EXTENSION hstore version '1.3'; ERROR: could not stat file /usr/local/pgsql/share/extension/hstore--1.2.sql: No such file or directory If we don't want to address this issue at least we should give some meaningful error message. On Tue, Dec 4, 2012 at 4:46 PM, Ibrar Ahmed ibrar.ah...@gmail.com wrote: On Tue, Dec 4, 2012 at 7:54 PM, Dimitri Fontaine dimi...@2ndquadrant.frwrote: Ibrar Ahmed ibrar.ah...@gmail.com writes: I am still getting the same error message. With the attached patch (v2), it works well: create extension hstore version '1.2' from 'unpackaged'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--unpackaged--1.0.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.1--1.2.sql' CREATE EXTENSION You have to remember that the spelling FROM 'unpackaged version' really means that we have previously installed a loose version of the extension (just \i hstore.sql) and want to apply the upgrade path from there. We can't have FROM meaning the same thing as default_full_version. I know. With default_full_version = '1.1' postgres=# CREATE EXTENSION hstore version '1.3' from '1.1'; DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1.sql' DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1--1.3.sql' *ERROR: could not stat file /usr/local/pgsql/share/extension/hstore--1.1--1.3.sql: No such file or directory* That's nonetheless a bug and is fixed now: - if (pcontrol-default_full_version) + if (pcontrol-default_full_version !unpackaged) Thanks, I will look at this again in detail. See attached. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com
Re: [HACKERS] Review: create extension default_full_version
On Mon, Dec 3, 2012 at 11:05 PM, Dimitri Fontaine dimi...@2ndquadrant.frwrote: Hi, Thanks for your very good review! Ibrar Ahmed ibrar.ah...@gmail.com writes: 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. Exactly, that was an idea from Robert and I implemented it quite quickly. Too quickly as we can see from your testing report. 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 Done. The thing is that meanwhile another solution to the main problem has been found: drop support for installing hstore 1.0. Attached patch fixes the problem by reinstalling hstore--1.0.sql and re-enabling this version, and removing the hstore--1.1.sql file now that it's enough to just have hstore--1.0--1.1.sql to install directly (and by default) the newer version. I think we will have to decide about taking only the mechanism or both the mechanism and the actual change for the hstore contrib. * This is a user visible change so documentation change is required here. Added coverage of the new parameter. * 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) Done. I also fixed the bugs you reported here. Here's an edited version of the new (fixed) output: dim=# set client_min_messages to debug1; dim=# create extension hstore version '1.0'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql' WARNING: = is deprecated as an operator name CREATE EXTENSION dim=# create extension hstore version '1.1'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql' WARNING: = is deprecated as an operator name DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' CREATE EXTENSION dim=# create extension hstore; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql' WARNING: = is deprecated as an operator name DETAIL: This name may be disallowed altogether in future versions of PostgreSQL. DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' CREATE EXTENSION 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 I liked your idea of extending the reporting about what files are used, but of course we can't keep that at the WARNING level, so I made that logging DEBUG1 in the attached patch. postgres=# CREATE EXTENSION hstore version '1.3' from '1.0'; Please try that case again, I believe it's fixed in the attached. I am still getting the same error message. Without default_full_version postgres=# CREATE EXTENSION hstore version '1.3' from '1.1'; DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1--1.2.sql' DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.2--1.3.sql' CREATE EXTENSION With default_full_version = '1.1' postgres=# CREATE EXTENSION hstore version '1.3' from '1.1'; DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1.sql' DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1--1.3.sql' *ERROR: could not stat file /usr/local/pgsql/share/extension/hstore--1.1--1.3.sql: No such file or directory* I think there is an issue in this area of the code if (pcontrol-default_full_version) { execute_extension_script(extensionOid, control, -- 1.1.sql NULL, oldVersionName, requiredSchemas, schemaName, schemaOid); ApplyExtensionUpdates(extensionOid, pcontrol, -- 1.1--1.3.sql (wrong) oldVersionName, updateVersions); The first statement is executing 1.1.sql because oldVersionName = 1.1. Keep in mind that versionName = 1.2 and updateVersions list contain only version name 1.3. So in case of default_full_version you are ignoring * versionName* which is *1.2* that
Re: [HACKERS] Review: create extension default_full_version
Ibrar Ahmed ibrar.ah...@gmail.com writes: I am still getting the same error message. With the attached patch (v2), it works well: create extension hstore version '1.2' from 'unpackaged'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--unpackaged--1.0.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.1--1.2.sql' CREATE EXTENSION You have to remember that the spelling FROM 'unpackaged version' really means that we have previously installed a loose version of the extension (just \i hstore.sql) and want to apply the upgrade path from there. We can't have FROM meaning the same thing as default_full_version. With default_full_version = '1.1' postgres=# CREATE EXTENSION hstore version '1.3' from '1.1'; DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1.sql' DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1--1.3.sql' *ERROR: could not stat file /usr/local/pgsql/share/extension/hstore--1.1--1.3.sql: No such file or directory* That's nonetheless a bug and is fixed now: - if (pcontrol-default_full_version) + if (pcontrol-default_full_version !unpackaged) See attached. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/contrib/hstore/Makefile --- b/contrib/hstore/Makefile *** *** 5,11 OBJS = hstore_io.o hstore_op.o hstore_gist.o hstore_gin.o hstore_compat.o \ crc32.o EXTENSION = hstore ! DATA = hstore--1.1.sql hstore--1.0--1.1.sql hstore--unpackaged--1.0.sql REGRESS = hstore --- 5,11 crc32.o EXTENSION = hstore ! DATA = hstore--1.0.sql hstore--1.0--1.1.sql hstore--unpackaged--1.0.sql REGRESS = hstore *** /dev/null --- b/contrib/hstore/hstore--1.0.sql *** *** 0 --- 1,530 + /* contrib/hstore/hstore--1.0.sql */ + + -- complain if script is sourced in psql, rather than via CREATE EXTENSION + \echo Use CREATE EXTENSION hstore to load this file. \quit + + CREATE TYPE hstore; + + CREATE FUNCTION hstore_in(cstring) + RETURNS hstore + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION hstore_out(hstore) + RETURNS cstring + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION hstore_recv(internal) + RETURNS hstore + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION hstore_send(hstore) + RETURNS bytea + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE TYPE hstore ( + INTERNALLENGTH = -1, + INPUT = hstore_in, + OUTPUT = hstore_out, + RECEIVE = hstore_recv, + SEND = hstore_send, + STORAGE = extended + ); + + CREATE FUNCTION hstore_version_diag(hstore) + RETURNS integer + AS 'MODULE_PATHNAME','hstore_version_diag' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION fetchval(hstore,text) + RETURNS text + AS 'MODULE_PATHNAME','hstore_fetchval' + LANGUAGE C STRICT IMMUTABLE; + + CREATE OPERATOR - ( + LEFTARG = hstore, + RIGHTARG = text, + PROCEDURE = fetchval + ); + + CREATE FUNCTION slice_array(hstore,text[]) + RETURNS text[] + AS 'MODULE_PATHNAME','hstore_slice_to_array' + LANGUAGE C STRICT IMMUTABLE; + + CREATE OPERATOR - ( + LEFTARG = hstore, + RIGHTARG = text[], + PROCEDURE = slice_array + ); + + CREATE FUNCTION slice(hstore,text[]) + RETURNS hstore + AS 'MODULE_PATHNAME','hstore_slice_to_hstore' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION isexists(hstore,text) + RETURNS bool + AS 'MODULE_PATHNAME','hstore_exists' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION exist(hstore,text) + RETURNS bool + AS 'MODULE_PATHNAME','hstore_exists' + LANGUAGE C STRICT IMMUTABLE; + + CREATE OPERATOR ? ( + LEFTARG = hstore, + RIGHTARG = text, + PROCEDURE = exist, + RESTRICT = contsel, + JOIN = contjoinsel + ); + + CREATE FUNCTION exists_any(hstore,text[]) + RETURNS bool + AS 'MODULE_PATHNAME','hstore_exists_any' + LANGUAGE C STRICT IMMUTABLE; + + CREATE OPERATOR ?| ( + LEFTARG = hstore, + RIGHTARG = text[], + PROCEDURE = exists_any, + RESTRICT = contsel, + JOIN = contjoinsel + ); + + CREATE FUNCTION exists_all(hstore,text[]) + RETURNS bool + AS 'MODULE_PATHNAME','hstore_exists_all' + LANGUAGE C STRICT IMMUTABLE; + + CREATE OPERATOR ? ( + LEFTARG = hstore, + RIGHTARG = text[], + PROCEDURE = exists_all, + RESTRICT = contsel, + JOIN = contjoinsel + ); + + CREATE FUNCTION isdefined(hstore,text) + RETURNS bool + AS 'MODULE_PATHNAME','hstore_defined' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION defined(hstore,text) + RETURNS bool + AS 'MODULE_PATHNAME','hstore_defined' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION delete(hstore,text) + RETURNS hstore + AS 'MODULE_PATHNAME','hstore_delete' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION
Re: [HACKERS] Review: create extension default_full_version
On Tue, Dec 4, 2012 at 7:54 PM, Dimitri Fontaine dimi...@2ndquadrant.frwrote: Ibrar Ahmed ibrar.ah...@gmail.com writes: I am still getting the same error message. With the attached patch (v2), it works well: create extension hstore version '1.2' from 'unpackaged'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--unpackaged--1.0.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.1--1.2.sql' CREATE EXTENSION You have to remember that the spelling FROM 'unpackaged version' really means that we have previously installed a loose version of the extension (just \i hstore.sql) and want to apply the upgrade path from there. We can't have FROM meaning the same thing as default_full_version. I know. With default_full_version = '1.1' postgres=# CREATE EXTENSION hstore version '1.3' from '1.1'; DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1.sql' DEBUG: execute_extension_script: '/usr/local/pgsql/share/extension/hstore--1.1--1.3.sql' *ERROR: could not stat file /usr/local/pgsql/share/extension/hstore--1.1--1.3.sql: No such file or directory* That's nonetheless a bug and is fixed now: - if (pcontrol-default_full_version) + if (pcontrol-default_full_version !unpackaged) Thanks, I will look at this again in detail. See attached. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com
Re: [HACKERS] Review: create extension default_full_version
Hi, Thanks for your very good review! Ibrar Ahmed ibrar.ah...@gmail.com writes: 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. Exactly, that was an idea from Robert and I implemented it quite quickly. Too quickly as we can see from your testing report. 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 Done. The thing is that meanwhile another solution to the main problem has been found: drop support for installing hstore 1.0. Attached patch fixes the problem by reinstalling hstore--1.0.sql and re-enabling this version, and removing the hstore--1.1.sql file now that it's enough to just have hstore--1.0--1.1.sql to install directly (and by default) the newer version. I think we will have to decide about taking only the mechanism or both the mechanism and the actual change for the hstore contrib. * This is a user visible change so documentation change is required here. Added coverage of the new parameter. * 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) Done. I also fixed the bugs you reported here. Here's an edited version of the new (fixed) output: dim=# set client_min_messages to debug1; dim=# create extension hstore version '1.0'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql' WARNING: = is deprecated as an operator name CREATE EXTENSION dim=# create extension hstore version '1.1'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql' WARNING: = is deprecated as an operator name DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' CREATE EXTENSION dim=# create extension hstore; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql' WARNING: = is deprecated as an operator name DETAIL: This name may be disallowed altogether in future versions of PostgreSQL. DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' CREATE EXTENSION 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 I liked your idea of extending the reporting about what files are used, but of course we can't keep that at the WARNING level, so I made that logging DEBUG1 in the attached patch. postgres=# CREATE EXTENSION hstore version '1.3' from '1.0'; Please try that case again, I believe it's fixed in the attached. - hstore regression is also failing. That's because it doesn't cope anymore with the operator = warning, and I left it this way because we have to decide about shipping hstore 1.0 once we have this patch in. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/contrib/hstore/Makefile --- b/contrib/hstore/Makefile *** *** 5,11 OBJS = hstore_io.o hstore_op.o hstore_gist.o hstore_gin.o hstore_compat.o \ crc32.o EXTENSION = hstore ! DATA = hstore--1.1.sql hstore--1.0--1.1.sql hstore--unpackaged--1.0.sql REGRESS = hstore --- 5,11 crc32.o EXTENSION = hstore ! DATA = hstore--1.0.sql hstore--1.0--1.1.sql hstore--unpackaged--1.0.sql REGRESS = hstore *** /dev/null --- b/contrib/hstore/hstore--1.0.sql *** *** 0 --- 1,530 + /* contrib/hstore/hstore--1.0.sql */ + + -- complain if script is sourced in psql, rather than via CREATE EXTENSION + \echo Use CREATE EXTENSION hstore to load this file. \quit + + CREATE TYPE hstore; + + CREATE FUNCTION hstore_in(cstring) + RETURNS hstore + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION hstore_out(hstore) + RETURNS cstring + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION hstore_recv(internal) + RETURNS hstore + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION hstore_send(hstore) + RETURNS bytea + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE TYPE
[HACKERS] 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@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