From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Make default subscription streaming option as Parallel |
Date: | 2024-10-23 03:52:38 |
Message-ID: | CAA4eK1Jz6tX78h7OOdAkYE+cwdJMfEczXh0rPHrH5cQE9Gk79Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Oct 23, 2024 at 5:58 AM Peter Smith <smithpb2250(at)gmail(dot)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.
From | Date | Subject | |
---|---|---|---|
Next Message | torikoshia | 2024-10-23 03:58:12 | Re: Enhancing Memory Context Statistics Reporting |
Previous Message | Jingtang Zhang | 2024-10-23 03:01:05 | Re: use a non-locking initial test in TAS_SPIN on AArch64 |