Re: Extension Templates S03E11

Hi,

2013-08-04 15:20 keltezéssel, Dimitri Fontaine írta:
> Thom Brown <thom(at)linux(dot)com> writes:
>> Could you please resubmit this without using SnapshotNow as it's no longer
>> supported?
> Sure, sorry that I missed that, please find v12 attached.

Here's a review for this patch.

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

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

* Does it include reasonable tests, necessary doc patches, etc?

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.

The SQL syntax has them in allcaps, while the referenced section
has them in lower case. It's easy to miss them while just browsing
for e.g. RELOCATABLE. I had to go back twice to find the proper
part of the text.

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.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

There's no such provision in the spec.
As far as I can tell, it follows the community-agreed behavior.

* Does it include pg_dump support (if applicable)?

Yes.

But the version check is already wrong in src/bin/pg_dump/pg_dump.c
since this patch missed 9.3.

+ /*
+ * Before 9.3, there are no extension templates.
+ */
+ if (fout->remoteVersion < 90300)
+ {
+ *numExtensionTemplates = 0;
+ return NULL;
+ }
+

* Are there dangers?

I don't think so.

* Have all the bases been covered?

It seems so.

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

I don't know.

* Are there any assertion failures or crashes?

No.

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

n/a

* Does it slow down other things?

No.

* Does it follow the project coding guidelines?

Yes.

Nitpicking. This chunk has an extra unnecessary space:

diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index c4d3f3c..689dc37 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -38,6 +38,7 @@ POSTGRES_BKI_SRCS = $(addprefix $(top_srcdir)/src/include/catalog/,\
pg_authid.h pg_auth_members.h pg_shdepend.h pg_shdescription.h \
pg_ts_config.h pg_ts_config_map.h pg_ts_dict.h \
pg_ts_parser.h pg_ts_template.h pg_extension.h \
+ pg_extension_control.h pg_extension_template.h pg_extension_uptmpl.h \
pg_foreign_data_wrapper.h pg_foreign_server.h pg_user_mapping.h \
pg_foreign_table.h \
pg_default_acl.h pg_seclabel.h pg_shseclabel.h pg_collation.h pg_range.h \

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

According to the added regression tests, yes.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other features/modules?

I think so.

* Are there interdependencies that can cause problems?

I don't know.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/