On Fri, Jul 30, 2021 at 4:33 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Fri, Jul 30, 2021 at 2:02 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Please find attached the latest patch set v100*
> >
> > v99-0002 --> v100-0001
> >
>
> A few minor comments:
>
> (1) doc/src/sgml/protocol.sgml
>
> In the following description, is the word "large" really needed? Also
> "the message ... for a ... message" sounds a bit odd, as does
> "two-phase prepare".
>
> What about the following:
>
> BEFORE:
> + Identifies the message as a two-phase prepare for a
> large in-progress transaction message.
> AFTER:
> + Identifies the message as a prepare for an
> in-progress two-phase transaction.
>
Updated in v101.
The other nearby messages are referring refer to a “streamed
transaction” so I’ve changed this to say “Identifies the message as a
two-phase prepare for a streamed transaction message.” (e.g. compare
this text with the existing similar text for ‘P’).
BTW, I agree with you that "the message ... for a ... message" seems
odd; it was written in this way only to be consistent with existing
documentation, which all uses the same odd phrasing.
> (2) src/backend/replication/logical/worker.c
>
> Similar format comment, but one uses a full-stop and the other
> doesn't, looks a bit odd, since the lines are near each other.
>
> * 1. Replay all the spooled operations - Similar code as for
>
> * 2. Mark the transaction as prepared. - Similar code as for
>
Updated in v101 to make the comments consistent.
> (3) src/test/subscription/t/023_twophase_stream.pl
>
> Shouldn't the following comment mention, for example, "with streaming"
> or something to that effect?
>
> # logical replication of 2PC test
>
Fixed as suggested in v101.
------
Kind Regards,
Peter Smith.
Fujitsu Australia.