On Mon, Jun 24, 2024 at 9:19 AM Jelte Fennema-Nio <postg...@jeltef.nl> wrote: > > > Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But > > > after bumping the version this would be a user visible API change, so > > > I expect it requires a bit more discussion. > > > > I don't know if this is the right idea or not. An alternative would be > > to leave this alone and add PQprotocolMinorVersion(). > > I considered that, but that makes the API a lot more annoying to use > for end users when they want to compare to a version, especially when > they want to include the major version too. > > PQprotocolVersion() >= 30001 > > vs > > PQprotocolVersion() > 3 || (PQprotocolVersion() == 3 && > PQprotocolVersionMinor() >= 1) > > Given that PQprotocolVersion currently has no practical use, because > it always returns 3 in practice. I personally think that changing the > behaviour of API in the way I suggested is the best option here.
Mmm, I was thinking of defining the new function to return the major and minor version in one number, while the existing function could continue to work as now. I see your point too, but I'd like to hear some other opinions before we decide, because I think it's unclear what is actually best here. And also: surely this is not the hill to die on. If it makes others a lot happier to do it one way or the other, I'm severely disinclined to spend energy arguing with those people. And, on the basis of previous experience, this is exactly the sort of question about which opinions sometimes run quite strongly. I would much rather swim with the current than get what I would myself prefer. > > > Patch 5: Starts using the new connection option from Patch 4 > > > > Not sure yet whether I like this approach. > > Could you explain a bit more what you don't like about it? I don't dislike it; I'm just not sure whether it is the best approach to testing the remainder of the patch series. Perhaps the commit message could explain more why this approach was chosen and what value we get from it. > Fair enough. I changed them a bit now, do you think they are good now? I'll try to re-review the patch set when I have a bit more time than I do at this exact moment. > > > Patch 7: Bump the protocol version to 3.2 (see commit message for why > > > not bumping to 3.1) > > > > Good commit message. The point is arguable, so putting forth your best > > argument is important. > > Just to clarify: do you agree with the point now, after that argument? Well, here again, I would like to know what other people think. It doesn't intrinsically matter to me that much what we do here, but it matters to me a lot that extensive recriminations don't ensue afterwards. > I did consider an error message (and I think that is what we discussed > in-person too). But during implementation a WARNING together with a > simple error indicator seemed nicer since that could hook into the > existing infrastructure for reporting warnings (both server and client > side). e.g. you can now provide detail/context/errorcode in the > warning, without having to add all those to the > SetProtocolParameterComplete message. I don't feel super strongly > either way though, so If you prefer the error message to be part of > the SetProtocolParameterComplete message then I'm happy to change > that. My general programming experience has been that any time I decide to include an error flag rather than an error message, I end up regretting it. It's possible that this case is, for some reason, an exception to that principle, but I feel like we need to nail down exactly what the protocol flow *and* the libpq API for these new messages is going to be before I can really have an intelligent opinion about that. -- Robert Haas EDB: http://www.enterprisedb.com
