zerebubuth/openstreetmap-cgimap

Compatibility with libpqxx 7.x.x

Closed this issue · 6 comments

Woazboat commented

libpqxx has changed quite a bit with version 7.x.x and above that makes it currently incompatible with cgimap

https://github.com/jtv/libpqxx/blob/master/NEWS

I've made a few changes that allow it to compile again here:
master...Woazboat:openstreetmap-cgimap:libpqxx-7-compatibility

Unfortunately that's not quite enough to allow cgimap to function properly again.
Currently, almost all prepared SQL statements are defined directly in the functions that call them and they are redefined with every call.

e.g.

int readonly_pgsql_selection::select_nodes(const std::vector<osm_nwr_id_t> &ids) {
  if (ids.empty())
    return 0;

  m.prepare("select_nodes", "SELECT id FROM current_nodes WHERE id = ANY($1)");

  return insert_results(m.exec_prepared("select_nodes", ids), sel_nodes);
}

In libpqxx 6.x this worked 'fine', however with libpqxx 7.x a redefinition of a prepared statement with the same identifier now causes an exception. This needs to be changed across the board so that all prepared statements are defined once only at the beginning and aren't redefined with every function call.

(Even if this didn't cause an error, it should be changed purely for performance reasons as redefining all prepared statements with every function call is unnecessary.)

mmd-osm commented

The way this works in libpqxx 6.x is that they’re using some caching and only prepare statements once, even if you call prepare multiple times. They're raising an exception only if you're trying to change the actual SQL statement from one call to the other. In other words, prepare behaves like a prepare_once kind of method. Maybe we could rename the wrapper method to make this a bit clearer.

From your description it seems this caching is no longer available (jtv/libpqxx#428 has some more discussion on the topic). Since we’re calling our own wrapper of prepare we could simply add that caching bit on connection level there, rather than changing everything. I need to check this in more detail.

I'm moving your proposed changes over to this branch, including compile time switches: https://github.com/mmd-osm/openstreetmap-cgimap/commits/patch/pqxx7

Unit tests are passing again now. There's a bit of warnings b/c libpqxx has changed the definition of pqxx::result::size_type across versions. I'll leave those for later.

Woazboat commented

Thanks for fixing this!

I still get some linker errors when I try to run make check. I had to change the build system to also link in libpqxx for some more of the tests: 2a8a7b6

make check

Making check in src
make[1]: Entering directory '/home/user/openstreetmap-cgimap/src'
make  ../test/test_core ../test/test_apidb_backend_nodes ../test/test_apidb_backend_oauth ../test/test_apidb_backend_oauth2 ../test/test_apidb_backend_historic ../test/test_apidb_backend_changesets ../test/test_apidb_backend_changeset_downloads ../test/test_apidb_backend_changeset_uploads ../test/test_parse_id_list ../test/test_basicauth ../test/test_oauth ../test/test_oauth2 ../test/test_http ../test/test_parse_time ../test/test_parse_osmchange_input ../test/test_parse_changeset_input
make[2]: Entering directory '/home/user/openstreetmap-cgimap/src'
  CXXLD    ../test/test_core
/usr/bin/ld: ../test/test_core.o: in function `_GLOBAL__sub_I__Z12read_headersRSiRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE':
/usr/include/pqxx/strconv.hxx:82:(.text.startup+0x24): undefined reference to `pqxx::internal::demangle_type_name[abi:cxx11](char const*)'
/usr/bin/ld: ../test/test_core.o: in function `__static_initialization_and_destruction_0':
/usr/include/pqxx/strconv.hxx:82:(.text.startup+0x48): undefined reference to `pqxx::internal::demangle_type_name[abi:cxx11](char const*)'
/usr/bin/ld: /usr/include/pqxx/strconv.hxx:82:(.text.startup+0x87): undefined reference to `pqxx::internal::demangle_type_name[abi:cxx11](char const*)'
/usr/bin/ld: /usr/include/pqxx/strconv.hxx:82:(.text.startup+0xab): undefined reference to `pqxx::internal::demangle_type_name[abi:cxx11](char const*)'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:1289: ../test/test_core] Error 1
mmd-osm commented

I've seen this change in your libpqxx-7-compatibility branch, but cannot reproduce here (also see the Github actions log file). Which environment does this happen in?

Since this is impacting unit tests only, I've cherry picked 2a8a7b6.. Can you please try again with latest master now?

Woazboat commented

I'm running debian testing. Not sure why that would lead to different libraries being linked in.

package version
g++ 4:13.2.0-1
libpqxx-dev 7.8.1-2
libxml2-dev 2.9.14+dfsg-1.3
libfcgi-dev 2.4.2-2
zlib1g-dev 1:1.2.13.dfsg-3
libboost-dev 1.74.0.3
libboost-program-options-dev 1.74.0.3
libfmt-dev 9.1.0+ds1-2
libmemcached-dev 1.1.4-1
libcrypto++-dev 8.9.0-1
libargon2-dev 0~20190702+dfsg-4
libyajl-dev 2.1.0-5
mmd-osm commented

I'm using --enable-static --disable-shared as configure options in my Dockerfile, although for some issues with fmt. Not sure if this might have some impact here: https://github.com/zerebubuth/openstreetmap-cgimap/blob/master/Dockerfile_trixie#L23

Woazboat commented

Ah, yes I think the static linking would explain that. When I compile it without the build system changes but with static linking it works. With the linker changes it works for both dynamic and static linking.

  • 👍1