Re: in-catalog Extension Scripts and Control parameters (templates?) - Mailing list pgsql-hackers

Hi,

Please find attached version 8 of the patch, with fixes for almost all
reported problems. Thanks a lot to you reviewers for finding them!

I need some help with:

  - toast tables for new catalog tables
  - extension.c:1150:25: warning: variable ‘evi’ set but not used

See details below.

Hitoshi Harada <umi.tanuki@gmail.com> writes:
> - Why do we need with() clause even if I don't need it?

Not needed anymore, regression test addded to cover the new grammar form.

> foo=# create template for extension ex2 version '1.0' with (requires ex1)
> as $$ $$;
> ERROR:  template option "requires" takes an argument

Not sure how to handle the grammar for that case, given that I'm using
the same tricks as in the CREATE ROLE options in order to avoid adding
new keywords in the patch.

> - create template ex2, create extension ex2, alter template ex2 rename to
> ex3, create extension ex3, drop template ex3;
> foo=# drop template for extension ex3 version '1.0';
> ERROR:  cannot drop unrecognized object 3179 16429 0 because other objects
> depend on it

Fixed in the attached.

> - a template that is created in another template script does not appear to
> depend on the parent template.

What I understand you meant is when doing CREATE TEMPLATE FOR EXTENSION
from within a CREATE EXTENSION script. That is now covered in the
attached.

> - Without control file/template, attempt to create a new extension gives:
> foo=# create extension plv8;
> ERROR:  extension "plv8" has no default control template
> but can it be better, like "extension plv8 has no default control file or
> template"?

Updated the error message, it now looks like that:

    create extension plv8;
    ERROR:  42704: Extension "plv8" is not available from "/Users/dim/pgsql/ddl/share/extension" nor as a template
    LOCATION:  read_extension_control, extension.c:676
    STATEMENT:  create extension plv8;

> - Looks like both tables don't have toast tables.  Some experiment gives:
> ERROR:  row is too big: size 8472, maximum size 8160

Not sure why. That's not fixed in the attached.

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Very minor comment here: these SGML "id" tags:
>
> + <refentry id="SQL-ALTEREXTENSIONTEMPLATE">

Changed to SQL-ALTER-TEMPLATE-FOR-EXTENSION, same with CREATE and DROP
commands, update the cross references.

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> What if read_extension_control_file() fails because of an out-of-memory
> error?  I think you need to extend that function to have a more useful
> API, not rely on it raising a specific error.  There is at least one
> more case when you're calling that function in the same way.

Good point. I'm now using something really simple:

    if (access(get_extension_control_filename(extname), F_OK) == 0)
    {
        ereport(ERROR,
                (errcode(ERRCODE_DUPLICATE_OBJECT),
                 errmsg("extension \"%s\" is already available", extname)));
    }

After pondering about it for a while, that doesn't strike me as a
modularity violation severe enough to warrant more complex changes.

Jaime Casanova <jaime@2ndquadrant.com> writes:
> - The error message in aclchk.c:5175 isn't very clear, i mean the user
> should  see something better than "uptmpl"

Fixed in the attached.

> - In alter.c you made AlterObjectRename_internal non static and
> replaced a SearchSysCache1 call with a get_catalog_object_by_oid one.
> Now, in its comment that function says that is for simple cases. And
> because of the things you're doing it seems to me this isn't a simple
> case. Maybe instead of modifying it you should create other function
> RenameExtensionTemplateInternal, just like tablecmd.c does?

The get_catalog_object_by_oid() is doing a SearchSysCache1 when that's
relevant and possible, so I don't think I changed enough things around
to warrant a different API. I'm open to hearing about why I'm wrong if
that's the case, though.

> - This is a typo i guess: AtlerExtensionTemplateRename

Fixed in the attached.

> - In event_triggers.c, it seems the intention was to keep the
> event_trigger_support array in order, any reason to for not doing it?

Fixed in the attached.

> - extension.c: In function ‘get_ext_ver_list_from_catalog’:
> extension.c:1150:25: warning: variable ‘evi’ set but not used
> [-Wunused-but-set-variable]

I don't have the warning here, and that code is unchanged from master's
branch, only the name of the function did change. Do you have the same
warning with master? which version of gcc are you using?

> Actually, what this did was to create an 123 schema and it puts the
> functions there.

There was a fault in the way default values are assigned to auxilliary
control entries in pg_extension_control when creating a template for
updating an extension. That's been fixed in the attached, a a new
regression test has been added.

> In this case only f1() and f3() exists, because the extension is going
> from 1.0 to 2.1. is this expected?

You can use the following SQL statement to debug your upgrade paths, as
you could already with file-based control information. This tells me
that yes it's expected.

    select * from pg_extension_update_paths('test2');
     source | target |   path
    --------+--------+----------
     1.0    | 1.1    | 1.0--1.1
     1.0    | 2.1    | 1.0--2.1
     1.1    | 1.0    |
     1.1    | 2.1    |
     2.1    | 1.0    |
     2.1    | 1.1    |
    (6 rows)

You have to remember that the shortest path only will get used to
upgrade your extension.

> and, yes... the functions are in the schema "2.1"

Fixed in the attached.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support