On 3/9/22 18:06, Nathan Bossart wrote:
> 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.
+1 from me.
Regards,
-David