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

Add libpq version discovery #323

Merged
merged 2 commits into from Jun 2, 2015

Conversation

Projects
None yet
2 participants
@a1exsh
Copy link
Contributor

a1exsh commented Jun 1, 2015

Patch for issue #35

@@ -899,6 +914,7 @@ INIT_MODULE(_psycopg)(void)
/* set some module's parameters */
PyModule_AddStringConstant(module, "__version__", PSYCOPG_VERSION);
PyModule_AddStringConstant(module, "__doc__", "psycopg PostgreSQL driver");
PyModule_AddIntConstant(module, "__libpq_version__", PG_VERSION_NUM);

This comment has been minimized.

@dvarrazzo

dvarrazzo Jun 1, 2015

Member

I think we may just use strtol(PG_VERSION_HEX, NULL, 0) here and avoid introducing PG_VERSION_NUM?

This comment has been minimized.

@a1exsh

a1exsh Jun 1, 2015

Contributor

On Mon, Jun 1, 2015 at 6:41 PM, Daniele Varrazzo notifications@github.com
wrote:

In psycopg/psycopgmodule.c
#323 (comment):

@@ -899,6 +914,7 @@ INIT_MODULE(_psycopg)(void)
/* set some module's parameters */
PyModule_AddStringConstant(module, "version", PSYCOPG_VERSION);
PyModule_AddStringConstant(module, "doc", "psycopg PostgreSQL driver");

  • PyModule_AddIntConstant(module, "libpq_version", PG_VERSION_NUM);

I think we may just use strtol(PG_VERSION_HEX, NULL, 0) here and avoid
introducing PG_VERSION_NUM?

I think that is going to be confusing, since both PQserverVersion() and
PQlibVersion() are specified to return the number constructed in decimal,
and we already have a check in skip_before/after_postgres that uses this
exact format.

Moreover, PG_VERSION_HEX is not a string, but also a number like
PG_VERSION_NUM, just constructed differently, so that strtol() call won't
even compile.

Now that I look at how PG_VERSION_HEX is constructed, I think it's going to
be a lot more confusing when PG version number wraps around 10: it's going
to look like, e.g. 0x0a0102, while PG_VERSION_NUM for the same version
would be 100102. Since we do not even expose the hex version in python, I
would see more sense in moving away from it in favor of decimal one.

Alex

This comment has been minimized.

@dvarrazzo

dvarrazzo Jun 1, 2015

Member

It could be a good idea actually. I actually never noticed that PQserverVersion is decimal while ours is hex :)

This comment has been minimized.

@a1exsh

a1exsh Jun 2, 2015

Contributor

And I think we don't really need to supply the version through setup.py, we can just include pg_config.h, where PG_VERSION_NUM is defined.

This comment has been minimized.

@dvarrazzo

dvarrazzo Jun 2, 2015

Member

I'm not 100% sure we have pg_config.h available if only libpq-dev is installed. I'm fine having PG_VERSION_NUM only for now. I'll drop _HEX on top of your patches and merge it, thank you.

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Jun 1, 2015

I would love to see a test here, just to verify that the constant exists and is a number and the function exists and returns a number or, if it raises the exception, that the constant reports < 9.1. Thank you very much for the patch!

@a1exsh

This comment has been minimized.

Copy link
Contributor

a1exsh commented Jun 2, 2015

Good point, test added.

@dvarrazzo dvarrazzo merged commit ffd98a8 into psycopg:master Jun 2, 2015

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Jun 2, 2015

I did some adjustments to the docs, dropped the _HEX constant and merged. Now you may use it for the test in #321. Thank you very much!

@a1exsh a1exsh deleted the zalando-stups:feature/libpq-version branch Jun 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment