Re: Extension Templates S03E11

Hi,

Boszormenyi Zoltan <zb@cybertec.at> writes:
> Here's a review for this patch.

Thanks for that review Zoltan!

> No, it has one reject in src/bin/pg_dump/pg_backup_archiver.c.
> It was obvious to fix, version 12a is attached.

Included in the new version of the patch (v13), attached.

> It has extended the SQL reference nicely but the reference to
> <xref linkend="extend-extensions"> was not obvious enough
> regarding the list of control parameters.

Fixed in the attached.

> I would like to see the control parameters documented in allcaps
> in CREATE EXTENSION TEMPLATE. Then ALTER EXTENSION
> TEMPLATE should reference the CREATE instead of the longer
> text in <xref linkend="extend-extensions">. This xref can still
> be there for reference in all three of CREATE/ALTER/DROP
> EXTENSION TEMPLATE.

I didn't follow exactly your proposal here because I didn't want to have
to maintain the control parameter description list in two different
places. I've still added a detailed list with references and details and
more importantly with coverage of e.g. "NORELOCATABLE" which is not
covered in the referenced material.

> But the version check is already wrong in src/bin/pg_dump/pg_dump.c
> since this patch missed 9.3.
>
> +       if (fout->remoteVersion < 90300)

Fixed.

> Nitpicking. This chunk has an extra unnecessary space:

Fixed.

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