Re: Add checks in pg_rewind to abort if backup_label file is present

On 05/12/2023 19:36, Krishnakumar R wrote:
> Hi,
>
> Please find the attached patch for $subject and associated test. Please review.

Thanks for picking up this long-standing TODO!

> +/*
> + * Check if a file is present using the connection to the
> + * database.
> + */
> +static bool
> +libpq_is_file_present(rewind_source *source, const char *path)
> +{
> + PGconn *conn = ((libpq_source *) source)->conn;
> + PGresult *res;
> + const char *paramValues[1];
> +
> + paramValues[0] = path;
> + res = PQexecParams(conn, "SELECT pg_stat_file($1)",
> + 1, NULL, paramValues, NULL, NULL, 1);
> + if (PQresultStatus(res) != PGRES_TUPLES_OK)
> + return false;
> +
> + return true;
> +}

The backup_label file cannot be present when the server is running. No
need to check for that when connected to a live server.

> --- a/src/bin/pg_rewind/pg_rewind.c
> +++ b/src/bin/pg_rewind/pg_rewind.c
> @@ -729,7 +729,11 @@ perform_rewind(filemap_t *filemap, rewind_source *source,
> static void
> sanityChecks(void)
> {
> - /* TODO Check that there's no backup_label in either cluster */
> + if (source->is_file_present(source, "backup_label"))
> + pg_fatal("The backup_label file is present in source cluster");
> +
> + if (is_file_present(datadir_target, "backup_label"))
> + pg_fatal("The backup_label file is present in target cluster");
>
> /* Check system_identifier match */
> if (ControlFile_target.system_identifier != ControlFile_source.system_identifier)

The error message isn't very user friendly. It's pretty dangerous
actually: I think a lot of users would just delete the backup_label file
when they see that message. Because then the file is no longer present
and problem solved, right?

The point of checking for backup_label is that if it's present, the
cluster wasn't really shut down cleanly. The correct fix is to start it,
let WAL recovery finish, and shut it down again. The error message
should make that clear. Perhaps make it similar to the existing "target
server must be shut down cleanly" message.

I think today if you try to run pg_rewind on a cluster that was restored
from a backup, so that backup_label is present, you get the "target
server must be shut down cleanly" message. But we don't have any tests
for it. We do have a test for when the server is still running, but not
for the restored-from-backup case. Would be nice to add one.

Perhaps move the backup_label check later in sanityChecks(), after
checking the state in the control file. That way, you still normally hit
the "target server must be shut down cleanly" case, and the backup_label
check would be just an additional "can't happen" sanity check.

In createBackupLabel() we have this:

> /* TODO: move old file out of the way, if any. */
> open_target_file("backup_label", true); /* BACKUP_LABEL_FILE */
> write_target_range(buf, 0, len);
> close_target_file();

That TODO comment needs to go away now. And we probably should complain
if the file already exists. With your patch, we already checked earlier
that it doesn't exists, so if it exists when we reach that code,
something's gone wrong.

--
Heikki Linnakangas
Neon (https://neon.tech)