On Wed, Mar 09, 2022 at 06:11:19PM -0500, Chapman Flack wrote:
> 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.
Done. I went ahead and added "label => 'label'" for consistency.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
