On Tue, 5 Sept 2023 at 05:50, Pavel Stehule <pavel.stehule@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
orminor number. It doesn't matter I think. But my opinion in this area is not strong, and I like to see feedback from
otherstoo. 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.