Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?) - Mailing list pgsql-hackers

On Fri, Sep 16, 2022 at 12:01 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Sep 14, 2022 at 02:24:12PM +0530, Bharath Rupireddy wrote:
> > I'm attaching the v4 patch that's rebased on to the latest HEAD.
> > Please consider this for review.
>
> I have been looking at this patch.

Thanks for reviewing it.

> -   StringInfo  labelfile;
> -   StringInfo  tblspc_map_file;
>     backup_manifest_info manifest;
> +   BackupState backup_state;
> You could use initialize the state here with a {0}.  That's simpler.

BackupState is a pointer  to BackupStateData, we can't initialize that
way. However, I got rid of BackupStateData and used BackupState for
the structure directly, whenever pointer to the structure is required,
I'm using BackupState * to be more clear.

> --- a/src/include/access/xlog_internal.h
> +++ b/src/include/access/xlog_internal.h
> @@ -380,6 +380,31 @@ GetRmgr(RmgrId rmid)
>  }
>  #endif
>
> +/* Structure to hold backup state. */
> +typedef struct BackupStateData
> +{
> Why is that in xlog_internal.h?  This header includes a lot of
> declarations about the internals of WAL, but the backup state is not
> that internal.  I'd like to think that we should begin separating the
> backup-related routines into their own file, as of a set of
> xlogbackup.c/h in this case.  That's a split I have been wondering
> about for some time now.  The internals of xlog.c for the start/stop
> backups are tied to XLogCtlData which map such a switch more
> complicated than it looks, so we can begin small and have the routines
> to create, free and build the label file and the tablespace map in
> this new file.

Good idea. It makes a lot more sense to me, because xlog.c is already
a file of 9000 LOC. I've created xlogbackup.c/.h files and added the
new code there. Once this patch gets in, I can offer my hand to move
do_pg_backup_start() and do_pg_backup_stop() from xlog.c and if okay,
pg_backup_start() and pg_backup_stop() from xlogfuncs.c to
xlogbackup.c/.h. Then, we might have to create new get/set APIs for
XLogCtl fields that do_pg_backup_start() and do_pg_backup_stop()
access.

> +   state->name = (char *) palloc0(len + 1);
> +   memcpy(state->name, name, len);
> Or just pstrdup()?

Done.

> +BackupState
> +get_backup_state(const char *name)
> +{
> I would name this one create_backup_state() instead.
>
> +void
> +create_backup_content_str(BackupState state, bool forhistoryfile)
> +{
> This could be a build_backup_content().

I came up with more meaningful names - allocate_backup_state(),
deallocate_backup_state(), build_backup_content().

> It seems to me that there is no point in having the list of
> tablespaces in BackupStateData? This actually makes the code harder
> to follow, see for example the changes with do_pg_backup_start(), we
> the list of tablespace may or may be not passed down as a pointer of
> BackupStateData while BackupStateData is itself the first argument of
> this routine.  These are independent from the label and backup history
> file, as well.

I haven't stored the list of tablespaces in BackupState, it's the
string that do_pg_backup_start() creates is stored in there for
carrying it till pg_backup_stop(). Adding the tablespace_map,
backup_label, history_file in BackupState makes it easy to carry them
across various backup related functions.

> We need to be careful about the file format (see read_backup_label()),
> and create_backup_content_str() is careful about that which is good.
> Core does not care about the format of the backup history file, though
> some community tools may.

Are you suggesting that we need something like check_history_file()
similar to what read_backup_label() does by parsing each line of the
label file and erroring out if not in the required format?

> I agree that what you are proposing here
> makes the generation of these files easier to follow, but let's
> document what forhistoryfile is here for, at least.  Saving the
> the backup label and history file strings in BackupState is a
> confusing interface IMO.  It would be more intuitive to have the
> backup state in input, and provide the string generated in output
> depending on what we want from the backup state.

We need to carry tablespace_map contents from do_pg_backup_start()
till pg_backup_stop(), backup_label and history_file too are easy to
carry across. Hence it will be good to have all of them i.e.
tablespace_map, backup_label and history_file in the BackupState
structure. IMO, this way is good.

> -   backup_started_in_recovery = RecoveryInProgress();
> +   Assert(state != NULL);
> +
> +   in_recovery = RecoveryInProgress();
> [...]
> -   if (strcmp(backupfrom, "standby") == 0 && !backup_started_in_recovery)
> +   if (state->started_in_recovery == true && in_recovery == false)
>
> I would have kept the naming to backup_started_in_recovery here.  What
> you are doing is logically right by relying on started_in_recovery to
> check if recovery was running when the backup started, but this just
> creates useless noise in the refactoring.

PSA new patch.

> Something unrelated to your patch that I am noticing while scanning
> the area is that we have been always lazy in freeing the label file
> data allocated in TopMemoryContext when using the SQL functions if the
> backup is aborted.  We are not talking about this much amount of
> memory each time a backup is performed, but that could be a cause for
> memory bloat in a server if the same session is used and backups keep
> failing, as the data is freed only on a successful pg_backup_stop().
> Base backups through the replication protocol don't care about that as
> the code keeps around the same pointer for the whole duration of
> perform_base_backup().  Trying to tie the cleanup of the label file
> with the abort phase would be the cause of more complications with
> do_pg_backup_stop(), and xlog.c has no need to know about that now.
> Just a remark for later.

Yeah, I think that can be solved by passing in backup_state to
do_pg_abort_backup(). If okay, I can work on this too as 0002 patch in
this thread or we can discuss this separately to get more attention
after this refactoring patch gets in.

I'm attaching v5 patch with the above review comments addressed,
please review it further.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com