> 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.
ok changed - there is minor problem - almost all PQ functions are of int type, but ProtocolVersion is uint
Using different mapping to int can be problematic - can be messy if we cannot to use PG_PROTOCOL macro.
>> + /* 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)
done
Regards
Pavel
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.