Re: Review: create extension default_full_version

On Mon, Dec 3, 2012 at 11:05 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>wrote:

> Hi,
>
> Thanks for your very good review!
>
> Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)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