missing documentation for streaming in-progress transactions

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

missing documentation for streaming in-progress transactions

Ajin Cherian
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

v1-0001-Add-missing-documentation-of-streaming-in-progres.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

akapila
On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <[hidden email]> 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 more needs 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.

v2-0001-doc-Update-information-of-new-messages-for-logica.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

Peter Smith
On Wed, Apr 7, 2021 at 10:15 PM Amit Kapila <[hidden email]> wrote:

>
> On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <[hidden email]> 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 more needs 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


Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

akapila
On Thu, Apr 8, 2021 at 3:49 AM Peter Smith <[hidden email]> wrote:

>
> On Wed, Apr 7, 2021 at 10:15 PM Amit Kapila <[hidden email]> wrote:
> >
> > On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

Peter Smith
On Thu, Apr 8, 2021 at 12:56 PM Amit Kapila <[hidden email]> wrote:

>
> On Thu, Apr 8, 2021 at 3:49 AM Peter Smith <[hidden email]> wrote:
> >
> > On Wed, Apr 7, 2021 at 10:15 PM Amit Kapila <[hidden email]> wrote:
> > >
> > > On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

Ajin Cherian
In reply to this post by Peter Smith



On Thu, Apr 8, 2021 at 8:19 AM Peter Smith <[hidden email]> 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



v3-0001-doc-Update-information-of-new-messages-for-logica.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

Euler Taveira-3
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.


--
Euler Taveira

Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

Ajin Cherian


On Fri, Apr 9, 2021 at 10:23 AM Euler Taveira <[hidden email]> 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. 

regards,
Ajin Cherian
Fujitsu Australia

v4-0001-doc-Update-information-of-new-messages-for-logica.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

akapila
On Fri, Apr 9, 2021 at 8:29 AM Ajin Cherian <[hidden email]> wrote:

>
> On Fri, Apr 9, 2021 at 10:23 AM Euler Taveira <[hidden email]> 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.

v5-0001-doc-Update-information-of-new-messages-for-logica.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

Justin Pryzby
In reply to this post by akapila
On Wed, Apr 07, 2021 at 05:45:16PM +0530, Amit Kapila wrote:
> On Wed, Apr 7, 2021 at 1:11 PM Ajin Cherian <[hidden email]> 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 more needs 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


Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

akapila
On Fri, Apr 9, 2021 at 9:58 AM Justin Pryzby <[hidden email]> 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 <[hidden email]> 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 more needs 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.


Reply | Threaded
Open this post in threaded view
|

Re: missing documentation for streaming in-progress transactions

akapila
In reply to this post by akapila
On Fri, Apr 9, 2021 at 9:39 AM Amit Kapila <[hidden email]> 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.