Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Sat, Mar 28, 2020 at 07:14:23PM +0100, Christoph Zwerschke wrote: > Am 28.03.2020 um 18:56 schrieb Justin Pryzby: > > Maybe that should be --with-ssl-info ? I found this: > > Not sure about that. In the context of setup.py and negative_opt, I only see > the 'no-' pattern being followed. Also, this is backward compatible. Okay. Better to be backwards compatible with itself than compatible with something else. -- Justin ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 28.03.2020 um 18:56 schrieb Justin Pryzby: > Maybe that should be --with-ssl-info ? I found this: Not sure about that. In the context of setup.py and negative_opt, I only see the 'no-' pattern being followed. Also, this is backward compatible. -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Sat, Mar 28, 2020 at 06:24:12PM +0100, Christoph Zwerschke wrote: > Am 28.03.2020 um 16:21 schrieb Justin Pryzby: > > Also, you have every option defaulting to try, with the only consequence of > > an > > unsupported PG version being a warning. I mentioned that packages (at > > least in > > the past for Debian) were encouraged to compile with --enable-foo > > --enable-bar.. > > ..such that a packages that was *expected* to compile again libfoo would > > fail > > loudly at the compile stage if it wasn't there or was somehow broken. > > Thanks for the quick feedback, Justin. > > I have addressed both of your points in > https://github.com/PyGreSQL/PyGreSQL/commit/03858234c9077ea7e492a4bbd0664b9fad74b70d python setup.py build_ext --ssl-info Maybe that should be --with-ssl-info ? I found this: https://sourceware.org/autobook/autobook/autobook_14.html |Using ‘--with-package=no’ is synonymous with ‘--without-package’ which is described below. -- Justin ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 28.03.2020 um 16:21 schrieb Justin Pryzby: Also, you have every option defaulting to try, with the only consequence of an unsupported PG version being a warning. I mentioned that packages (at least in the past for Debian) were encouraged to compile with --enable-foo --enable-bar.. ..such that a packages that was *expected* to compile again libfoo would fail loudly at the compile stage if it wasn't there or was somehow broken. Thanks for the quick feedback, Justin. I have addressed both of your points in https://github.com/PyGreSQL/PyGreSQL/commit/03858234c9077ea7e492a4bbd0664b9fad74b70d Better? -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Sat, Mar 28, 2020 at 03:50:57PM +0100, Christoph Zwerschke wrote: > Ok, I found that distutils even has a way to define "negative" options and > used it in setup.py now: > > Please have a look at: > https://github.com/PyGreSQL/PyGreSQL/commit/4b88231afd6bdfb40dca20780cdf006fe2346ea8 +self.escaping_funcs = True +self.ssl_info = True + If the installed PostgreSQL version does not support some features, you will + see a warning during installation, and these feature will not be enabled, so ... => I would change "during installation" to "during setup.py build phase" or something. To me, "installation" reads as "installing a binary package". Also, you have every option defaulting to try, with the only consequence of an unsupported PG version being a warning. I mentioned that packages (at least in the past for Debian) were encouraged to compile with --enable-foo --enable-bar.. ..such that a packages that was *expected* to compile again libfoo would fail loudly at the compile stage if it wasn't there or was somehow broken. -- Justin ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Ok, I found that distutils even has a way to define "negative" options and used it in setup.py now: Please have a look at: https://github.com/PyGreSQL/PyGreSQL/commit/4b88231afd6bdfb40dca20780cdf006fe2346ea8 Let me know what you think about this change. -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 25.03.2020 um 12:28 schrieb Justin Pryzby: I think (1) is more common, I think that's what autoconf and gnu tools use. In that case, maybe we should use enable/disable or with/without as prefixes, like autotools? But I think it's not very common as setup option. Also, by using the old option name and just adding the "no-" variant, we would stay backward compatible. -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Wed, Mar 25, 2020 at 12:25:12PM +0100, Christoph Zwerschke wrote: > Am 25.03.2020 um 10:34 schrieb Justin Pryzby: > > I think I would leave the "positive" variable names, and *add* negative > > forms > > of the commandline options. --no-foo and/or --foo=no > > 1) Either two options --escaping-funcs and --no-escaping-funcs, both without > arguments or > 2) one option --escaping funcs with a mandatory boolean argument > > In both cases, when we set self.escaping_funcs = None in initialize_options, > we can implement the ternary on/off/default. > > Not sure whether 1) or 2) is better, what do you think? I think (1) is more common, I think that's what autoconf and gnu tools use. -- Justin ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 25.03.2020 um 10:34 schrieb Justin Pryzby:
I think I would leave the "positive" variable names, and *add* negative forms
of the commandline options. --no-foo and/or --foo=no
Yes, that would be nice. Unfortunately, the options are not interpreted
by us, but by distutils. The way the options are currently defined in
PyGreSQL they act as booleans that do not take arguments, i.e. you can
set --foo, but not --foo=false. Unfortunately how the options are
interpreted is not well documented (at least I find nothing), so I had
to look into the Python source and found distutils uses getopt to parse
the raw options. Therefore it is possible to require arguments by
writing a "=" after the name of the argument in user_options, e.g.
('escaping-funcs=', None, "enable string escaping functions"),
When defined that way, we will be able to pass "--escaping-funcs=no" or
"--escaping-funcs false" as an option. When 'escaping-funcs' is added to
boolean_options, then the argument needs to be something like 0/false/no
or 1/true/yes and will be converted to a Python bool.
Unfortunately, getopt does not allow default arguments. So it looks like
we have to choose between:
1) Either two options --escaping-funcs and --no-escaping-funcs, both
without arguments or
2) one option --escaping funcs with a mandatory boolean argument
In both cases, when we set self.escaping_funcs = None in
initialize_options, we can implement the ternary on/off/default.
Not sure whether 1) or 2) is better, what do you think?
-- Christoph
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Wed, Mar 25, 2020 at 10:08:25AM +0100, Christoph Zwerschke wrote:
>
> Am 25.03.2020 um 02:20 schrieb Justin Pryzby:
> > The old stuff can be removed eventually. I like this:
> >
> > if self.escaping_funcs and pg_version >= (9, 0):
> > define_macros.append(('ESCAPING_FUNCS', None))
> > That allows a packager to disable a feature
> > if they want the abilityto run> against old libpq.
>
> The problem is that these switches are by default set to true. The idea was
> that you set them to false on the command line if you don't want these
> features to be compiled. However, I think that never worked since you simply
> can't set these flags to false on the command line, you only can set them to
> true by specifying the names of the options (i.e. pass --escaping-funcs as
> option).
>
> So we need to change that and instead use the negatives of these flags
> everywhere as options i.e. "no_escaping_funcs" in this case.
I think I would leave the "positive" variable names, and *add* negative forms
of the commandline options. --no-foo and/or --foo=no
Also, I believe it used to be a best-practice for distribution packagers to
specify --with-foo --with-bar --with-baz even if those were the defaults (or if
they defaulted to true if libfoo/bar/baz were installed). That avoids silently
building packages with reduced functionality if libfoo isn't found at compile
time. So logic like this might be nice:
if self.enable_memsize and pg_version < (12, 0):
error('memsize not supported until pg12')
postgres vacuum options have a ternary system: on/off/default. If you need
more than a line of logic, maybe you'd write:
if self.enable_memsize is True and pg_version < (12, 0):
error('memsize not supported until pg12')
else if self.enable_memsize == -1: # default/unset
self.enable_memsize = bool(pg_version >= (12, 0):)
...
if self.enable_memsize is True:
define_macros.append(('WITH_MEMSIZE', None))
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 25.03.2020 um 02:20 schrieb Justin Pryzby:
> The old stuff can be removed eventually. I like this:
>
> if self.escaping_funcs and pg_version >= (9, 0):
> define_macros.append(('ESCAPING_FUNCS', None))
> That allows a packager to disable a feature
> if they want the abilityto run> against old libpq.
The problem is that these switches are by default set to true. The idea
was that you set them to false on the command line if you don't want
these features to be compiled. However, I think that never worked since
you simply can't set these flags to false on the command line, you only
can set them to true by specifying the names of the options (i.e. pass
--escaping-funcs as option).
So we need to change that and instead use the negatives of these flags
everywhere as options i.e. "no_escaping_funcs" in this case. I usually
avoid negative forms since "Can I not have my coffee with no sugar"
sounds silly when I can say "Can I have coffee with sugar". But in the
case where "sugar" is always the default, it's ok, since you only need
to say "no sugar please" and never need to ask for sugar explicitly. (I
hope this was not confusing :)
-- Christoph
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Tue, Mar 24, 2020 at 09:10:18AM +0100, Christoph Zwerschke wrote:
> Maybe where we release binaries (currently only for Windows) we could ship
> them with a matching libql dll that will be installed with Pip alongside the
> PyGres dll?
I thinnk that would require the windows version of rpath which (depending on
who you ask) is discouraged.
> Or maybe link the pqlib statically into the PyGres dll in that
> case? This would make it even more convenient for Windows users since they
> don't need to install Postgres.
+1
Here, you talked about various compile-time switches.
https://github.com/PyGreSQL/PyGreSQL/issues/12
I guess it's good to continue supporting those, at least for newly-added
symbols that we *know* aren't in old releases. What's out there seems to be
working for everyone.
The old stuff can be removed eventually. I like this:
if self.escaping_funcs and pg_version >= (9, 0):
define_macros.append(('ESCAPING_FUNCS', None))
That allows a packager to disable a feature if they want the ability to run
against old libpq.
--
Justin
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 24.03.2020 um 05:46 schrieb Justin Pryzby: This patch got stuck due to our limited understand of python's dynamic loading of shared libraries. I picked it up again ince I have some downtime.. Thanks for working on this problem. Would be great if we could resolve it. Otherwise it always hinders us supporting the latest features. > Since it's BIND_NOW, it means that any python program linked to > pygres willsimply fail at startup in the unlikely even that it was > compiled against libpq for PG12 but runs against an lib older It's unlikely when you compile yourself, but Windows users normally don't do that since they have no compiler. So if we ship a version that was compiler against Pg12, it will not run for users with Pg11 installed on their machine. On the other hand, Windows DLL work a little bit differently anyway, maybe we can find a different solution there. Maybe where we release binaries (currently only for Windows) we could ship them with a matching libql dll that will be installed with Pip alongside the PyGres dll? Or maybe link the pqlib statically into the PyGres dll in that case? This would make it even more convenient for Windows users since they don't need to install Postgres. -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
This patch got stuck due to our limited understand of python's dynamic loading
of shared libraries. I picked it up again ince I have some downtime..
I think that way it's supposed to work, for an OS distribution, is that, at
compile time, whatever library is linked to, its SONAME is used, but also, its
*minor* version is captured by the distribution tools, and creates a package
dependency on library>=minorversion. That means if you install a new version
of some tool, it might pull in a new version of a library, even if that's a
minor version. Or it might pull in a totally new library package, since
distributions usually allow simultaneously installing libfoo1 (major=1) and
libfoo2 (major=2) to allow rolling transitions. The debian tool that handles
that is dpkg-shlibdeps, and I know RHEL has something similar.
I don't see how that can work with pip, though..
It looks like python uses BIND_NOW when it dlopen's a library, like when pygres
does "from _pg import *". Maybe because it *can't* work with pip.
I was able to make this work as I originally intended like:
$ PYTHONPATH=build/lib.linux-x86_64-3.5 python3 -c "import sys;
sys.setdlopenflags(1); import pg; print(pg.DB('postgres').query('SELECT
generate_series(1,)').memsize())"
None
sys says:
|Among other things, this will enable a lazy resolving of symbols when
importing a module, if called as sys.setdlopenflags(0).
But that didn't work for me.. Also, ctypes say:
|On posix systems, RTLD_NOW is always added, and is not configurable.
In any case, I don't think calling sys modules before importing _pg is a good
solution. So I think the "solution" is to use a compile time check only, and
assume that it won't be run against an older version. (And if need be, wait
until the new symbol is common enough that's a nonissue).
Since it's BIND_NOW, it means that any python program linked to pygres will
simply fail at startup in the unlikely even that it was compiled against libpq
for PG12 but runs against an lib older than 12. It doesn't run for awhile and
then crash when the function is eventually called, which is bad.
On Sat, Oct 05, 2019 at 11:04:35AM -0500, Justin Pryzby wrote:
> On Sat, Oct 05, 2019 at 04:48:40PM +0200, Christoph Zwerschke wrote:
> > >Find attached minimal patch.
>
> I thought it ought to have compile time check (for headers and link library
> version) and runtime check (for runtime lib version), but, now, even *with* my
> runtime library check, this happens if pygres is run against an earlier
> version
> of libpq:
>
> |$ PYTHONPATH=build/lib.linux-x86_64-2.7/ python2.7 -c "import pg; print
> pg.DB('postgres').query('SELECT generate_series(1,)').memsize()"
> |Traceback (most recent call last):
> | File "", line 1, in
> | File "pg.py", line 27, in
> |from _pg import *
> |ImportError: /home/pryzbyj/src/pygres/build/lib.linux-x86_64-2.7/_pg.so:
> undefined symbol: PQresultMemorySize
>
> ..so maybe my runtime check is useless, or maybe libpq isn't doing proper
> library versioning? I've forgotten whatever I once knew about this.
>
> $ ldd build/lib.linux-x86_64-2.7/_pg.so |grep libpq
> libpq.so.5 => /usr/lib/x86_64-linux-gnu/libpq.so.5
> (0x7fb3f652a000)
On Sat, Oct 05, 2019 at 11:47:40AM -0500, Justin Pryzby wrote:
> On Sat, Oct 05, 2019 at 06:37:36PM +0200, Christoph Zwerschke wrote:
> > >|$ PYTHONPATH=build/lib.linux-x86_64-2.7/ python2.7 -c "import pg; print
> > >pg.DB('postgres').query('SELECT generate_series(1,)').memsize()"
> > >|Traceback (most recent call last):
> > >| File "", line 1, in
> > >| File "pg.py", line 27, in
> > >|from _pg import *
> > >|ImportError: /home/pryzbyj/src/pygres/build/lib.linux-x86_64-2.7/_pg.so:
> > >undefined symbol: PQresultMemorySize
> >
> > Are you sure you compiled that with the pgconfig.h of Postgres 12? setup.py
> > is using the includes specified by the pgconfig tool.
>
> The intended behavior is to hit the runtime version check and then return
> None.
> To check, I compiled against pg12 and then downgraded libpq to v10.
>
> Built like:
> PATH=/usr/lib/postgresql/12/bin:$PATH ./setup.py build
>
> If I hadn't, then it wouldn't have referenced PQresultMemorySize at all,
> right ?
On Sat, Oct 05, 2019 at 01:53:23PM -0500, Justin Pryzby wrote:
> On Sat, Oct 05, 2019 at 07:47:50PM +0200, Christoph Zwerschke wrote:
> > Am 05.10.2019 um 18:47 schrieb Justin Pryzby:
> > > On Sat, Oct 05, 2019 at 06:37:36PM +0200, Christoph Zwerschke wrote:
> > >> Are you sure you compiled that with the pgconfig.h of Postgres 12?
> > >> setup.py is using the includes specified by the pgconfig tool.
> > > The intended behavior is to hit the runtime version check and then
> > > return None.
> > > To check, I compiled against pg12 and then downgraded libpq to v10.
> > > Built like: PATH=/usr/lib/postgresql/12/bin:$PATH ./setup.py build
> > > If I hadn't, then it wouldn't have referenced PQresultMemorySize at
> > > all, right ?
> >
> > Ah, yes, you're
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 06.10.2019 um 21:39 schrieb Justin Pryzby: > That doesn't explain it :( > > Find attached t.c building t.so... Thanks. At least we have ruled that out :/ Winter is coming and maybe we can find some rainy weekend to investigate deeper and learn a thing or two about the sacred science of building shared libraries... ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Sun, Oct 06, 2019 at 05:57:38PM +0200, Christoph Zwerschke wrote:
> Am 06.10.2019 um 15:20 schrieb Justin Pryzby:
> > Remember, lazy binding is the default on my machine, and my patch
> > doesn't DWIW here.. So I don't think adding lazy link options
> > is going to help. I think because of something python is doing (??)
>
> Another difference is that in your example you're using libpq as shared lib
> directly, whereas with Python you create a shared lib that then uses libpq
> as a shared lib. Maybe it has something to do with that?
That doesn't explain it :(
Find attached t.c building t.so...
$ ldd t |sed 3q
linux-vdso.so.1 => (0x7ffd3fd32000)
t.so => not found
libpq.so.5 => /usr/lib/x86_64-linux-gnu/libpq.so.5 (0x7f27a8ba6000)
$ PGDATABASE=postgres LD_LIBRARY_PATH=. ./t 0
$ PGDATABASE=postgres LD_LIBRARY_PATH=. ./t 1
./t: symbol lookup error: ./t.so: undefined symbol: PQresultMemorySize
// sh -exc 'gcc -Wall -Wextra -O3 -g -I /usr/include/postgresql -o t.so -shared -fPIC -DSHARED=1 t.c && gcc -Wall -Wextra -O3 -g -o t -DSHARED=0 t.c t.so -lpq'
// $ PGDATABASE=postgres LD_LIBRARY_PATH=. ./t 1
#if SHARED
#include
#include
#include
void f(char **argv)
{
PGconn *conn = PQconnectdb("");
if (PQstatus(conn) != CONNECTION_OK)
exit(1);
PGresult *res = PQexec(conn, "SELECT generate_series(1,9)");
if (argv[1][0] == '1')
printf("%zd\n", PQresultMemorySize(res));
}
#else
void f(char **argv);
int main(int argc, char **argv)
{
f(argv);
return 0;
}
#endif // SHARED
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 06.10.2019 um 15:20 schrieb Justin Pryzby: > Remember, lazy binding is the default on my machine, and my patch > doesn't DWIW here.. So I don't think adding lazy link options > is going to help. I think because of something python is doing (??) Another difference is that in your example you're using libpq as shared lib directly, whereas with Python you create a shared lib that then uses libpq as a shared lib. Maybe it has something to do with that? I kept the GitHub issue open as a reminder, and also created a Trac ticket referencing it. -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Sun, Oct 06, 2019 at 01:03:21PM +0200, Christoph Zwerschke wrote: > Am 06.10.2019 um 01:15 schrieb Justin Pryzby: > >On Sat, Oct 05, 2019 at 11:43:23PM +0200, Christoph Zwerschke wrote: > >..but I don't think it's a solution for pygres ? I tried it and still get a > >symbol resolution error. So, unless someone knows or finds better, it means > >there's nothing stopping installing of pygres compiled against one library > >with > >an earlier library, even though that combination might be broken. > > > >My #ifs and ifs don't help, and I suspect your setup.py stuff doesn't either > >(?) > > When I tried building with setup.py, I noticed that Python links the shared > lib in a separate step after the compilation. Options for this second step > must be set in extra_link_args, not in extra_compile_args. > I also noticed in the output that the shared lib is linked with the options > -Wl,-Bsymbolic-functions and -Wl,-z,relro. Maybe this is what defeats the > lazy binding? > > Unfortunately. adding the options for lazy loading to extra_link_args, > CFLAGS or LDFLAGS did not help, maybe because they are only appended (added) > to the options above, and only the first option is effective? Remember, lazy binding is the default on my machine, and my patch doesn't DWIW here.. So I don't think adding lazy link options is going to help. I think because of something python is doing (??) Justin ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 06.10.2019 um 01:15 schrieb Justin Pryzby: On Sat, Oct 05, 2019 at 11:43:23PM +0200, Christoph Zwerschke wrote: ..but I don't think it's a solution for pygres ? I tried it and still get a symbol resolution error. So, unless someone knows or finds better, it means there's nothing stopping installing of pygres compiled against one library with an earlier library, even though that combination might be broken. My #ifs and ifs don't help, and I suspect your setup.py stuff doesn't either (?) When I tried building with setup.py, I noticed that Python links the shared lib in a separate step after the compilation. Options for this second step must be set in extra_link_args, not in extra_compile_args. I also noticed in the output that the shared lib is linked with the options -Wl,-Bsymbolic-functions and -Wl,-z,relro. Maybe this is what defeats the lazy binding? Unfortunately. adding the options for lazy loading to extra_link_args, CFLAGS or LDFLAGS did not help, maybe because they are only appended (added) to the options above, and only the first option is effective? Running out of time, need to investigate when I find some time again. Or maybe someone with more knowledge of these things can explain what's going on here and how we can avoid the "undefined symbol" errors when using an older libpq version. -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Sat, Oct 05, 2019 at 11:43:23PM +0200, Christoph Zwerschke wrote: > Am 05.10.2019 um 23:10 schrieb Christoph Zwerschke: > >Just tried what you did with that t.c program, but strangely I get the > >"undefined symbol" error when I downgrade libpq and run "./t 0", even when > >LD_BIND_NOW is not defined. I am using Ubuntu instead of Debian, but that > >shouldn't matter. > > Ok, found the problem - I needed to explicitly add the "-Wl,-z,lazy" option > when compiling t.c to make it use lazy binding. For some strange reason, it > does not seem to be the default. > > Maybe we need to add this to the extra_compile_args in the setup.py file as > well? It's intriguing that it's not the default on your machine.. (Maybe you can debug it some with nm -A |grep PQresultMemorySize or ldd or LD_DEBUG) ..but I don't think it's a solution for pygres ? I tried it and still get a symbol resolution error. So, unless someone knows or finds better, it means there's nothing stopping installing of pygres compiled against one library with an earlier library, even though that combination might be broken. My #ifs and ifs don't help, and I suspect your setup.py stuff doesn't either (?) Justin ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 05.10.2019 um 23:33 schrieb Justin Pryzby: > You probably realized that my email failed to show that I > downgraded libpq between invocacations of "PGDATABASE=postgres ./t 1" Yes, I understood what you were doing. But I could only reproduce it when adding the "-Wl,-z,lazy" option. Not sure why it's not the default on my machine. -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 05.10.2019 um 23:10 schrieb Christoph Zwerschke: Just tried what you did with that t.c program, but strangely I get the "undefined symbol" error when I downgrade libpq and run "./t 0", even when LD_BIND_NOW is not defined. I am using Ubuntu instead of Debian, but that shouldn't matter. Ok, found the problem - I needed to explicitly add the "-Wl,-z,lazy" option when compiling t.c to make it use lazy binding. For some strange reason, it does not seem to be the default. Maybe we need to add this to the extra_compile_args in the setup.py file as well? -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Sat, Oct 05, 2019 at 07:47:50PM +0200, Christoph Zwerschke wrote:
>
> Am 05.10.2019 um 18:47 schrieb Justin Pryzby:
> > On Sat, Oct 05, 2019 at 06:37:36PM +0200, Christoph Zwerschke wrote:
> >> Are you sure you compiled that with the pgconfig.h of Postgres 12?
> >> setup.py is using the includes specified by the pgconfig tool.
> > The intended behavior is to hit the runtime version check and then
> > return None.
> > To check, I compiled against pg12 and then downgraded libpq to v10.
> > Built like: PATH=/usr/lib/postgresql/12/bin:$PATH ./setup.py build
> > If I hadn't, then it wouldn't have referenced PQresultMemorySize at
> > all, right ?
>
> Ah, yes, you're right. The runtime checks are useless because you can't even
> load a shared library that doesn't support all the symbols that you are
> referencing somewhere.
In general, that's false, due to lazy binding (man ld.so: /LD_BIND_NOW/)
...but maybe true for python's c modules ??
$ sudo dpkg -i /var/cache/apt/archives/libpq{5,-dev}_12*deb
$ make CFLAGS="-Wall -Wextra -O3 -g -I /usr/include/postgresql" LDLIBS=-lpq t
$ PGDATABASE=postgres ./t 1
3459288
$ PGDATABASE=postgres ./t 1
./t: symbol lookup error: ./t: undefined symbol: PQresultMemorySize
$ PGDATABASE=postgres ./t 0 && echo OK
OK
$ LD_BIND_NOW=t PGDATABASE=postgres ./t 0
./t: symbol lookup error: ./t: undefined symbol: PQresultMemorySize
Justin
// make CFLAGS="-Wall -Wextra -O3 -g -I /usr/include/postgresql" LDLIBS=-lpq t
#include
#include
#include
int main(int argc, char **argv)
{
PGconn *conn = PQconnectdb(""); // PGDATABASE dbname=postgres");
if (PQstatus(conn) != CONNECTION_OK)
exit(1);
PGresult *res = PQexec(conn, "SELECT generate_series(1,9)");
if (argv[1][0] == '1')
printf("%zd\n", PQresultMemorySize(res));
return 0;
}
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 05.10.2019 um 18:47 schrieb Justin Pryzby: > On Sat, Oct 05, 2019 at 06:37:36PM +0200, Christoph Zwerschke wrote: >> Are you sure you compiled that with the pgconfig.h of Postgres 12? >> setup.py is using the includes specified by the pgconfig tool. > The intended behavior is to hit the runtime version check and then > return None. > To check, I compiled against pg12 and then downgraded libpq to v10. > Built like: PATH=/usr/lib/postgresql/12/bin:$PATH ./setup.py build > If I hadn't, then it wouldn't have referenced PQresultMemorySize at > all, right ? Ah, yes, you're right. The runtime checks are useless because you can't even load a shared library that doesn't support all the symbols that you are referencing somewhere. That's actually the reason why I proposed supporting a "compatible version" option when building PyGreSQL, so that it's possible to create PyGreSQL binaries that can be used with older libpq versions. -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 05.10.2019 um 18:04 schrieb Justin Pryzby:
On Sat, Oct 05, 2019 at 04:48:40PM +0200, Christoph Zwerschke wrote:
Find attached minimal patch.
Thank you, looks fine. I've already added docs and tests as well, but not
yet committed to trunk.
I didn't mean you had to stop and do that - I hammered it out while waiting for
pg_checksums to run :)
And I just had some spare time and was working on PyGres anyway :)
Good idea. But I wonder whether it should be a property, which you suggested
before for types of queryResult columns (for which I sent a WIP patches July 19
and 20).
I thought about that. But this is a module level function and there are
no module level properties in Python. Maybe we could use PEP 549 or
PEP562, but these require Python 3.7. We could provide it as an
attribute but that means we need to initialize it by making a call once
when importing.
|$ PYTHONPATH=build/lib.linux-x86_64-2.7/ python2.7 -c "import pg; print
pg.DB('postgres').query('SELECT generate_series(1,)').memsize()"
|Traceback (most recent call last):
| File "", line 1, in
| File "pg.py", line 27, in
|from _pg import *
|ImportError: /home/pryzbyj/src/pygres/build/lib.linux-x86_64-2.7/_pg.so:
undefined symbol: PQresultMemorySize
Are you sure you compiled that with the pgconfig.h of Postgres 12?
setup.py is using the includes specified by the pgconfig tool.
-- Christoph
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Sat, Oct 05, 2019 at 04:48:40PM +0200, Christoph Zwerschke wrote:
> >Find attached minimal patch.
>
> Thank you, looks fine. I've already added docs and tests as well, but not
> yet committed to trunk.
I didn't mean you had to stop and do that - I hammered it out while waiting for
pg_checksums to run :)
Also, it occurred to me this "else" is unnecessary:
+#if PG_VERSION_NUM >= 12 /* Compile time version */
+if (PQlibVersion() >= 12) /* Runtime version */
+return PyLong_FromSize_t(PQresultMemorySize(self->result));
+else
> In addition to query.memsize() I've also added get_pqlib_version() as a module
> level function.
Good idea. But I wonder whether it should be a property, which you suggested
before for types of queryResult columns (for which I sent a WIP patches July 19
and 20).
> But I wonder if we should create a new minor version (5.2) instead of patch
> release (5.1.1) since it's new functionality?
It seems to me the usual procedure is to either issue a security release with
no other changes at all (not for py37, not for pg12, etc). Or include the
security update with the next release. FWIW, your schema-qualification commit
is already out there for anyone who wants it, and the CVE is from last year.
But, that's not an important patch; I don't think we'll use it at telsasoft
(preferring iterators); I think 5.2 should have more than a trivial new
feature.
> Another thing I'd also like to address is the overall handling of
> functionality that is optional or does only work with newer pqlib versions.
> Currently these are behind compiler switches which are set in setup.py, by
> default they are enabled if the pqlib version is new enough to support the
> respective functionality.
I think you're referring to:
$ grep -h '#if' *c |sort |uniq -c |sort -nr
... LARGE_OBJECTS DEFAULT_VARS ESCAPING_FUNCS DIRECT_ACCESS SSL_INFO NO_PQSOCKET
> Alternatively, the pqlib version could be checked directly using pg_config.h
> in the code instead of using the various switches,
> like you are doing here. I'd like to handle this more consistently through
> the code. But this would also warrant a new 5.2 version:
> see https://github.com/Cito/PyGreSQL/issues/12
I thought it ought to have compile time check (for headers and link library
version) and runtime check (for runtime lib version), but, now, even *with* my
runtime library check, this happens if pygres is run against an earlier version
of libpq:
|$ PYTHONPATH=build/lib.linux-x86_64-2.7/ python2.7 -c "import pg; print
pg.DB('postgres').query('SELECT generate_series(1,)').memsize()"
|Traceback (most recent call last):
| File "", line 1, in
| File "pg.py", line 27, in
|from _pg import *
|ImportError: /home/pryzbyj/src/pygres/build/lib.linux-x86_64-2.7/_pg.so:
undefined symbol: PQresultMemorySize
..so maybe my runtime check is useless, or maybe libpq isn't doing proper
library versioning? I've forgotten whatever I once knew about this.
$ ldd build/lib.linux-x86_64-2.7/_pg.so |grep libpq
libpq.so.5 => /usr/lib/x86_64-linux-gnu/libpq.so.5 (0x7fb3f652a000)
Justin
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 05.10.2019 um 02:07 schrieb Justin Pryzby: On Wed, May 15, 2019 at 08:44:49AM -0500, Justin Pryzby wrote: Also, as of PG12 (unreleased), there'll be this new interface: https://www.postgresql.org/docs/devel/release-12.html#id-1.3.7 |Add libpq function to report the memory size of the query result (Lars Kanis, Tom Lane) |The function is PQresultMemorySize(). I'd propose to include that with the first release following PG12...although I think you'd have to check PG_VERSION_NUM or similar, which I see isn't currently being done. Find attached minimal patch. Thank you, looks fine. I've already added docs and tests as well, but not yet committed to trunk. In addition to query.memsize() I've also added get_pqlib_version() as a module level function. But I wonder if we should create a new minor version (5.2) instead of patch release (5.1.1) since it's new functionality? Another thing I'd also like to address is the overall handling of functionality that is optional or does only work with newer pqlib versions. Currently these are behind compiler switches which are set in setup.py, by default they are enabled if the pqlib version is new enough to support the respective functionality. Alternatively, the pqlib version could be checked directly using pg_config.h in the code instead of using the various switches, like you are doing here. I'd like to handle this more consistently through the code. But this would also warrant a new 5.2 version: see https://github.com/Cito/PyGreSQL/issues/12 -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
[PyGreSQL] [PATCH] PQresultMemorySize
On Wed, May 15, 2019 at 08:44:49AM -0500, Justin Pryzby wrote:
> Also, as of PG12 (unreleased), there'll be this new interface:
> https://www.postgresql.org/docs/devel/release-12.html#id-1.3.7
> |Add libpq function to report the memory size of the query result (Lars
> Kanis, Tom Lane)
> |The function is PQresultMemorySize().
>
> I'd propose to include that with the first release following PG12...although I
> think you'd have to check PG_VERSION_NUM or similar, which I see isn't
> currently being done.
Find attached minimal patch.
$ PYTHONPATH=build/lib.linux-x86_64-2.7/ python2.7 -c "import pg; print
pg.DB('postgres').query('SELECT generate_series(1,)').memsize()"
372952
Index: pgmodule.c
===
--- pgmodule.c (revision 1019)
+++ pgmodule.c (working copy)
@@ -17,6 +17,7 @@
#include
#include
+#include
/* The type definitions from */
#include "pgtypes.h"
Index: pgquery.c
===
--- pgquery.c (revision 1019)
+++ pgquery.c (working copy)
@@ -11,6 +11,20 @@
*
*/
+static char query_memsize__doc__[] =
+"memsize() -- return number of bytes allocated by query";
+static PyObject *
+query_memsize(queryObject *self)
+{
+#if PG_VERSION_NUM >= 12 /* Compile time version */
+if (PQlibVersion() >= 12) /* Runtime version */
+return PyLong_FromSize_t(PQresultMemorySize(self->result));
+else
+#endif
+Py_INCREF(Py_None);
+return Py_None;
+}
+
/* Deallocate the query object. */
static void
query_dealloc(queryObject *self)
@@ -707,13 +788,21 @@
METH_VARARGS, query_fieldnum__doc__},
{"listfields", (PyCFunction) query_listfields,
METH_NOARGS, query_listfields__doc__},
{"ntuples", (PyCFunction) query_ntuples,
METH_NOARGS, query_ntuples__doc__},
+{"memsize", (PyCFunction) query_memsize,
+METH_NOARGS, query_memsize__doc__},
{NULL, NULL}
};
static char query__doc__[] = "PyGreSQL query object";
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
