On Wed, Oct 23, 2024 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh, here are some review comments for the patch v5-0001 (docs only).
>
> ======
> Commit message
>
> 1.
> The commit message only refers to this as the "streaming option", but
> an option of what? Somewhere we should mention this is an option of
> CREATE SUBSCRIPTION.
>
I think we can change the first line to: "Previously the default value
of streaming option for a subscription was 'off'...."
>
> ======
> doc/src/sgml/ref/create_subscription.sgml
>
> 3.
> <para>
> Specifies whether to enable streaming of in-progress transactions
> - for this subscription. The default value is <literal>off</literal>,
> - meaning all transactions are fully decoded on the publisher and only
> - then sent to the subscriber as a whole.
> + for this subscription. The default value is
> <literal>parallel</literal>,
> + meaning incoming changes are directly applied via one of the parallel
> + apply workers, if available. If no parallel apply worker is free to
> + handle streaming transactions then the changes are written to
> + temporary files and applied after the transaction is committed. Note
> + that if an error happens in a parallel apply worker, the finish LSN
> + of the remote transaction might not be reported in the server log.
> </para>
>
> The other enum values have separate paragraphs:
> - "If set to 'on'" and
> - "If set to 'off'"
>
> I felt the 'parallel' value description should have this same style --
> e.g. a separate paragraph saying:
> - "If set to 'parallel' (the default value)...".
>
> IMO, doing this makes the 3 available enum values much clearer.
>
The currently proposed way is better as it maintains the description
flow. With your proposal, there is some repetition and it is not
making things significantly better. This is a matter of opinion, so I
leave it to others to see if they have any opinions.
--
With Regards,
Amit Kapila.
