On Tue, 5 Sept 2023 at 05:50, Pavel Stehule <pavel.steh...@gmail.com> wrote:
> I prefer to introduce a new function - it is ten lines of code. The form is > not important - it can be a full number or minor number. It doesn't matter I > think. But my opinion in this area is not strong, and I like to see feedback > from others too. It is true that this feature and interface is not fully > complete. I think when using the API it is nicest to have a single function that returns the full version number. i.e. if we're introducing a new function I think it should be PQprotocolFullVersion instead of PQprotocolSubversion. Then instead of doing a version check like this: if (PQprotocolVersion(pset.db) == 3 && PQprotocolSubversion(pset.db) >= 1) You can do the simpler: if (PQprotocolFullVersion(pset.db) >= 301) This is also in line with how you do version checks for postgres versions. So I think this patch should at least add that one instead of PQprotocolSubversion. If we then decide to replace PQprotocolVersion with this new implementation, that would be a trivial change. >> + /* The protocol 3.0 is required */ >> + if (PG_PROTOCOL_MAJOR(their_version) == 3) >> + conn->pversion = their_version; >> >> Let's compare against the actual PG_PROTOCOL_EARLIEST and >> PG_PROTOCOL_LATEST to determine if the version is supported or not. > > > changed I think we should also check the minor version part. So like this instead + if (their_version < PG_PROTOCOL_EARLIEST || their_version > PG_PROTOCOL_LATEST) PS. If you use the -v flag of git format-patch a version number is prepended to your patches. That makes it easier to reference them.
