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

On Tue, May 11, 2021 at 8:06 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, May 10, 2021 at 11:46 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Sun, May 9, 2021 at 6:54 PM Euler Taveira <euler@eulerto.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

Thanks for the comments, Attached v3 patch has the changes as suggested.

Regards,
Vignesh