Re: backup manifests

Hi,

On 2020-03-31 14:10:34 -0400, Robert Haas wrote:
> I made an attempt to implement this.

Awesome!


> In the attached patch set, 0001 I'm going to work on those things. I
> would appreciate *very timely* feedback on anything people do or do
> not like about this, because I want to commit this patch set by the
> end of the work week and that isn't very far away. I would also
> appreciate if people would bear in mind the principle that half a loaf
> is better than none, and further improvements can be made in future
> releases.
> 
> As part of my light testing, I tried promoting a standby that was
> running pg_basebackup, and found that pg_basebackup failed like this:
> 
> pg_basebackup: error: could not get COPY data stream: ERROR:  the
> standby was promoted during online backup
> HINT:  This means that the backup being taken is corrupt and should
> not be used. Try taking another online backup.
> pg_basebackup: removing data directory "/Users/rhaas/pgslave2"
> 
> My first thought was that this error message is hard to reconcile with
> this comment:
> 
>         /*
>          * Send timeline history files too. Only the latest timeline history
>          * file is required for recovery, and even that only if there happens
>          * to be a timeline switch in the first WAL segment that contains the
>          * checkpoint record, or if we're taking a base backup from a standby
>          * server and the target timeline changes while the backup is taken.
>          * But they are small and highly useful for debugging purposes, so
>          * better include them all, always.
>          */
> 
> But then it occurred to me that this might be a cascading standby.

Yea. The check just prevents the walsender's database from being
promoted:

		/*
		 * Check if the postmaster has signaled us to exit, and abort with an
		 * error in that case. The error handler further up will call
		 * do_pg_abort_backup() for us. Also check that if the backup was
		 * started while still in recovery, the server wasn't promoted.
		 * do_pg_stop_backup() will check that too, but it's better to stop
		 * the backup early than continue to the end and fail there.
		 */
		CHECK_FOR_INTERRUPTS();
		if (RecoveryInProgress() != backup_started_in_recovery)
			ereport(ERROR,
					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
					 errmsg("the standby was promoted during online backup"),
					 errhint("This means that the backup being taken is corrupt "
							 "and should not be used. "
							 "Try taking another online backup.")));
and

	if (strcmp(backupfrom, "standby") == 0 && !backup_started_in_recovery)
		ereport(ERROR,
				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
				 errmsg("the standby was promoted during online backup"),
				 errhint("This means that the backup being taken is corrupt "
						 "and should not be used. "
						 "Try taking another online backup.")));

So that just prevents promotions of the current node, afaict.



> Regardless, it seems like a really good idea to store a list of WAL
> ranges rather than a single start/end/timeline, because even if it's
> impossible today it might become possible in the future.

Indeed.


> Still, unless there's an easy way to set up a test scenario where
> multiple WAL ranges need to be verified, it may be hard to test that
> this code actually behaves properly.

I think it'd be possible to test without a fully cascading setup, by
creating an initial base backup, then do some work to create a bunch of
new timelines, and then start the initial base backup. That'd have to
follow all those timelines.  Not sure that's better than a cascading
setup though.


> +/*
> + * Add information about the WAL that will need to be replayed when restoring
> + * this backup to the manifest.
> + */
> +static void
> +AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
> +					 TimeLineID starttli, XLogRecPtr endptr, TimeLineID endtli)
> +{
> +	List *timelines = readTimeLineHistory(endtli);

should probably happen after the manifest->buffile check.


> +	ListCell *lc;
> +	bool	first_wal_range = true;
> +	bool	found_ending_tli = false;
> +
> +	/* If there is no buffile, then the user doesn't want a manifest. */
> +	if (manifest->buffile == NULL)
> +		return;

Not really about this patch/function specifically: I wonder if this'd
look better if you added ManifestEnabled() macro instead of repeating
the comment repeatedly.



> +	/* Unless --no-parse-wal was specified, we will need pg_waldump. */
> +	if (!no_parse_wal)
> +	{
> +		int		ret;
> +
> +		pg_waldump_path = pg_malloc(MAXPGPATH);
> +		ret = find_other_exec(argv[0], "pg_waldump",
> +							  "pg_waldump (PostgreSQL) " PG_VERSION "\n",
> +							 pg_waldump_path);
> +		if (ret < 0)
> +		{
> +			char	full_path[MAXPGPATH];
> +
> +			if (find_my_exec(argv[0], full_path) < 0)
> +				strlcpy(full_path, progname, sizeof(full_path));
> +			if (ret == -1)
> +				pg_log_fatal("The program \"%s\" is needed by %s but was\n"
> +							 "not found in the same directory as \"%s\".\n"
> +							 "Check your installation.",
> +							 "pg_waldump", "pg_validatebackup", full_path);
> +			else
> +				pg_log_fatal("The program \"%s\" was found by \"%s\" but was\n"
> +							 "not the same version as %s.\n"
> +							 "Check your installation.",
> +							 "pg_waldump", full_path, "pg_validatebackup");
> +		}
> +	}

ISTM, and this can definitely wait for another time, that we should have
one wrapper doing all of this, instead of having quite a few copies of
very similar logic to the above.


> +/*
> + * Attempt to parse the WAL files required to restore from backup using
> + * pg_waldump.
> + */
> +static void
> +parse_required_wal(validator_context *context, char *pg_waldump_path,
> +				   char *wal_directory, manifest_wal_range *first_wal_range)
> +{
> +	manifest_wal_range *this_wal_range = first_wal_range;
> +
> +	while (this_wal_range != NULL)
> +	{
> +		char *pg_waldump_cmd;
> +
> +		pg_waldump_cmd = psprintf("\"%s\" --quiet --path=\"%s\" --timeline=%u --start=%X/%X --end=%X/%X\n",
> +			   pg_waldump_path, wal_directory, this_wal_range->tli,
> +			   (uint32) (this_wal_range->start_lsn >> 32),
> +			   (uint32) this_wal_range->start_lsn,
> +			   (uint32) (this_wal_range->end_lsn >> 32),
> +			   (uint32) this_wal_range->end_lsn);
> +		if (system(pg_waldump_cmd) != 0)
> +			report_backup_error(context,
> +								"WAL parsing failed for timeline %u",
> +								this_wal_range->tli);
> +
> +		this_wal_range = this_wal_range->next;
> +	}
> +}

Should we have a function to properly escape paths in cases like this?
Not that it's likely or really problematic, but the quoting for path
could be "circumvented".


Greetings,

Andres Freund