On 03/09/22 17:21, Nathan Bossart wrote: > Great. Is there any additional feedback on this patch? Should we add an > example of using pg_basebackup in the "Standalone Hot Backups" section, or > should we leave all documentation additions like this for Chap's new > thread?
I'm composing something longer for the new thread, but on the way I noticed something we might fit into this one. I think the listitem In the same connection as before, issue the command: SELECT * FROM pg_backup_stop(true); would be clearer if it used named-parameter form, wait_for_archive => true. This is not strictly necessary, of course, for a function with a single IN parameter, but it's good documentation (and also could save us headaches like these if there is ever another future need to give it more parameters). That listitem doesn't say anything about what the parameter means, which is a little weird, but probably ok because the next listitem does go into it in some detail. I don't think a larger reorg is needed to bring that language one listitem earlier. Just naming the parameter is probably enough to make it less puzzling (or adding in that listitem, at most, "the effect of the wait_for_archive parameter is explained below"). For consistency (and the same futureproofing benefit), I'd go to fast => false in the earlier pg_backup_start as well. I'm more ambivalent about label => 'label'. It would be consistent, but should we just agree for conciseness that there will always be a label and it will always be first? You can pretty much tell in a call what's a label; it's those anonymous trues and falses that are easier to read with named notation. Regards, -Chap
