Обсуждение: missing documentation for streaming in-progress transactions
Hi,
Found that some documentation hasn't been updated for the changes made as part of
streaming large in-progress transactions. Attached a patch that adds the missing changes. Let me know if anything more needs to be added or any comments on this change.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <itsajin@gmail.com> wrote: > > Hi, > > Found that some documentation hasn't been updated for the changes made as part of > streaming large in-progress transactions. Attached a patch that adds the missing changes. Let me know if anything moreneeds to be added or any comments on this change. > Thanks, this mostly looks good to me. I have slightly modified it. See, what do you think of the attached? -- With Regards, Amit Kapila.
Вложения
On Wed, Apr 7, 2021 at 10:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <itsajin@gmail.com> wrote: > > > > Hi, > > > > Found that some documentation hasn't been updated for the changes made as part of > > streaming large in-progress transactions. Attached a patch that adds the missing changes. Let me know if anything moreneeds to be added or any comments on this change. > > > > Thanks, this mostly looks good to me. I have slightly modified it. > See, what do you think of the attached? > 1. I felt that this protocol documentation needs to include something like a "Since: 2" notation (e.g. see how the javadoc API [1] does it) otherwise with more message types and more protocol versions it is quickly going to become too complicated to know what message or message attribute belongs with what protocol. 2. There are inconsistent terms used for a transaction id. e.g.1 Sometimes called " Transaction id." e.g.2 Sometimes called "Xid of the transaction" I think there should be consistent terminology used on this page 3. There is inconsistent wording for what seems to be the same condition. e.g.1 The existing documentation [2] says "Xid of the transaction. The XID is sent only when user has requested streaming of in-progress transactions." e.g.2 For a similar case the patch says "Xid of the transaction (only present for streamed transactions)." I think there should be consistent wording used on this page where possible. ------ [1] https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html [2] https://www.postgresql.org/docs/devel/protocol-logicalrep-message-formats.html Kind Regards, Peter Smith. Fujitsu Australia
On Thu, Apr 8, 2021 at 3:49 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Wed, Apr 7, 2021 at 10:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <itsajin@gmail.com> wrote: > > 3. > There is inconsistent wording for what seems to be the same condition. > e.g.1 The existing documentation [2] says "Xid of the transaction. The > XID is sent only when user has requested streaming of in-progress > transactions." > e.g.2 For a similar case the patch says "Xid of the transaction (only > present for streamed transactions)." > I think there should be consistent wording used on this page where possible. > I think this is already modified as below in the patch. Is there any other place you are referring to? @@ -6457,8 +6462,7 @@ Message </term> <listitem> <para> - Xid of the transaction. The XID is sent only when user has - requested streaming of in-progress transactions. + Xid of the transaction (only present for streamed transactions). -- With Regards, Amit Kapila.
On Thu, Apr 8, 2021 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Thu, Apr 8, 2021 at 3:49 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > On Wed, Apr 7, 2021 at 10:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <itsajin@gmail.com> wrote: > > > > 3. > > There is inconsistent wording for what seems to be the same condition. > > e.g.1 The existing documentation [2] says "Xid of the transaction. The > > XID is sent only when user has requested streaming of in-progress > > transactions." > > e.g.2 For a similar case the patch says "Xid of the transaction (only > > present for streamed transactions)." > > I think there should be consistent wording used on this page where possible. > > > > I think this is already modified as below in the patch. Is there any > other place you are referring to? No. My mistake. Sorry for the false alarm. ------ KInd Regards, Peter Smith. Fujitsu Australia
On Thu, Apr 8, 2021 at 8:19 AM Peter Smith <smithpb2250@gmail.com> wrote:
1.
I felt that this protocol documentation needs to include something
like a "Since: 2" notation (e.g. see how the javadoc API [1] does it)
otherwise with more message types and more protocol versions it is
quickly going to become too complicated to know what message or
message attribute belongs with what protocol.
Updated.
2.
There are inconsistent terms used for a transaction id.
e.g.1 Sometimes called " Transaction id."
e.g.2 Sometimes called "Xid of the transaction"
I think there should be consistent terminology used on this page
Updated.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Thu, Apr 8, 2021, at 4:25 AM, Ajin Cherian wrote:
Updated.
- Protocol version. Currently only version <literal>1</literal> is
- supported.
- </para>
+ Protocol version. Currently versions <literal>1</literal> and
+ <literal>2</literal> are supported. The version <literal>2</literal>
+ is supported only for server versions 14 and above, and is used to allow
+ streaming of large in-progress transactions.
+ </para>
s/server versions/server version/
I suggest that the last part of the sentence might be "and it allows streaming
of large in-progress transactions"
+ Since: 2
+</para>
+<para>
I didn't like this style because it is not descriptive enough. It is also not a
style adopted by Postgres. I suggest to add something like "This field was
introduced in version 2" or "This field is available since version 2" after the
field description.
+ Xid of the sub-transaction (will be same as xid of the transaction for top-level
+ transactions).
+</para>
Although, sub-transaction is also used in the documentation, I suggest to use
subtransaction. Maybe change the other sub-transaction occurrences too.
On Fri, Apr 9, 2021 at 10:23 AM Euler Taveira <euler@eulerto.com> wrote:
I didn't like this style because it is not descriptive enough. It is also not astyle adopted by Postgres. I suggest to add something like "This field wasintroduced in version 2" or "This field is available since version 2" after thefield description.
I have updated this to "Since protocol version 2"
+ Xid of the sub-transaction (will be same as xid of the transaction for top-level+ transactions).+</para>Although, sub-transaction is also used in the documentation, I suggest to usesubtransaction. Maybe change the other sub-transaction occurrences too.
Updated.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Fri, Apr 9, 2021 at 8:29 AM Ajin Cherian <itsajin@gmail.com> wrote: > > On Fri, Apr 9, 2021 at 10:23 AM Euler Taveira <euler@eulerto.com> wrote: >> >> >> I didn't like this style because it is not descriptive enough. It is also not a >> style adopted by Postgres. I suggest to add something like "This field was >> introduced in version 2" or "This field is available since version 2" after the >> field description. > > > I have updated this to "Since protocol version 2" >> >> >> + Xid of the sub-transaction (will be same as xid of the transaction for top-level >> + transactions). >> +</para> >> >> Although, sub-transaction is also used in the documentation, I suggest to use >> subtransaction. Maybe change the other sub-transaction occurrences too. > > > Updated. > I don't like repeating the same thing for all new messages. So added separate para for the same and few other changes. See what do you think of the attached? -- With Regards, Amit Kapila.
Вложения
On Wed, Apr 07, 2021 at 05:45:16PM +0530, Amit Kapila wrote: > On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <itsajin@gmail.com> wrote: > > > > Found that some documentation hasn't been updated for the changes made as part of > > streaming large in-progress transactions. Attached a patch that adds the missing changes. Let me know if anything moreneeds to be added or any comments on this change. > > > > Thanks, this mostly looks good to me. I have slightly modified it. > See, what do you think of the attached? + Protocol version. Currently versions <literal>1</literal> and + <literal>2</literal> are supported. The version <literal>2</literal> + is supported only for server versions 14 and above, and is used to allow + streaming of large in-progress transactions. The diff briefly confused me, since this is in protocol.sgml, and since the libpq protocol version is 1/2/3, with 2 being removed in v14 (3174d69fb). I suggest to say "replication protocol version 2". I realize that the headings make this more clear when reading the .html, so maybe there's no issue. -- Justin
On Fri, Apr 9, 2021 at 9:58 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Wed, Apr 07, 2021 at 05:45:16PM +0530, Amit Kapila wrote: > > On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <itsajin@gmail.com> wrote: > > > > > > Found that some documentation hasn't been updated for the changes made as part of > > > streaming large in-progress transactions. Attached a patch that adds the missing changes. Let me know if anything moreneeds to be added or any comments on this change. > > > > > > > Thanks, this mostly looks good to me. I have slightly modified it. > > See, what do you think of the attached? > > + Protocol version. Currently versions <literal>1</literal> and > + <literal>2</literal> are supported. The version <literal>2</literal> > + is supported only for server versions 14 and above, and is used to allow > + streaming of large in-progress transactions. > > The diff briefly confused me, since this is in protocol.sgml, and since the > libpq protocol version is 1/2/3, with 2 being removed in v14 (3174d69fb). > I suggest to say "replication protocol version 2". > > I realize that the headings make this more clear when reading the .html, so > maybe there's no issue. > Yeah, this was the reason to not include replication. If one is reading the document or even *.sgml, there shouldn't be any confusion but if you or others feel so, we can use 'replication' as well. -- With Regards, Amit Kapila.
On Fri, Apr 9, 2021 at 9:39 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > I don't like repeating the same thing for all new messages. So added > separate para for the same and few other changes. See what do you > think of the attached? > Pushed. -- With Regards, Amit Kapila.