Skip to content

Further issues with new major/minor/fix level parsing #256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cpaelzer opened this issue Feb 6, 2018 · 2 comments
Closed

Further issues with new major/minor/fix level parsing #256

cpaelzer opened this issue Feb 6, 2018 · 2 comments

Comments

@cpaelzer
Copy link

cpaelzer commented Feb 6, 2018

  • asyncpg version: 0.13
  • PostgreSQL version: 10.1
  • Python version: 2.7.14-4 / 3.6.4-1
  • Platform: Linux Debian sid / Linux Ubuntu Bionic
  • Do you use pgbouncer?: No
  • Did you install asyncpg with pip?: No (package repository)

In Issue #250 was a fix for Debian branding in version 10.1
But I think it has slight issues and there are more issues with 10.1, not related to the branding but the change of major/minor/fixed version handling.

First of all for the change in #251 :
("PostgreSQL 10.1 (Debian 10.1-3)", (10, 1, 0, 'final', 0),),
I ran into a test fail with this:
E AssertionError: (10, 0, 1, 'final', 0) != ServerVersion(major=10, minor=1, micro=0, releaselevel='final', serial=0)
Following the argument in the comment above the code I think it should instead be:
("PostgreSQL 10.1 (Debian 10.1-3)", (10, 0, 1, 'final', 0),),

Further on I ran into a fail with async def test_server_version_01:

  self.assertEqual(version[:3], (ver_maj, ver_min, ver_fix))

E AssertionError: Tuples differ: (10, 1, 0) != (10, 0, 1)

Again - broken due to Postgresql 10's second number means micro.
That means 10.1 now is:
SELECT current_setting('server_version_num'); => 100001
While 9.5.6 for example was 90506
asyncpg/connection.py:161 get_server_version already does it right, as postgres is doing the mapping.
But test_server_version_01 needs to learn the changed behavior.

That would be solved by:

         version_num = await self.con.fetchval("SELECT current_setting($1)",
                                               'server_version_num', column=0)
         ver_maj = int(version_num[:-4])
-        ver_min = int(version_num[-4:-2])
-        ver_fix = int(version_num[-2:])
+        if ver_maj < 10:
+            ver_min = int(version_num[-4:-2])
+            ver_fix = int(version_num[-2:])
+        else:
+            ver_min = int(version_num[-2:])
+            ver_fix = 0
 
         self.assertEqual(version[:3], (ver_maj, ver_min, ver_fix))

@vitaly-burovoy
Copy link
Contributor

Your reported version is 0.13.0, but a fix is present for 0.14.0 by the commit:
1fa12fe

@cpaelzer
Copy link
Author

cpaelzer commented Feb 6, 2018

Hmm I checked latest head if the code looks the same way, maybe it was just fixed in a different place.
... checking the commit ...
@vitaly-burovoy you are right, that looks good - Thanks!
I'll replace my changes with this (on top of 0.13) and test again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants