Re: Return pg_control from pg_backup_stop().

On 10/2/24 10:11, Michael Paquier wrote:
> On Fri, May 17, 2024 at 12:46:49PM +1000, David Steele wrote:
>
>> The basic idea is to harden recovery by returning a copy of pg_control from
>> pg_backup_stop() that has a flag set to prevent recovery if the backup_label
>> file is missing. Instead of backup software copying pg_control from PGDATA,
>> it stores an updated version that is returned from pg_backup_stop().
>
> Hmm, okay. There is also a slight impact for BASE_BACKUP, requiring
> basebackup_progress_wait_wal_archive() and do_pg_backup_stop() to be
> called earlier when sending the main data directory which is the last
> one in the list of tablespaces. As far as I can see, this does not
> change the logic because do_pg_backup_stop() does not touch the
> control file, but it seems to me that we'd rather keep these two
> calls as they are now, and send the control file once we are out of
> the for loop that processes all the tablespaces. That seems slightly
> cleaner to me, still I agree that both are the same things.

Sending pg_control later results in even more code churn because of how
tar files are terminated. I've updated it that way in v2 so you can see
what I mean. I don't have a strong preference, though, so if you prefer
the implementation in v2 then that's fine with me.

> Anyway, finishing do_pg_backup_stop() and then sending the control
> file is just a consequence of the implementation choice driven by the
> output required for the SQL function so as this is stored in the
> backup state to get it down to pg_backup_stop() in xlogfuncs.c, so we
> could also take one step back and forget about the SQL function,
> setting only the new flag when sending a BASE_BACKUP, or just not use
> the backupState to store this data as the exercise involves forcing
> one boolean and recalculate a CRC32.

I can definitely see us making other updates to pg_control so I would
rather keep this logic centralized, even though it is not too
complicated at this point. Still, even 8 lines of code (as it is now)
seems better not to duplicate.

>> * We don't need to worry about backup software seeing a torn copy of
>> pg_control, since Postgres can safely read it out of memory and provide a
>> valid copy via pg_backup_stop(). This solves torn reads without needing to
>> write pg_control via a temp file, which may affect performance on a standby.
>
> We're talking about a 8kB file which has a size of 512B
> (PG_CONTROL_MAX_SAFE_SIZE) to avoid such issues. So I'm not sure to
> see your point here?

Even at 512B it is possible to see tears in pg_control and they happen
in the build farm right now. In fact, this thread [1] trying to fix the
problem was what got me thinking about alternate solutions to preventing
tears in pg_control. Thomas' proposed fixes have not been committed to
my knowledge so the problem remains, but would be fixed by this commit.

> There is a large comment block in do_pg_backup_stop() around
> backup_stopped_in_recovery. Perhaps it should be improved based on
> this patch.

I added a sentence to this comment block in v2.

> The main concern that I have over this patch is: who is actually going
> to use this extension of the SQL stop function?

Primarily existing backup software, I would imagine. The idea is that it
would give them feature parity with pg_basebackup, rather than the new
protections being exclusive to pg_basebackup.

> Perhaps existing
> backup solutions are good enough risk vs reward is not worth it?

I'm not sure I see the risk here. Saving out pg_control is optional so
no changes to current software is required. Of course they miss the
benefit of the protection against tears and missing backup_label, but
that is a choice.

Also, no matter what current backup solutions do, they cannot prevent a
user from removing backup_label after restore. This patch prevents
invalid recovery when that happens.

> The
> label_file and the tablespace map are text, this is a series of bytes
> that has no visibility for the end-user unless checked on the
> client-side. This adds a new step where backups would need to copy
> the control file to the data folder.

Again, optional, but if I was able to manage these saves using the psql
interface in the TAP tests then I'd say it would be pretty easy for
anyone with a normal connection to Postgres. Also, we require users to
treat tabelspace_map and backup_label as binary so not too big a change
here.

[1]
https://www.postgresql.org/message-id/CA%2BhUKG%2Bjig%2BQdBETj_ab%2B%2BVWSoJjbwT3sLNCnk0wFsY_6tRqoA%40mail.gmail.com