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

On 2022/09/18 23:09, Bharath Rupireddy wrote:
> On Sun, Sep 18, 2022 at 1:23 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> cfbot fails [1] with v6 patch. I made a silly mistake by not checking
> the output of "make check-world -j 16" fully, I just saw the end
> message "All tests successful." before posting the v6 patch.
>
> The failure is due to perform_base_backup() accessing BackupState's
> tablespace_map without a null check, so I fixed it.
>
> Sorry for the noise. Please review the attached v7 patch further.

Thanks for updating the patch!

=# SELECT * FROM pg_backup_start('test', true);
=# SELECT * FROM pg_backup_stop();
LOG: server process (PID 15651) was terminated by signal 11: Segmentation fault: 11
DETAIL: Failed process was running: SELECT * FROM pg_backup_stop();

With v7 patch, pg_backup_stop() caused the segmentation fault.

=# SELECT * FROM pg_backup_start(repeat('test', 1024));
ERROR: backup label too long (max 1024 bytes)
STATEMENT: SELECT * FROM pg_backup_start(repeat('test', 1024));

=# SELECT * FROM pg_backup_start(repeat('testtest', 1024));
LOG: server process (PID 15844) was terminated by signal 11: Segmentation fault: 11
DETAIL: Failed process was running: SELECT * FROM pg_backup_start(repeat('testtest', 1024));

When I specified longer backup label in the second run of pg_backup_start()
after the first run failed, it caused the segmentation fault.

+ state = (BackupState *) palloc0(sizeof(BackupState));
+ state->name = pstrdup(name);

pg_backup_start() calls allocate_backup_state() and allocates the memory for
the input backup label before checking its length in do_pg_backup_start().
This can cause the memory for backup label to be allocated too much
unnecessary. I think that the maximum length of BackupState.name should
be MAXPGPATH (i.e., maximum allowed length for backup label).

>> The definition of SessionBackupState enum type also should be in xlogbackup.h?
>
> Correct. Basically, all the backup related code from xlog.c,
> xlogfuncs.c and elsewhere can go to xlogbackup.c/.h. I will focus on
> that refactoring patch once this gets in.

Understood.

> Yeah, but they have to be carried from do_pg_backup_stop() to
> pg_backup_stop() or callers and also instead of keeping tablespace_map
> in BackupState and others elsewhere don't seem to be a good idea to
> me. IMO, BackupState is a good place to contain all the information
> that's carried across various functions.

In v7 patch, since pg_backup_stop() calls build_backup_content(),
backup_label and history_file seem not to be carried from do_pg_backup_stop()
to pg_backup_stop(). This makes me still think that it's better not to include
them in BackupState...

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION