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

Remove need for shared library PostgreSQL functions #2383

Merged
merged 5 commits into from Oct 20, 2019

Conversation

@zerebubuth
Copy link
Contributor

zerebubuth commented Oct 4, 2019

This repo contains code for three functions to be loaded into PostgreSQL as a shared library:

  1. maptile_for_point, which is used only by the /changes API call. This API call is little-used and IMHO should be deprecated and removed. However, even now it's hardly on the hot path for most development activities.
  2. tile_for_point, which is used only in migrations. At this point, it seems unlikely that anyone will be doing a migration on existing data which would call this function (most developers will be running migrations on an empty database, to set it up).
  3. xid_to_int4, which is only used for replication using Osmosis and isn't used in the Rails code at all. Hopefully this will be replaced Real Soon Now, but until then it's a quite advanced feature that most developers won't need.

Therefore, this patch proposes to replace the above three shared library functions with SQL implementations of the first two. These are much slower - by a factor of about 30x, however this makes no difference when they're run on a completely empty database. In return, we're able to drop a dependency on the PostgreSQL server development package, and clean a few lines out of the installation instructions.

It's still possible to make and install the shared library functions, and I've included instructions about how to do that - although it shouldn't be necessary for the vast majority of openstreetmap-website developers.

This repo contains code for three functions to be loaded into PostgreSQL as a shared library:

1. `maptile_for_point`, which is used only by the `/changes` API call. This API call is little-used and IMHO should be deprecated and removed. However, even now it's hardly on the hot path for most development activities.
2. `tile_for_point`, which is used only in migrations. At this point, it seems unlikely that anyone will be doing a migration on existing data which would call this function (most developers will be running migrations on an empty database, to set it up).
3. `xid_to_int4`, which is only used for replication using Osmosis and isn't used in the Rails code at all. Hopefully this will be replaced Real Soon Now, but until then it's a quite advanced feature that most developers won't need.

Therefore, this patch proposes to replace the above three shared library functions with SQL implementations of the first two. These are _much_ slower - by a factor of about 30x, however this makes no difference when they're run on a completely empty database. In return, we're able to drop a dependency on the PostgreSQL server development package, and clean a few lines out of the installation instructions.

It's still possible to make and install the shared library functions, and I've included instructions about how to do that - although it shouldn't be necessary for the vast majority of `openstreetmap-website` developers.
@zerebubuth zerebubuth requested a review from gravitystorm Oct 4, 2019
zerebubuth added 2 commits Oct 4, 2019
…ionally committed. (perhaps PG version difference?)
@mmd-osm

This comment has been minimized.

Copy link
Contributor

mmd-osm commented Oct 4, 2019

Good idea, I was about to propose the same, but haven't gotten to it. 👍 This would greatly simplify #2272 where those shared libs already caused quite some headache.

Also, agree on deprecating /changes. It's totally undocumented, and a pretty obscure feature.

Copy link
Collaborator

gravitystorm left a comment

This is great and I'm all for it. It's not only better for developers, but opens up new installation environments where users might not be able to load/compile shared libraries.

