Re: Corrected documentation of data type for the logical replication message formats.
On Sun, May 9, 2021 at 10:38 PM vignesh C wrote: > > Hi, > > For some of the logical replication messages the data type documented > was not correct, especially for lsn and xid. For lsn actual datatype > used is uint64 but is documented as int64, similarly for xid, datatype > used is uint32 but documented as int32. > Attached is a patch which has the fix for the same. > Thoughts? If you want to do this then there are more - e.g. Flags should be Uint8 instead of Int8. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Corrected documentation of data type for the logical replication message formats.
On Sun, May 9, 2021, at 9:37 AM, vignesh C wrote: > For some of the logical replication messages the data type documented > was not correct, especially for lsn and xid. For lsn actual datatype > used is uint64 but is documented as int64, similarly for xid, datatype > used is uint32 but documented as int32. > Attached is a patch which has the fix for the same. > Thoughts? There was a discussion [1] a few months ago about it. Read the Message Data Types definition [2]. It is confusing that an internal data type (int64) has a name similar to a generic data type in a protocol definition. As I said [1] we should probably inform that that piece of information (LSN) is a XLogRecPtr. Since this chapter is intended for developers, I think it is fine to include such internal detail. [1] https://www.postgresql.org/message-id/CAH503wBwC8A7DbDYUXRqW1ZAHKpj%2BD9bN7hcgszvP_1FzXbs_Q%40mail.gmail.com [2] https://www.postgresql.org/docs/current/protocol-message-types.html -- Euler Taveira EDB https://www.enterprisedb.com/
Re: Corrected documentation of data type for the logical replication message formats.
On Sun, May 9, 2021 at 11:13 PM Peter Smith wrote: > > On Sun, May 9, 2021 at 10:38 PM vignesh C wrote: > > > > Hi, > > > > For some of the logical replication messages the data type documented > > was not correct, especially for lsn and xid. For lsn actual datatype > > used is uint64 but is documented as int64, similarly for xid, datatype > > used is uint32 but documented as int32. > > Attached is a patch which has the fix for the same. > > Thoughts? > > If you want to do this then there are more - e.g. Flags should be > Uint8 instead of Int8. Irrespective of signed/unsigned, from the description of types [1] it does seem like all those unused "(must be 0)" replication flags ought to have been written as "Int8(0)" instead of "Int8". -- [1] https://www.postgresql.org/docs/devel/protocol-message-types.html Kind Regards, Peter Smith. Fujitsu Australia
Re: Corrected documentation of data type for the logical replication message formats.
On Sun, May 9, 2021 at 6:54 PM Euler Taveira wrote: > > On Sun, May 9, 2021, at 9:37 AM, vignesh C wrote: > > For some of the logical replication messages the data type documented > was not correct, especially for lsn and xid. For lsn actual datatype > used is uint64 but is documented as int64, similarly for xid, datatype > used is uint32 but documented as int32. > Attached is a patch which has the fix for the same. > Thoughts? > > There was a discussion [1] a few months ago about it. Read the Message Data > Types definition [2]. It is confusing that an internal data type (int64) has a > name similar to a generic data type in a protocol definition. As I said [1] we > should probably inform that that piece of information (LSN) is a XLogRecPtr. > Since this chapter is intended for developers, I think it is fine to include > such internal detail. I agree to specifying the actual dataypes like XLogRecPtr for lsn, TimestampTz for timestamp, TransactionId for xid and Oid for the object id. Attached v2 patch which is changed on similar lines. Thoughts? Regards, Vignesh From e82378313f955699375f15f539a5267eb756b313 Mon Sep 17 00:00:00 2001 From: vignesh Date: Sun, 9 May 2021 17:57:08 +0530 Subject: [PATCH v2] Corrected data type for the logical replication message formats. Corrected data type for the logical replication message formats. --- doc/src/sgml/protocol.sgml | 74 +++--- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 2f4dde3beb..bfc29a25f5 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -6403,7 +6403,7 @@ Begin -Int64 +XLogRecPtr @@ -6413,7 +6413,7 @@ Begin -Int64 +TimestampTz @@ -6424,7 +6424,7 @@ Begin -Int32 +TransactionId @@ -6458,7 +6458,7 @@ Message -Int32 +TransactionId @@ -6469,7 +6469,7 @@ Message -Int8 +Uint8 @@ -6480,7 +6480,7 @@ Message -Int64 +XLogRecPtr @@ -6534,7 +6534,7 @@ Commit -Int8 +Uint8(0) @@ -6544,7 +6544,7 @@ Commit -Int64 +XLogRecPtr @@ -6554,7 +6554,7 @@ Commit -Int64 +XLogRecPtr @@ -6564,7 +6564,7 @@ Commit -Int64 +TimestampTz @@ -6599,7 +6599,7 @@ Origin -Int64 +XLogRecPtr @@ -6648,7 +6648,7 @@ Relation -Int32 +TransactionId @@ -6659,7 +6659,7 @@ Relation -Int32 +Oid @@ -6702,7 +6702,7 @@ Relation -Int16 +Uint16 @@ -6715,7 +6715,7 @@ Relation -Int8 +Uint8 @@ -6736,7 +6736,7 @@ Relation -Int32 +Oid @@ -6780,7 +6780,7 @@ Type -Int32 +TransactionId @@ -6791,7 +6791,7 @@ Type -Int32 +Oid @@ -6845,7 +6845,7 @@ Insert -Int32 +TransactionId @@ -6856,7 +6856,7 @@ Insert -Int32 +Oid @@ -6912,7 +6912,7 @@ Update -Int32 +TransactionId @@ -6923,7 +6923,7 @@ Update -Int32 +Oid @@ -7026,7 +7026,7 @@ Delete -Int32 +TransactionId @@ -7037,7 +7037,7 @@ Delete -Int32 +Oid @@ -7115,7 +7115,7 @@ Truncate -Int32 +TransactionId @@ -7136,7 +7136,7 @@ Truncate -Int8 +Uint8 @@ -7147,7 +7147,7 @@ Truncate -Int32 +Oid @@ -7193,7 +7193,7 @@ Stream Start -Int32 +TransactionId @@ -7203,7 +7203,7 @@ Stream Start -Int8 +Uint8 @@ -7262,7 +7262,7 @@ Stream Commit -Int32 +TransactionId @@ -7272,7 +7272,7 @@ Stream Commit -Int8 +Uint8(0) @@ -7282,7 +7282,7 @@ Stream Commit -Int64 +XLogRecPtr @@ -7292,7 +7292,7 @@ Stream Commit -Int64 +XLogRecPtr @@ -7302,7 +7302,7 @@ Stream Commit -Int64 +TimestampTz @@ -7337,7 +7337,7 @@ Stream Abort -Int32 +TransactionId @@ -7347,7 +7347,7 @@ Stream Abort -Int32 +TransactionId @@ -7382,7 +7382,7 @@ TupleData -Int16 +Uint16 -- 2.25.1
Re: Corrected documentation of data type for the logical replication message formats.
On Sun, May 9, 2021 at 6:44 PM Peter Smith wrote: > > On Sun, May 9, 2021 at 10:38 PM vignesh C wrote: > > > > Hi, > > > > For some of the logical replication messages the data type documented > > was not correct, especially for lsn and xid. For lsn actual datatype > > used is uint64 but is documented as int64, similarly for xid, datatype > > used is uint32 but documented as int32. > > Attached is a patch which has the fix for the same. > > Thoughts? > > If you want to do this then there are more - e.g. Flags should be > Uint8 instead of Int8. Thanks for the comments. I have made this change in v2 patch posted at [1]. This also includes the fix to specify uint8(0) at appropriate places. [1] - https://www.postgresql.org/message-id/CALDaNm2G_BJ9G%3DCxy9A6ht-TXPn4nB8W9_BcawuA1uxsNvoWfQ%40mail.gmail.com Regards, Vignesh
Re: Corrected documentation of data type for the logical replication message formats.
On Mon, May 10, 2021 at 11:46 PM vignesh C wrote: > > On Sun, May 9, 2021 at 6:54 PM Euler Taveira wrote: > > > > On Sun, May 9, 2021, at 9:37 AM, vignesh C wrote: > > > > For some of the logical replication messages the data type documented > > was not correct, especially for lsn and xid. For lsn actual datatype > > used is uint64 but is documented as int64, similarly for xid, datatype > > used is uint32 but documented as int32. > > Attached is a patch which has the fix for the same. > > Thoughts? > > > > There was a discussion [1] a few months ago about it. Read the Message Data > > Types definition [2]. It is confusing that an internal data type (int64) > > has a > > name similar to a generic data type in a protocol definition. As I said [1] > > we > > should probably inform that that piece of information (LSN) is a XLogRecPtr. > > Since this chapter is intended for developers, I think it is fine to include > > such internal detail. > > I agree to specifying the actual dataypes like XLogRecPtr for lsn, > TimestampTz for timestamp, TransactionId for xid and Oid for the > object id. Attached v2 patch which is changed on similar lines. > Thoughts? Adding new message "types" does not seem like a good idea to me. e.g. All the message types must be defined by the page [1] so if you add new ones then they should also be defined on that page. But then how many other places ought to make use of those new types? IMO this approach will snowball out of control. But I am also doubtful there was ever actually a (signed/unsigned) problem in the first place. AFAIK the message types like "Int32" etc just happen to have a name that "looks" like a C type, but I think that is the extent of it. It is simply saying how data bytes are transferred on the wire. All the low level C functions [2] always deal with unsigned. ~~ My suggestion would be to restrict your changes to the *description* parts of each message. e.g. maybe you could say what C type the bytes represent when they come off the wire at the other end - something like below. BEFORE Int64 The final LSN of the transaction. AFTER Int64 The final LSN (XLogRecPtr) of the transaction -- [1] https://www.postgresql.org/docs/devel/protocol-message-types.html [2] https://linux.die.net/man/3/ntohl Kind Regards, Peter Smith. Fujitsu Australia.
Re: Corrected documentation of data type for the logical replication message formats.
On Mon, May 10, 2021, at 10:45 AM, vignesh C wrote: > I agree to specifying the actual dataypes like XLogRecPtr for lsn, > TimestampTz for timestamp, TransactionId for xid and Oid for the > object id. Attached v2 patch which is changed on similar lines. > Thoughts? Perhaps I didn't make myself clear, I didn't suggest to replace the actual message data types [1] with the internal representation. We could probably expand the description to include the internal representation. Hence, it is less confusing that the actual text. Peter suggested the same in a previous email. Although, "Message Data Types" is one section before "Message Formats", it is probably intuitive that the data type for each message refer to the previous section. However, it is not so clear three section later. A sentence like The base data types used are described in the section "Messages Data Types". at the first paragraph could help understand what these data types refer to (and also add a link to the data types section). [1] https://www.postgresql.org/docs/current/protocol-message-types.html -- Euler Taveira EDB https://www.enterprisedb.com/
Re: Corrected documentation of data type for the logical replication message formats.
On Tue, May 11, 2021 at 8:06 AM Peter Smith wrote: > > On Mon, May 10, 2021 at 11:46 PM vignesh C wrote: > > > > On Sun, May 9, 2021 at 6:54 PM Euler Taveira wrote: > > > > > > On Sun, May 9, 2021, at 9:37 AM, vignesh C wrote: > > > > > > For some of the logical replication messages the data type documented > > > was not correct, especially for lsn and xid. For lsn actual datatype > > > used is uint64 but is documented as int64, similarly for xid, datatype > > > used is uint32 but documented as int32. > > > Attached is a patch which has the fix for the same. > > > Thoughts? > > > > > > There was a discussion [1] a few months ago about it. Read the Message > > > Data > > > Types definition [2]. It is confusing that an internal data type (int64) > > > has a > > > name similar to a generic data type in a protocol definition. As I said > > > [1] we > > > should probably inform that that piece of information (LSN) is a > > > XLogRecPtr. > > > Since this chapter is intended for developers, I think it is fine to > > > include > > > such internal detail. > > > > I agree to specifying the actual dataypes like XLogRecPtr for lsn, > > TimestampTz for timestamp, TransactionId for xid and Oid for the > > object id. Attached v2 patch which is changed on similar lines. > > Thoughts? > > Adding new message "types" does not seem like a good idea to me. e.g. > All the message types must be defined by the page [1] so if you add > new ones then they should also be defined on that page. But then how > many other places ought to make use of those new types? IMO this > approach will snowball out of control. > > But I am also doubtful there was ever actually a (signed/unsigned) > problem in the first place. AFAIK the message types like "Int32" etc > just happen to have a name that "looks" like a C type, but I think > that is the extent of it. It is simply saying how data bytes are > transferred on the wire. All the low level C functions [2] always deal > with unsigned. > > ~~ > > My suggestion would be to restrict your changes to the *description* > parts of each message. e.g. maybe you could say what C type the bytes > represent when they come off the wire at the other end - something > like below. > > BEFORE > Int64 > The final LSN of the transaction. > > AFTER > Int64 > The final LSN (XLogRecPtr) of the transaction Thanks for the comments, Attached v3 patch has the changes as suggested. Regards, Vignesh From b6d46119b902d3f3007f4b1383774af5859cb615 Mon Sep 17 00:00:00 2001 From: vignesh Date: Tue, 11 May 2021 20:29:43 +0530 Subject: [PATCH v3] Included the actual datatype used in logical replication message description. Included the actual datatype used in logical replication message description. --- doc/src/sgml/protocol.sgml | 105 - 1 file changed, 57 insertions(+), 48 deletions(-) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 2f4dde3beb..ce779b7b1e 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -6378,7 +6378,8 @@ This section describes the detailed format of each logical replication message. These messages are returned either by the replication slot SQL interface or are sent by a walsender. In case of a walsender they are encapsulated inside the replication protocol WAL messages as described in -and generally obey same message flow as physical replication. +and generally obey same message flow as physical replication. The base data types +used are described in . @@ -6407,7 +6408,7 @@ Begin -The final LSN of the transaction. +The final LSN (XLogRecPtr) of the transaction. @@ -6417,8 +6418,8 @@ Begin -Commit timestamp of the transaction. The value is in number -of microseconds since PostgreSQL epoch (2000-01-01). +Commit timestamp (TimestampTz) of the transaction. The value is +in number of microseconds since PostgreSQL epoch (2000-01-01). @@ -6428,7 +6429,7 @@ Begin -Xid of the transaction. +Xid (TransactionId) of the transaction. @@ -6462,8 +6463,9 @@ Message -Xid of the transaction (only present for streamed transactions). -This field is available since protocol version 2. +Xid (TransactionId) of the transaction (only present for +streamed transactions). This field is available since protocol +version 2. @@ -6473,8 +6475,8 @@ Message -Flags; Either 0 for no flags or 1 if the logical decoding -message is transactional. +Flags (uint8); Either 0 for no flags or 1 if the logical +decoding message is transactional. @@ -6484,7 +6486,7 @@ Message -The LSN of the logical decoding message. +The LSN (XLogRecPtr) of the logical decoding message.
Re: Corrected documentation of data type for the logical replication message formats.
On Tue, May 11, 2021 at 9:09 AM Euler Taveira wrote: > > On Mon, May 10, 2021, at 10:45 AM, vignesh C wrote: > > I agree to specifying the actual dataypes like XLogRecPtr for lsn, > TimestampTz for timestamp, TransactionId for xid and Oid for the > object id. Attached v2 patch which is changed on similar lines. > Thoughts? > > Perhaps I didn't make myself clear, I didn't suggest to replace the actual > message data types [1] with the internal representation. We could probably > expand the description to include the internal representation. Hence, it is > less confusing that the actual text. Peter suggested the same in a previous > email. > > Although, "Message Data Types" is one section before "Message Formats", it is > probably intuitive that the data type for each message refer to the previous > section. However, it is not so clear three section later. A sentence like > > The base data types used are described in the section "Messages Data Types". > > at the first paragraph could help understand what these data types refer to > (and also add a link to the data types section). I have included this at the beginning, the same is available in the patch posted at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm2QrB-_96ohonQs-YADC9Puk3caXjn%2B2UYZwxAkX%3DREQQ%40mail.gmail.com Regards, Vignesh
Re: Corrected documentation of data type for the logical replication message formats.
On Wed, May 12, 2021 at 1:02 AM vignesh C wrote: > > Thanks for the comments, Attached v3 patch has the changes as suggested. This v3 mostly looks good to me now except for some minor comments about the flags. ~~~ 1. Commit flags @@ -6534,11 +6536,11 @@ Commit -Int8 +Uint8(0) -Flags; currently unused (must be 0). +Flags (uint8(0)); currently unused (must be 0). a) There is no data type Uint8. That should be "Int8(0)" b) I think the "(0)" does not belong in the description part. e.g. change to "Flags (uint8); currently unused (must be 0)." ~~~ 2. Stream Commit flags @@ -7276,7 +7284,7 @@ Stream Commit -Flags; currently unused (must be 0). +Flags (uint8(0)); currently unused (must be 0). a) The data type should say "Int8(0)" b) I think the "(0)" does not belong in the description part. e.g. change to "Flags (uint8); currently unused (must be 0)." ~~~ 3. I felt that saying "(must be 0)" for those unused flag descriptions is unnecessary since that is exactly what "Int8(0)" already means. e.g. consider change to just say "Flags (uint8); currently unused." -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Corrected documentation of data type for the logical replication message formats.
On Wed, May 12, 2021 at 3:36 AM Peter Smith wrote: > > On Wed, May 12, 2021 at 1:02 AM vignesh C wrote: > > > > Thanks for the comments, Attached v3 patch has the changes as suggested. > > This v3 mostly looks good to me now except for some minor comments > about the flags. > > ~~~ > > 1. Commit flags > > @@ -6534,11 +6536,11 @@ Commit > > > > -Int8 > +Uint8(0) > > > > -Flags; currently unused (must be 0). > +Flags (uint8(0)); currently unused (must be 0). > > > > > a) There is no data type Uint8. That should be "Int8(0)" > Modified. > b) I think the "(0)" does not belong in the description part. > e.g. change to "Flags (uint8); currently unused (must be 0)." > Modified > ~~~ > > 2. Stream Commit flags > > @@ -7276,7 +7284,7 @@ Stream Commit > > > > -Flags; currently unused (must be 0). > +Flags (uint8(0)); currently unused (must be 0). > > > > > a) The data type should say "Int8(0)" > Modified. > b) I think the "(0)" does not belong in the description part. > e.g. change to "Flags (uint8); currently unused (must be 0)." > Modified. > ~~~ > > 3. I felt that saying "(must be 0)" for those unused flag descriptions > is unnecessary since that is exactly what "Int8(0)" already means. > e.g. consider change to just say "Flags (uint8); currently unused." Modified. Thanks for the comments. Attached v4 patch has the fix for the same. Regards, Vignesh From b7efd44f0bbbf694fff7e97dea7cf6ca0271ca69 Mon Sep 17 00:00:00 2001 From: vignesh Date: Tue, 11 May 2021 20:29:43 +0530 Subject: [PATCH v4] Included the actual datatype used in logical replication message description. Included the actual datatype used in logical replication message description. --- doc/src/sgml/protocol.sgml | 107 - 1 file changed, 58 insertions(+), 49 deletions(-) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 2f4dde3beb..c05fd3af4c 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -6378,7 +6378,8 @@ This section describes the detailed format of each logical replication message. These messages are returned either by the replication slot SQL interface or are sent by a walsender. In case of a walsender they are encapsulated inside the replication protocol WAL messages as described in -and generally obey same message flow as physical replication. +and generally obey same message flow as physical replication. The base data types +used are described in . @@ -6407,7 +6408,7 @@ Begin -The final LSN of the transaction. +The final LSN (XLogRecPtr) of the transaction. @@ -6417,8 +6418,8 @@ Begin -Commit timestamp of the transaction. The value is in number -of microseconds since PostgreSQL epoch (2000-01-01). +Commit timestamp (TimestampTz) of the transaction. The value is +in number of microseconds since PostgreSQL epoch (2000-01-01). @@ -6428,7 +6429,7 @@ Begin -Xid of the transaction. +Xid (TransactionId) of the transaction. @@ -6462,8 +6463,9 @@ Message -Xid of the transaction (only present for streamed transactions). -This field is available since protocol version 2. +Xid (TransactionId) of the transaction (only present for +streamed transactions). This field is available since protocol +version 2. @@ -6473,8 +6475,8 @@ Message -Flags; Either 0 for no flags or 1 if the logical decoding -message is transactional. +Flags (uint8); Either 0 for no flags or 1 if the logical +decoding message is transactional. @@ -6484,7 +6486,7 @@ Message -The LSN of the logical decoding message. +The LSN (XLogRecPtr) of the logical decoding message. @@ -6534,11 +6536,11 @@ Commit -Int8 +Int8(0) -Flags; currently unused (must be 0). +Flags (uint8); currently unused. @@ -6548,7 +6550,7 @@ Commit -The LSN of the commit. +The LSN (XLogRecPtr) of the commit. @@ -6558,7 +6560,7 @@ Commit -The end LSN of the transaction. +The end LSN (XLogRecPtr) of the transaction. @@ -6568,8 +6570,8 @@ Commit -Commit timestamp of the transaction. The value is in number -of microseconds since PostgreSQL epoch (2000-01-01). +Commit timestamp (TimestampTz) of the transaction. The value is +in number of microseconds since PostgreSQL epoch (2000-01-01). @@ -6603,7 +6605,7 @@ Origin -The LSN of the commit on the origin server. +The LSN (XLogRec
Re: Corrected documentation of data type for the logical replication message formats.
On Wed, May 12, 2021 at 11:09 PM vignesh C wrote: ... > > Thanks for the comments. Attached v4 patch has the fix for the same. > I have not tried this patch so I cannot confirm whether it applies or renders OK, but just going by the v4 content this now LGTM. Kind Regards, Peter Smith. Fujitsu Australia
Re: Corrected documentation of data type for the logical replication message formats.
On Thu, May 13, 2021 at 5:57 AM Peter Smith wrote: > > On Wed, May 12, 2021 at 11:09 PM vignesh C wrote: > ... > > > > Thanks for the comments. Attached v4 patch has the fix for the same. > > > > I have not tried this patch so I cannot confirm whether it applies or > renders OK, but just going by the v4 content this now LGTM. Thanks for having a look at it. I have added a commitfest entry for this: https://commitfest.postgresql.org/33/3117/ Regards, Vignesh
Re: Corrected documentation of data type for the logical replication message formats.
Hi Vignesh. FYI - Because the other patch [1] was pushed ahead of this one, I think your patch now needs to be updated for the new message types that were introduced. -- [1] https://github.com/postgres/postgres/commit/a8fd13cab0ba815e9925dc9676e6309f699b5f72#diff-331c33fd11c3ed85f9dbfead93f139c20ff3a25176651fc2ed37c486b97630e6 Kind Regards, Peter Smith. Fujitsu Australia
Re: Corrected documentation of data type for the logical replication message formats.
On Fri, Jul 16, 2021 at 8:52 AM Peter Smith wrote: > > Hi Vignesh. > > FYI - Because the other patch [1] was pushed ahead of this one, I > think your patch now needs to be updated for the new message types > that were introduced. Thanks, I have made the changes for the same in the v5 patch attached. Regards, Vignesh From a5ce224c739b63f63ec68c268078c338e7de6ea9 Mon Sep 17 00:00:00 2001 From: vignesh Date: Tue, 11 May 2021 20:29:43 +0530 Subject: [PATCH v5] Included the actual datatype used in logical replication message description. Included the actual datatype used in logical replication message description. --- doc/src/sgml/protocol.sgml | 156 +++-- 1 file changed, 82 insertions(+), 74 deletions(-) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index e8cb78ff1f..5648ddc813 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -6399,7 +6399,8 @@ This section describes the detailed format of each logical replication message. These messages are returned either by the replication slot SQL interface or are sent by a walsender. In case of a walsender they are encapsulated inside the replication protocol WAL messages as described in -and generally obey same message flow as physical replication. +and generally obey same message flow as physical replication. The base data types +used are described in . @@ -6428,7 +6429,7 @@ Begin -The final LSN of the transaction. +The final LSN (XLogRecPtr) of the transaction. @@ -6438,8 +6439,8 @@ Begin -Commit timestamp of the transaction. The value is in number -of microseconds since PostgreSQL epoch (2000-01-01). +Commit timestamp (TimestampTz) of the transaction. The value is +in number of microseconds since PostgreSQL epoch (2000-01-01). @@ -6449,7 +6450,7 @@ Begin -Xid of the transaction. +Xid (TransactionId) of the transaction. @@ -6483,8 +6484,9 @@ Message -Xid of the transaction (only present for streamed transactions). -This field is available since protocol version 2. +Xid (TransactionId) of the transaction (only present for +streamed transactions). This field is available since protocol +version 2. @@ -6494,8 +6496,8 @@ Message -Flags; Either 0 for no flags or 1 if the logical decoding -message is transactional. +Flags (uint8); Either 0 for no flags or 1 if the logical +decoding message is transactional. @@ -6505,7 +6507,7 @@ Message -The LSN of the logical decoding message. +The LSN (XLogRecPtr) of the logical decoding message. @@ -6567,11 +6569,11 @@ Commit -Int8 +Int8(0) -Flags; currently unused (must be 0). +Flags (uint8); currently unused. @@ -6581,7 +6583,7 @@ Commit -The LSN of the commit. +The LSN (XLogRecPtr) of the commit. @@ -6591,7 +6593,7 @@ Commit -The end LSN of the transaction. +The end LSN (XLogRecPtr) of the transaction. @@ -6601,8 +6603,8 @@ Commit -Commit timestamp of the transaction. The value is in number -of microseconds since PostgreSQL epoch (2000-01-01). +Commit timestamp (TimestampTz) of the transaction. The value is +in number of microseconds since PostgreSQL epoch (2000-01-01). @@ -6636,7 +6638,7 @@ Origin -The LSN of the commit on the origin server. +The LSN (XLogRecPtr) of the commit on the origin server. @@ -6685,8 +6687,9 @@ Relation - Xid of the transaction (only present for streamed transactions). - This field is available since protocol version 2. + Xid (TransactionId) of the transaction (only present for + streamed transactions). This field is available since + protocol version 2. @@ -6696,7 +6699,7 @@ Relation -ID of the relation. +ID (Oid) of the relation. @@ -6752,8 +6755,8 @@ Relation -Flags for the column. Currently can be either 0 for no flags -or 1 which marks the column as part of the key. +Flags (uint8) for the column. Currently can be either 0 for no +flags or 1 which marks the column as part of the key. @@ -6773,7 +6776,7 @@ Relation -ID of the column's data type. +ID (Oid) of the column's data type. @@ -6817,8 +6820,9 @@ Type - Xid of the transaction (only present for streamed transactions). - This field is ava
Re: Corrected documentation of data type for the logical replication message formats.
I think the patch maybe is not quite correct for all the flags. For example, @@ -7607,44 +7615,44 @@ are available since protocol version 3. Int8 -Flags; currently unused (must be 0). +Flags (uint8); currently unused. AFAIK, even though the flags are "unused", the code still insists that most (or all? Please check the code) of these flag values MUST be 0, so I think that this zero value requirement ought to be indicated in the docs using the "Int8(0)" convention [1]. For example, BEFORE Int8 Flags (uint8); currently unused. AFTER Int8(0) Flags (uint8); currently unused. -- [1] https://www.postgresql.org/docs/devel/protocol-message-types.html Kind Regards, Peter Smith. Fujitsu Australia.
Re: Corrected documentation of data type for the logical replication message formats.
On Fri, Jul 23, 2021 at 3:23 AM Peter Smith wrote: > > I think the patch maybe is not quite correct for all the flags. > > For example, > > @@ -7607,44 +7615,44 @@ are available since protocol version 3. > > Int8 > > -Flags; currently unused (must be 0). > +Flags (uint8); currently unused. > > > > AFAIK, even though the flags are "unused", the code still insists that > most (or all? Please check the code) of these flag values MUST be 0, > so I think that this zero value requirement ought to be indicated in > the docs using the "Int8(0)" convention [1]. For example, > > BEFORE > Int8 > Flags (uint8); currently unused. > > AFTER > Int8(0) > Flags (uint8); currently unused. Thanks for the comments. Attached v6 patch has the changes for the same. Regards, Vignesh From 398e67e672cb4b264dd8863f17caa431b2a82153 Mon Sep 17 00:00:00 2001 From: vignesh Date: Tue, 11 May 2021 20:29:43 +0530 Subject: [PATCH v6] Included the actual datatype used in logical replication message description. Included the actual datatype used in logical replication message description. --- doc/src/sgml/protocol.sgml | 168 - 1 file changed, 91 insertions(+), 77 deletions(-) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index e8cb78ff1f..5aad6dbf7a 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -6399,7 +6399,8 @@ This section describes the detailed format of each logical replication message. These messages are returned either by the replication slot SQL interface or are sent by a walsender. In case of a walsender they are encapsulated inside the replication protocol WAL messages as described in -and generally obey same message flow as physical replication. +and generally obey same message flow as physical replication. The base data types +used are described in . @@ -6428,7 +6429,7 @@ Begin -The final LSN of the transaction. +The final LSN (XLogRecPtr) of the transaction. @@ -6438,8 +6439,8 @@ Begin -Commit timestamp of the transaction. The value is in number -of microseconds since PostgreSQL epoch (2000-01-01). +Commit timestamp (TimestampTz) of the transaction. The value is +in number of microseconds since PostgreSQL epoch (2000-01-01). @@ -6449,7 +6450,7 @@ Begin -Xid of the transaction. +Xid (TransactionId) of the transaction. @@ -6483,8 +6484,9 @@ Message -Xid of the transaction (only present for streamed transactions). -This field is available since protocol version 2. +Xid (TransactionId) of the transaction (only present for +streamed transactions). This field is available since protocol +version 2. @@ -6494,8 +6496,8 @@ Message -Flags; Either 0 for no flags or 1 if the logical decoding -message is transactional. +Flags (uint8); Either 0 for no flags or 1 if the logical +decoding message is transactional. @@ -6505,7 +6507,7 @@ Message -The LSN of the logical decoding message. +The LSN (XLogRecPtr) of the logical decoding message. @@ -6567,11 +6569,11 @@ Commit -Int8 +Int8(0) -Flags; currently unused (must be 0). +Flags (uint8); currently unused. @@ -6581,7 +6583,7 @@ Commit -The LSN of the commit. +The LSN (XLogRecPtr) of the commit. @@ -6591,7 +6593,7 @@ Commit -The end LSN of the transaction. +The end LSN (XLogRecPtr) of the transaction. @@ -6601,8 +6603,8 @@ Commit -Commit timestamp of the transaction. The value is in number -of microseconds since PostgreSQL epoch (2000-01-01). +Commit timestamp (TimestampTz) of the transaction. The value is +in number of microseconds since PostgreSQL epoch (2000-01-01). @@ -6636,7 +6638,7 @@ Origin -The LSN of the commit on the origin server. +The LSN (XLogRecPtr) of the commit on the origin server. @@ -6685,8 +6687,9 @@ Relation - Xid of the transaction (only present for streamed transactions). - This field is available since protocol version 2. + Xid (TransactionId) of the transaction (only present for + streamed transactions). This field is available since + protocol version 2. @@ -6696,7 +6699,7 @@ Relation -ID of the relation. +ID (Oid) of the relation. @@ -6752,8 +6755,8 @@ Relation -Flags for the column. Currently can be either 0 for no flags -or
Re: Corrected documentation of data type for the logical replication message formats.
vignesh C writes: [ v6-0001-Included-the-actual-datatype-used-in-logical-repl.patch ] I see what you want to do here, but the way you did it seems quite detrimental to the readability of the field descriptions. Parenthesized interjections should be used sparingly. I'm inclined to think that the equivalent data type is part of the field data type specification, and thus that we ought to put it in the data type part of each entry. So we'd have something like Int64 (XLogRecPtr) The final LSN of the transaction. instead of what you did here. Parentheses might not be the best punctuation to use, given the existing convention about parenthesized specific values, but we could probably settle on some other markup. Or just ignore the ambiguity. Another idea is to add the data type info at the ends of items instead of cramming it into the sentences, thus: The final LSN of the transaction. (XLogRecPtr) I don't find that better personally, but maybe others will think differently. regards, tom lane
Re: Corrected documentation of data type for the logical replication message formats.
On Sat, Jul 31, 2021 at 7:00 AM Tom Lane wrote: > > vignesh C writes: > [ v6-0001-Included-the-actual-datatype-used-in-logical-repl.patch ] > > I see what you want to do here, but the way you did it seems quite > detrimental to the readability of the field descriptions. > Parenthesized interjections should be used sparingly. > > I'm inclined to think that the equivalent data type is part of the > field data type specification, and thus that we ought to put it in > the data type part of each entry. So we'd have something like > > > > Int64 (XLogRecPtr) > > > > The final LSN of the transaction. > > > > > instead of what you did here. Parentheses might not be the best > punctuation to use, given the existing convention about parenthesized > specific values, but we could probably settle on some other markup. > Or just ignore the ambiguity. +1 to change it like suggested above. The specific value for the flags might then look like below, but that does not look too bad to me. Int8 (uint8) (0) -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Corrected documentation of data type for the logical replication message formats.
On Sat, Jul 31, 2021 at 2:30 AM Tom Lane wrote: > > vignesh C writes: > [ v6-0001-Included-the-actual-datatype-used-in-logical-repl.patch ] > > I see what you want to do here, but the way you did it seems quite > detrimental to the readability of the field descriptions. > Parenthesized interjections should be used sparingly. > > I'm inclined to think that the equivalent data type is part of the > field data type specification, and thus that we ought to put it in > the data type part of each entry. So we'd have something like > > > > Int64 (XLogRecPtr) > > > > The final LSN of the transaction. > > > > I made changes based on the feedback, since Peter also was in favour of using this approach, I modified based on the first approach. Attached v7 patch has the changes for the same. Regards, Vignesh From 285f4fe337222690f27f164d7210afbda84c37d0 Mon Sep 17 00:00:00 2001 From: Vigneshwaran c Date: Sun, 1 Aug 2021 20:21:31 +0530 Subject: [PATCH v7] Included the actual datatype used in logical replication message format documentation. Included the actual datatype used in logical replication message format documentation. --- doc/src/sgml/protocol.sgml | 159 +++-- 1 file changed, 100 insertions(+), 59 deletions(-) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 9843953b05..ac53c82d19 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -6411,7 +6411,8 @@ This section describes the detailed format of each logical replication message. These messages are returned either by the replication slot SQL interface or are sent by a walsender. In case of a walsender they are encapsulated inside the replication protocol WAL messages as described in -and generally obey same message flow as physical replication. +and generally obey same message flow as physical replication. The base data types +used are described in . @@ -6436,7 +6437,7 @@ Begin -Int64 +Int64 (XLogRecPtr) @@ -6446,7 +6447,7 @@ Begin -Int64 +Int64 (TimestampTz) @@ -6457,7 +6458,7 @@ Begin -Int32 +Int32 (TransactionId) @@ -6491,7 +6492,7 @@ Message -Int32 +Int32 (TransactionId) @@ -6502,7 +6503,7 @@ Message -Int8 +Int8 (uint8) @@ -6513,7 +6514,7 @@ Message -Int64 +Int64 (XLogRecPtr) @@ -6579,17 +6580,17 @@ Commit -Int8 +Int8(0) (uint8) -Flags; currently unused (must be 0). +Flags; currently unused. -Int64 +Int64 (XLogRecPtr) @@ -6599,7 +6600,7 @@ Commit -Int64 +Int64 (XLogRecPtr) @@ -6609,7 +6610,7 @@ Commit -Int64 +Int64 (TimestampTz) @@ -6644,7 +6645,7 @@ Origin -Int64 +Int64 (XLogRecPtr) @@ -6693,7 +6694,7 @@ Relation -Int32 +Int32 (TransactionId) @@ -6704,7 +6705,7 @@ Relation -Int32 +Int32 (Oid) @@ -6760,7 +6761,7 @@ Relation -Int8 +Int8 (uint8) @@ -6781,7 +6782,7 @@ Relation -Int32 +Int32 (Oid) @@ -6825,7 +6826,7 @@ Type -Int32 +Int32 (TransactionId) @@ -6836,7 +6837,7 @@ Type -Int32 +Int32 (Oid) @@ -6890,7 +6891,7 @@ Insert -Int32 +Int32 (TransactionId) @@ -6901,7 +6902,7 @@ Insert -Int32 +Int32 (Oid) @@ -6957,7 +6958,7 @@ Update -Int32 +Int32 (TransactionId) @@ -6968,7 +6969,7 @@ Update -Int32 +Int32 (Oid) @@ -7071,7 +7072,7 @@ Delete -Int32 +Int32 (TransactionId) @@ -7082,7 +7083,7 @@ Delete -Int32 +Int32 (Oid) @@ -7160,7 +7161,7 @@ Truncate -Int32 +Int32 (TransactionId) @@ -7192,7 +7193,7 @@ Truncate -Int32 +Int32 (Oid) @@ -7238,7 +7239,7 @@ Stream Start -Int32 +Int32 (TransactionId) @@ -7307,7 +7308,7 @@ Stream Commit -Int32 +Int32 (TransactionId) @@ -7317,17 +7318,17 @@ Stream Commit -Int8 +Int8(0) (uint8) -Flags; currently unused (must be 0). +Flags; currently unused. -Int64 +Int64 (XLogRecPtr) @@ -7337,7 +7338,7 @@ Stream Commit -Int64 +Int64 (XLogRecPtr) @@ -7347,7 +7348,7 @@ Stream Commit -Int64 +Int64 (TimestampTz) @@ -7382,7 +7383,7 @@ Stream Abort -Int32 +Int32 (TransactionId) @@ -7392,7 +7393,7 @@ Stream Abort -Int32 +Int32 (TransactionId) @@ -7432,21 +7433,27 @@ are available since protocol version 3. -Int64 + +Int64 (XLogR
Re: Corrected documentation of data type for the logical replication message formats.
On Sun, Aug 1, 2021 at 4:11 PM Peter Smith wrote:
>
> On Sat, Jul 31, 2021 at 7:00 AM Tom Lane wrote:
> >
> > vignesh C writes:
> > [ v6-0001-Included-the-actual-datatype-used-in-logical-repl.patch ]
> >
> > I see what you want to do here, but the way you did it seems quite
> > detrimental to the readability of the field descriptions.
> > Parenthesized interjections should be used sparingly.
> >
> > I'm inclined to think that the equivalent data type is part of the
> > field data type specification, and thus that we ought to put it in
> > the data type part of each entry. So we'd have something like
> >
> >
> >
> > Int64 (XLogRecPtr)
> >
> >
> >
> > The final LSN of the transaction.
> >
> >
> >
> >
> > instead of what you did here. Parentheses might not be the best
> > punctuation to use, given the existing convention about parenthesized
> > specific values, but we could probably settle on some other markup.
> > Or just ignore the ambiguity.
>
> +1 to change it like suggested above.
>
> The specific value for the flags might then look like below, but that
> does not look too bad to me.
>
>
> Int8 (uint8) (0)
>
I felt we can change it like:
Int8(0) (uint8)
I felt the flag value can be kept first followed by the data type since it
is used similarly for the other message types like below:
Byte1('C')
I have made changes in similar lines and posted the patch at [1].
Thoughts?
[1] -
https://www.postgresql.org/message-id/CALDaNm3sK75Mo%2BVzLmNGe29gYtJoeKHshAK0GDiAzfAj6LQPdw%40mail.gmail.com
Regards,
Vignesh
Re: Corrected documentation of data type for the logical replication message formats.
On Mon, Aug 2, 2021 at 1:32 AM vignesh C wrote:
>
> On Sun, Aug 1, 2021 at 4:11 PM Peter Smith wrote:
> >
> > On Sat, Jul 31, 2021 at 7:00 AM Tom Lane wrote:
> > >
> > > vignesh C writes:
> > > [ v6-0001-Included-the-actual-datatype-used-in-logical-repl.patch ]
> > >
> > > I see what you want to do here, but the way you did it seems quite
> > > detrimental to the readability of the field descriptions.
> > > Parenthesized interjections should be used sparingly.
> > >
> > > I'm inclined to think that the equivalent data type is part of the
> > > field data type specification, and thus that we ought to put it in
> > > the data type part of each entry. So we'd have something like
> > >
> > >
> > >
> > > Int64 (XLogRecPtr)
> > >
> > >
> > >
> > > The final LSN of the transaction.
> > >
> > >
> > >
> > >
> > > instead of what you did here. Parentheses might not be the best
> > > punctuation to use, given the existing convention about parenthesized
> > > specific values, but we could probably settle on some other markup.
> > > Or just ignore the ambiguity.
> >
> > +1 to change it like suggested above.
> >
> > The specific value for the flags might then look like below, but that
> > does not look too bad to me.
> >
> >
> > Int8 (uint8) (0)
> >
>
> I felt we can change it like:
>
> Int8(0) (uint8)
>
>
> I felt the flag value can be kept first followed by the data type since it is
> used similarly for the other message types like below:
>
> Byte1('C')
>
>
> I have made changes in similar lines and posted the patch at [1].
> Thoughts?
I agree. The specified value looks better when it comes first, as you did it.
~~
Option #1:
Int8(0) (uint8)
Int64 (XLogRecPtr)
Option #2:
Int8(0) [uint8]
Int64 [XLogRecPtr]
Option #3:
Int8(0) -- uint
Int64 -- XLogRecPtr
etc...
Probably my slight favourite is Option #2 above, but YMMV. Any format
you choose which is similar to those above is fine by me.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Corrected documentation of data type for the logical replication message formats.
Peter Smith writes: > I agree. The specified value looks better when it comes first, as you did it. Actually, it looks to me like we don't have to resolve the question of which should come first, because I don't see any cases where it's useful to have both. I don't agree with appending "uint8" to those field descriptions, because it's adding no information, especially when the high bit couldn't be set anyway. At some point it might be useful to add UInt to the set of base data types, and then go through all the message types and decide which fields we think are unsigned. But that is not this patch, and there would be questions about whether it constituted a protocol break. I noticed also that having to add "(Oid)" was sort of self-inflicted damage, because the field descriptions were using the very vague term "ID", when they could have said "OID" and been clear. I left the "(Oid)" additions in place but also changed the text. Pushed with those changes. I couldn't resist copy-editing the section intro, too. regards, tom lane
Re: Corrected documentation of data type for the logical replication message formats.
On Mon, Aug 2, 2021 at 9:10 PM Tom Lane wrote: > > Peter Smith writes: > > I agree. The specified value looks better when it comes first, as you did > > it. > > Actually, it looks to me like we don't have to resolve the question of > which should come first, because I don't see any cases where it's > useful to have both. I don't agree with appending "uint8" to those > field descriptions, because it's adding no information, especially > when the high bit couldn't be set anyway. > > At some point it might be useful to add UInt to the set of base > data types, and then go through all the message types and decide > which fields we think are unsigned. But that is not this patch, > and there would be questions about whether it constituted a protocol > break. > > I noticed also that having to add "(Oid)" was sort of self-inflicted > damage, because the field descriptions were using the very vague > term "ID", when they could have said "OID" and been clear. I left > the "(Oid)" additions in place but also changed the text. > > Pushed with those changes. I couldn't resist copy-editing the section > intro, too. Thanks for pushing the patch. Regards, Vignesh
