Skip to content
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

Fix #79532: sizeof off_t can be wrong #5482

Closed
wants to merge 4 commits into from
Closed

Conversation

cmb69
Copy link
Contributor

@cmb69 cmb69 commented Apr 28, 2020

We have to actually determine the proper SIZEOF_OFF_T.

We have to actually determine the proper `SIZEOF_OFF_T`.
@nikic
Copy link
Member

nikic commented Apr 28, 2020

Added test fails on travis.

@cmb69
Copy link
Contributor Author

cmb69 commented Apr 28, 2020

Thanks! Should be fixed now.

@nikic
Copy link
Member

nikic commented Apr 28, 2020

@cmb69 Including pg_config.h seems super fishy. It looks like libpq supports PQlibVersion() since 9.1, we should switch to using that instead of PG_VERSION.

@cmb69
Copy link
Contributor Author

cmb69 commented Apr 28, 2020

ACK. However, while we could construct PGSQL_LIBPQ_VERSION (a string) from PQlibVersion(), we cannot construct PGSQL_LIBPQ_VERSION_STR, which is documented as "Long libpq version that includes compiler information." (e.g. "PostgreSQL 11.4, compiled by Visual C++" ", 64-bit"). IOW, that is a (minor) BC break.

@nikic
Copy link
Member

nikic commented Apr 28, 2020

@cmb69 For now, please add an #undef SIZEOF_OFF_T before the pg_config.h includes, we can deal with removing them in master.

The clean solution would likely be to not include pg_config.h at all,
but that's out of scope for BC reasons for now.
@cmb69
Copy link
Contributor Author

cmb69 commented Apr 28, 2020

Okay, makes sense. I'll have a closer look at the PostgreSQL issue later.

@cmb69
Copy link
Contributor Author

cmb69 commented Apr 29, 2020

Thanks! Applied as 67f9b0b.

@cmb69 cmb69 closed this Apr 29, 2020
@cmb69 cmb69 deleted the cmb/79532 branch April 29, 2020 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants