Thread: Review: create extension default_full_version

Review: create extension default_full_version

From

Ibrar Ahmed

Date:

30 November 2012, 14:00:02

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

Re: Review: create extension default_full_version

From

Dimitri Fontaine

Date:

03 December 2012, 21:06:10

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

Attachment

extension-default-full-version.v1.patch

Re: Review: create extension default_full_version

From

Ibrar Ahmed

Date:

04 December 2012, 16:04:09

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

Re: Review: create extension default_full_version

From

Dimitri Fontaine

Date:

04 December 2012, 17:55:07

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

Attachment

extension-default-full-version.v2.patch

Re: Review: create extension default_full_version

From

Ibrar Ahmed

Date:

04 December 2012, 19:46:35

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

Re: Review: create extension default_full_version

From

Ibrar Ahmed

Date:

11 December 2012, 18:07:24

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

Re: Review: create extension default_full_version

From

Dimitri Fontaine

Date:

22 February 2013, 19:15:15

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