Thread

1. Review: create extension default_full_version

   Ibrar Ahmed <ibrar.ahmad@gmail.com> — 2012-11-30T10:59:58Z

   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
   

2. Re: Review: create extension default_full_version

   Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-12-03T18:05:49Z

   Hi,
   
   Thanks for your very good review!
   
   Ibrar Ahmed <ibrar.ahmad@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
   
   

3. Re: Review: create extension default_full_version

   Ibrar Ahmed <ibrar.ahmad@gmail.com> — 2012-12-04T13:04:04Z

   On Mon, Dec 3, 2012 at 11:05 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr>wrote:
   
   > Hi,
   >
   > Thanks for your very good review!
   >
   > Ibrar Ahmed <ibrar.ahmad@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 is causing this error message
   *
   *
   *ERROR:  could not stat file
   "/usr/local/pgsql/share/extension/hstore--1.1--1.3.sql": No such file or
   directory*
   
   
   
   
   
   
   > > - 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
   >
   >
   
   
   -- 
   Ibrar Ahmed
   EnterpriseDB   http://www.enterprisedb.com
   

4. Re: Review: create extension default_full_version

   Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-12-04T14:54:34Z

   Ibrar Ahmed <ibrar.ahmad@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
   
   

5. Re: Review: create extension default_full_version

   Ibrar Ahmed <ibrar.ahmad@gmail.com> — 2012-12-04T16:46:30Z

   On Tue, Dec 4, 2012 at 7:54 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr>wrote:
   
   > Ibrar Ahmed <ibrar.ahmad@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
   

6. Re: Review: create extension default_full_version

   Ibrar Ahmed <ibrar.ahmad@gmail.com> — 2012-12-11T15:07:18Z

   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.ahmad@gmail.com> wrote:
   
   >
   >
   > On Tue, Dec 4, 2012 at 7:54 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr>wrote:
   >
   >> Ibrar Ahmed <ibrar.ahmad@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
   

7. Re: Review: create extension default_full_version

   Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-02-22T16:14:54Z

   Ibrar Ahmed <ibrar.ahmad@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.fsf@2ndQuadrant.fr
   
   Where you will even find regression tests for that problem.
   
   Regards,
   -- 
   Dimitri Fontaine
   http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support