On Fri, 11 Apr 2025 at 09:39, Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> > > On 2025/04/11 18:27, Dave Cramer wrote: > > > > > > On Fri, 11 Apr 2025 at 05:05, Fujii Masao <masao.fu...@oss.nttdata.com > <mailto:masao.fu...@oss.nttdata.com>> wrote: > > > > > > > > On 2025/04/11 5:17, Dave Cramer wrote: > > > No, you are correct. > > > > > > See new patch > > > > Thanks for updating the patch! > > > > - Identifies the message as a protocol version negotiation > > + Identifies the message as a protocol version negotiation. > > + The server sends this message if the requested protocol is > > + not equal to the version the server supports or the client > > + requests protocol options that are not recognized. > > message. > > > You added the sentence starting with "The server sends..." > > between "negotiation" and "message", but it should be placed > > after "message", right? > > > > Even though the requested version is not equal to the latest > > version that the server supports, if it's older than > > the latest one, the message is not sent. So how about > > wording it like this instead: > > > > ------------- > > Identifies the message as a protocol version negotiation message. > > The server sends this message when the client requests a newer > > protocol version than the server supports, or when the client > > includes protocol options that the server does not recognize. > > ------------- > > > > + The protcol version requested by the client unless it is > higher than the > > + latest version we support in which case the latest > protocol version we support. > > > > Maybe rewording this for clarity and using “the server > > instead of “we” would help. For example: > > > > ------------- > > The latest protocol version supported by the server if the client > > requests a newer protocol version than the server supports. > > The protocol version requested by the client, otherwise. > > ------------- > > > > > > Reworded as suggested > > Thanks for updating the patch! > > > While checking the code in older branches, I noticed that the returned > protocol version is always the latest version supported by the server. > However, as we discussed, in master, the server may return the version > requested by the client. The change was introduced in commit 516b87502dc. > So, probably we'll need to update the documentation differently for > master and the older branches. > > > The patch adds a new explanation about when the NegotiateProtocolVersion > message is sent. But a similar explanation already exists in protocol.sgml: > > <term>NegotiateProtocolVersion</term> > <listitem> > <para> > The server does not support the minor protocol version requested > by the client, but does support an earlier version of the > protocol; > this message indicates the highest supported minor version. This > message will also be sent if the client requested unsupported > protocol > options (i.e., beginning with <literal>_pq_.</literal>) in the > startup packet. > Well this isn't quite true since if you request 3.0 and have invalid options it will return 3.0, which is not the highest supported minor version. > > Given that, I'm now wondering if the new description in the patch > might be redundant. > > > Also, your original concern was that the phrase "Newest minor protocol > version" > is inaccurate since the field contains both major and minor version numbers > (e.g., 3.2). However, based on other usage in protocol.sgml and source > comments in related code, "minor version" seems to refer to the full > version > like 3.2, i.e., not just the minor part, so we might not need to reword it > after all. > IMO the comments should be changed to reflect reality. If 3.2 is a minor version what is a major version ? Dave
