On Fri, Feb 28, 2020 at 03:37:47PM +0300, Alexey Kondratov wrote:
> I would prefer to keep it, since there are plenty of similar comments near
> Asserts and elogs all over the Postgres. Otherwise it may look like a valid
> error state. It may be obvious now, but for someone who is not aware of
> BuildRestoreCommand refactoring it may be not. So from my perspective there
> is nothing bad in this extra one line comment.
elog() is called in the event of errors which should never happen by
definition, so your comment is just a duplication of this state. I'd
still remove that.
> I have added explicit exit(1) calls, since pg_fatal was used only twice in
> the archive.c. Probably, pg_log_fatal from common/logging should obey the
> same logic as FATAL log-level in the backend and do exit the process, but
> for now including pg_rewind.h inside archive.c or vice versa does not look
> like a solution.
archive.c should not depend on anything from src/bin/.
> + * For fixed-size files, the caller may pass the expected size as an
> + * additional crosscheck on successful recovery. If the file size is not
> + * known, set expectedSize = 0.
> + */
> +int
> +RestoreArchivedWALFile(const char *path, const char *xlogfname,
> + off_t expectedSize, const char *restoreCommand)
Actually, expectedSize is IMO a bad idea, because any caller of this
routine passing down zero could be trapped with an incorrect file
size. So let's remove the behavior where it is possible to bypass
this sanity check. We don't need it in pg_rewind either.
> + if ((output_fp = popen(postgres_cmd, "r")) == NULL ||
> + fgets(cmd_output, sizeof(cmd_output), output_fp) == NULL)
> + pg_fatal("could not get restore_command using %s: %s",
> + postgres_cmd, strerror(errno));
Still missing one %m here.
> + /* Remove trailing newline */
> + if (strchr(cmd_output, '\n') != NULL)
> + *strchr(cmd_output, '\n') = '\0';
It seems to me that what you are looking here is to use
pg_strip_crlf(). Thinking harder, we have pipe_read_line() in
src/common/exec.c which does the exact same job..
> - /*
> - * construct the command to be executed
> - */
Perhaps you meant "build" here.
> + if (restore_wal)
> + {
> + int rc;
> + char postgres_exec_path[MAXPGPATH],
> + postgres_cmd[MAXPGPATH],
> + cmd_output[MAXPGPATH];
> + FILE *output_fp;
> +
> + /* Find postgres executable. */
> + rc = find_other_exec(argv[0], "postgres",
> + PG_BACKEND_VERSIONSTR,
> + postgres_exec_path);
For readability, I would move that into its own separate routine.
--
Michael