Re: Review: create extension default_full_version

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