Re: Corrected documentation of data type for the logical replication message formats.

On Mon, May 10, 2021 at 11:46 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Sun, May 9, 2021 at 6:54 PM Euler Taveira <euler(at)eulerto(dot)com> 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.