On 2022/09/18 23:09, Bharath Rupireddy wrote:
On Sun, Sep 18, 2022 at 1:23 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.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
