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/20 20:43, Bharath Rupireddy wrote:
>> -   if (strlen(backupidstr) > MAXPGPATH)
>> +   if (state->name_overflowed == true)
>>          ereport(ERROR,
>>                  (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>>                   errmsg("backup label too long (max %d bytes)",
>> It does not strike me as a huge issue to force a truncation of such
>> backup label names.  1024 is large enough for basically all users,
>> in my opinion.  Or you could just truncate in the internal logic, but
>> still complain at MAXPGPATH - 1 as the last byte would be for the zero
>> termination.  In short, there is no need to complicate things with
>> name_overflowed.
> 
> We currently allow MAXPGPATH bytes of label name excluding null
> termination. I don't want to change this behaviour. In the attached v9
> patch, I'm truncating the larger names to  MAXPGPATH + 1 bytes in
> backup state (one extra byte for representing that the name has
> overflown, and another extra byte for null termination).

This looks much complicated to me.

Instead of making allocate_backup_state() or reset_backup_state()
store the label name in BackupState before do_pg_backup_start(),
how about making do_pg_backup_start() do that after checking its length?
Seems this can simplify the code very much.

If so, ISTM that we can replace allocate_backup_state() and
reset_backup_state() with just palloc0() and MemSet(), respectively.
Also we can remove fill_backup_label_name().

Regards,

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