From: | Sergei Kornilov <sk(at)zsrv(dot)org> |
---|---|
To: | Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> |
Cc: | PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Continue work on changes to recovery.conf API |
Date: | 2018-11-13 14:57:10 |
Message-ID: | 22367601542121030@iva5-d3020dc3459d.qloud-c.yandex.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello
Thank you! Here is patch update addressing your comments.
> - useless whitespace change in xlog.c
Oops, did not notice. Fixed.
> - 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().
Agreed, done.
> - Why introduce MAXRESTOREPOINTNAMELEN? If you think this is useful,
> then we could do it as a separate patch.
Reverted back. This was changed in prev proposal.
> - Make the help text change in pg_archivecleanup.c similar to pg_standby.c.
Changed.
> - In pg_basebackup.c, duplicate defines PG_AUTOCONF_FILENAME and
> STANDBY_SIGNAL_FILE. See that you can put those into a header file
> somewhere.
I move constants from xlog.h to xlog_internal.h. Also i revert back RECOVERY_COMMAND_FILE and RECOVERY_COMMAND_DONE into xlog.c (was moved to xlog.h few weeks ago)
But i have no good idea for PG_AUTOCONF_FILENAME. Seems most src/bin/ application uses hardcoded file path. How about miscadmin.h?
> - 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.
Maybe leave just "write configuration for replication" with explanation in docs, as was before?
> - The test function $node_standby->request_standby_mode() could use a
> better name. How about set_ instead of request_ ?
Sounds good, changed.
> - 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.
Fixed
> - 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.
Agreed, revert back.
regards, Sergei
Attachment | Content-Type | Size |
---|---|---|
new_recovery_api_v006.patch | text/x-diff | 98.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Jesper Pedersen | 2018-11-13 15:32:33 | Re: Speeding up INSERTs and UPDATEs to partitioned tables |
Previous Message | Alvaro Herrera | 2018-11-13 13:59:15 | Re: move PartitionBoundInfo creation code |