On Mon, Nov 20, 2023 at 11:11:13AM -0500, Robert Haas wrote: > I think we need more votes to make a change this big. I have a > concern, which I think I've expressed before, that we keep whacking > around the backup APIs, and that has a cost which is potentially > larger than the benefits. The last time we changed the API, we changed > pg_stop_backup to pg_backup_stop, but this doesn't do that, and I > wonder if that's OK. Even if it is, do we really want to change this > API around again after such a short time?
Agreed. > That said, I don't have an intrinsic problem with moving this > information from the backup_label to the backup_manifest file since it > is purely informational. I do think there should perhaps be some > additions to the test cases, though. Yep, cool. Even if we decide to not go with what's discussed in this patch, I think that's useful for some users at the end to get more redundancy, as well. And that's in a format easier to parse. > I am concerned about the interaction of this proposal with incremental > backup. When you take an incremental backup, you get something that > looks a lot like a usable data directory but isn't. To prevent that > from causing avoidable disasters, the present version of the patch > adds INCREMENTAL FROM LSN and INCREMENTAL FROM TLI fields to the > backup_label. pg_combinebackup knows to look for those fields, and the > server knows that if they are present it should refuse to start. With > this change, though, I suppose those fields would end up in > pg_control. But that does not feel entirely great, because we have a > goal of keeping the amount of real data in pg_control below 512 bytes, > the traditional sector size, and this adds another 12 bytes of stuff > to that file that currently doesn't need to be there. I feel like > that's kind of a problem. I don't recall one time where the addition of new fields to the control file was easy to discuss because of its 512B hard limit. Anyway, putting the addition aside for a second, and I've not looked at the incremental backup patch, does the removal of the backup_label make the combine logic more complicated, or that's just moving a chunk of code to do a control file lookup instead of a backup_file parsing? Making the information less readable is definitely an issue for me. A different alternative that I've mentioned upthread is to keep an equivalent of the backup_label and rename it to something like backup.debug or similar, with a name good enough to tell people that we don't care about it being removed. -- Michael
signature.asc
Description: PGP signature
