On 30/10/2018 14:30, Sergei Kornilov wrote:
> I attached new version of this patch due merge conflict with pg_promote function.
This patch looks pretty good to me functionality-wise. There are some
code details to work through, listed below.
In this review, I'm skipping over your documentation changes. It seems
you have found all the places that mention recovery.conf and updated
them adequately. But I think in some cases we will need to take a few
steps back and reword a paragraph or section altogether. For example,
there will no longer be a reason for recovery-config.sgml to be a
separate chapter if it's all part of the main GUC system. We can work
through that later.
Code discussion:
- useless whitespace change in xlog.c
- I think we can drop logRecoveryParameters(). Users can now inspect
those parameters using the normal GUC mechanisms. (They were all DEBUG2
anyway, so it's not like many users will be missing this.) Then you can
also drop RecoveryTargetActionText().
- Why introduce MAXRESTOREPOINTNAMELEN? If you think this is useful,
then we could do it as a separate patch.
- Make the help text change in pg_archivecleanup.c similar to pg_standby.c.
- In pg_basebackup.c, duplicate defines PG_AUTOCONF_FILENAME and
STANDBY_SIGNAL_FILE. See that you can put those into a header file
somewhere.
- This stuff breaks translation strings:
printf(_(" -R, --write-recovery-conf\n"
- " write recovery.conf for
replication\n"));
+ " append replication config to "
PG_AUTOCONF_FILENAME "\n"
+ " and place " STANDBY_SIGNAL_FILE "
file\n"));
Use %s placeholders, or better yet replace it in a more compact way.
- The test function $node_standby->request_standby_mode() could use a
better name. How about set_ instead of request_ ?
- New string GUCs should have "" instead of NULL as their default in
guc.c. (Not sure whether that is strictly necessary, but it seems good
to be consistent.)
- Help strings in guc.c should end with a period.
- Renaming the promote and fallback_promote files seems unnecessary for
this patch, and I would take it out. Otherwise, the change in pg_ctl.c
is out of date with the comment above it.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
