Horton

Thread

Commits

  1. doc: Update information of new messages for logical replication.

  1. missing documentation for streaming in-progress transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-04-07T07:40:56Z

    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

  2. Re: missing documentation for streaming in-progress transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-04-07T12:15:16Z

    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 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.

  3. Re: missing documentation for streaming in-progress transactions

    Peter Smith <smithpb2250@gmail.com> — 2021-04-07T22:19:45Z

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

  4. Re: missing documentation for streaming in-progress transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-04-08T02:56:37Z

    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.

  5. Re: missing documentation for streaming in-progress transactions

    Peter Smith <smithpb2250@gmail.com> — 2021-04-08T03:04:27Z

    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

  6. Re: missing documentation for streaming in-progress transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-04-08T07:25:33Z

    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

  7. Re: missing documentation for streaming in-progress transactions

    Euler Taveira <euler@eulerto.com> — 2021-04-09T00:23:02Z

    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
EDB   https://www.enterprisedb.com/

  8. Re: missing documentation for streaming in-progress transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-04-09T02:59:32Z

    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.

regards,
Ajin Cherian
Fujitsu Australia

  9. Re: missing documentation for streaming in-progress transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-04-09T04:09:16Z

    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.

  10. Re: missing documentation for streaming in-progress transactions

    Justin Pryzby <pryzby@telsasoft.com> — 2021-04-09T04:28:52Z

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

  11. Re: missing documentation for streaming in-progress transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-04-09T05:04:38Z

    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 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.

  12. Re: missing documentation for streaming in-progress transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-04-12T06:32:02Z

    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.