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