I've added some minor notes on the documentation inline - I'm just trying to streamline the first-install experience for new developers. Additionally:

  • I think it's worth adding a line to the "Production Deployment" section of CONFIGURE.md
  • I'd like @tomhughes to confirm if he's happy with this PR too.
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
@gravitystorm gravitystorm requested a review from tomhughes Oct 9, 2019
…d-library SQL explanations to 'advanced' documentation. Also mention in the CONFIGURING.md.
@@ -131,3 +131,5 @@ If you want to deploy The Rails Port for production use, you'll need to make a f
* The included version of the GPX importer is slow and/or completely inoperable. You should consider using [the high-speed GPX importer](https://git.openstreetmap.org/gpx-import.git/).
* Make sure you generate the i18n files and precompile the production assets: `RAILS_ENV=production rake i18n:js:export assets:precompile`
* Make sure the web server user as well as the rails user can read, write and create directories in `tmp/`.
* If you want to use diff replication then you will need to install the shared library versions of the special SQL functions (see the bottom of [INSTALL.md](INSTALL.md)).

This comment has been minimized.

Copy link
@mmd-osm

mmd-osm Oct 9, 2019

Contributor

This sounds a bit as if there was a non-shared library version of the xid_to_int4 function available, which isn't the case. To make this a bit clearer maybe state that diff replication depends on a function xid_to_int4, which is only being made available as a shared library. Additional bonus information would be to state that this function is only supported up to and including Postgres 9.5.

This comment has been minimized.

Copy link
@tomhughes

tomhughes Oct 9, 2019

Member

Well the point is that the xid_to_int4 function is something that as far as I know only we use and we would simply arrange to carry on using the 3GL version without it being part of the standard installation.

This comment has been minimized.

Copy link
@mmd-osm

mmd-osm Oct 9, 2019

Contributor

I wasn't even aware of a 3GL version, but you're right: osmosis ships one: https://github.com/openstreetmap/osmosis/blob/master/package/script/contrib/apidb_0.6_osmosis_xid_indexing.sql - but I guess the same limitations for USING btree ((xid_to_int4(xmin))); apply on 9.6+ as for the C-version, right?

This comment has been minimized.

Copy link
@zerebubuth

zerebubuth Oct 9, 2019

Author Contributor

To try and avoid confusion, I've changed that line in 7fb5bca to explicitly say there's no pure SQL version of xid_to_int4.

@grischard

This comment has been minimized.

Copy link
Contributor

grischard commented Oct 10, 2019

Just out of curiosity: what did /changes do?

@tomhughes

This comment has been minimized.

Copy link
Member

tomhughes commented Oct 10, 2019

It gets a list of tiles at a given zoom level that have changed in a given time period - by default if does z12 for the last hour but both can be changed.

It only looks at nodes though so it won't notice changes to ways or relations that cross a tile.

openbrian added a commit to openbrian/osm-microcosms that referenced this pull request Oct 18, 2019
…. When rake dumped the database they were removed from the file. Despite PR openstreetmap#2383, at this time, the statements should still be in the file.
openbrian added a commit to openbrian/osm-microcosms that referenced this pull request Oct 18, 2019
… manually. When rake dumped the database they were removed from the file. Despite PR openstreetmap#2383, at this time, the statements should still be in the file.
@mmd-osm mmd-osm referenced this pull request Oct 20, 2019
3 of 8 tasks complete
@tomhughes tomhughes merged commit 7fb5bca into openstreetmap:master Oct 20, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 87.95%
Details
jalessio added a commit to jalessio/openstreetmap-website that referenced this pull request Oct 20, 2019
jalessio added a commit to jalessio/openstreetmap-website that referenced this pull request Oct 20, 2019
@timwaters

This comment has been minimized.

Copy link

timwaters commented on 7fb5bca Oct 20, 2019

Could the xid_to_int4 function part of https://github.com/openstreetmap/osmosis/blob/master/package/script/contrib/apidb_0.6_osmosis_xid_indexing.sql be a pure sql alternative? I tried entering in a few numbers with this and with the C functions e.g. select xid_to_int4('123'::xid); and it appeared to produce the same result. I've no idea about performance though, nor did I delve into fully testing it, and it is 9 years old... Cheers :)

This comment has been minimized.

Copy link
Member

tomhughes replied Oct 20, 2019

It's totally irrelevant given that we're almost certainly the only users of it, and we're happy with the C version and in any case are looking to replace it.

This comment has been minimized.

Copy link
Collaborator

gravitystorm replied Oct 21, 2019

It's totally irrelevant given that we're almost certainly the only users of it, and we're happy with the C version and in any case are looking to replace it.

I know other organisations who (at least in the past) used the osmosis replication pipeline. So if that's a legit SQL version, it would be useful to incorporate it here too.

This comment has been minimized.

Copy link
Collaborator

gravitystorm replied Oct 21, 2019

@zerebubuth zerebubuth deleted the zerebubuth:no-shlib-pg-functions branch Oct 21, 2019
jalessio added a commit to jalessio/openstreetmap-website that referenced this pull request Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.