Re: 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

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.

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

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

Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)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

On Tue, Dec 4, 2012 at 7:54 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>wrote:

> Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)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

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(dot)ahmad(at)gmail(dot)com> wrote:

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

Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)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