Skip to content

Conversation

@mgcuthbert
Copy link
Contributor

This PR adds database locking functionality for Osmosis. The locking is quite simplistic allowing read and write locks on a first come first serve basis. Multiple read locks can be obtained and only a single write lock may be obtained. This allows the any incoming read requests from external processes on your database to wait until some write operation has completed before executing the read. It does assume any external read operations will use the locking functionality as it doesn't use database level locking. This does give a little more flexibility in how you use it. If the database that you are replicating data into does not have the required locking functions it will work exactly as it did before, as if there is no locking.

@migurski
Copy link
Collaborator

migurski commented Jun 5, 2020

Great functionality! Is there an API change introduced to support this, or is it implicit based on the underlying data store?

@mmd-osm
Copy link

mmd-osm commented Jun 5, 2020

Was it a deliberate choice to have those new stored procedures in pgsnapshot_schema_0.6_changes.sql only? Is this functionality limited to pgsnapshot?

I have to say that this stored procedure approach seems just a little bit over-engineered, ymmv. Also, any DML level changes would typically need to go via the respective owner of the schema, e.g. the Rails port in case of an API db. Changes outside of the established db migration schema are usually discouraged.

By the way, what was the original issue this is trying to solve?

@mgcuthbert
Copy link
Contributor Author

@migurski So there is no API changes required for this, the underlying database essentially defines if it is enabled or not.

@mmd-osm So the new stored procedures were placed in the pgsnapshot_schema_0.6_changes.sql primarily due to that is where tests exist, although the tests could just as easily be built into the core plugin. This functionality is not limited to the pgsnapshot, it is essentially built at the base layer, so not something specific to pgsnapshot, or really any of the plugins. There probably seems like a lot around that particular module due to the initial use case and what was trying to be solved.

As to that question, the locking essentially stops anyone body from say extracting PBF data from the database while replication is going on. We found that if those 2 processes run at the same time your PBF extract could return somewhat invalid data. Locking allows the PBF extract to either finish before replication continues, or for the PBF extract to fail immediately if replication is currently executing.

@mmd-osm
Copy link

mmd-osm commented Jun 8, 2020

Thanks for your detailed feedback. One thing I’m wondering about is why you don’t use a single database transaction with proper isolation level for all of your queries in your PBF extraction task. I’m assuming that applying a diff update would always run in a single database transaction, so you shouldn’t see inconsistent data in this case.

@mgcuthbert
Copy link
Contributor Author

So yes that was something I briefly thought about. Firstly the pbf extraction task uses Osmosis, so this would be required to be built into Osmosis, certainly not impossible, but a little more challenging. As for the diff update running in a single transaction, this is correct I believe it does exactly that making sure that the data in the database remains consistent. However if you run a very large pbf extract it could take multiple hours to complete, during this time a separate replication process could run multiple times (processing hourly diff files), due to some of the details around how database locking works you can get a situation where the replication completes and everything is runs exactly as expected and the database is consistent, but your PBF extract that is spanning multiple replication processes will now contain data from essentially 2 or more different time periods.

Hopefully I have explained it correctly, I feel I did not explain it very well. My apologies if that is the case.

@mmd-osm
Copy link

mmd-osm commented Jun 9, 2020

I think if you run your PBF extraction (which presumably is a read-only process) as one transaction with isolation level REPEATABLE READ (see https://www.postgresql.org/docs/9.5/transaction-iso.html), you won't see the effects of subsequent updates after issuing the first SELECT. That's irrespective of the number of minutely diffs applied subsequently. If you take a look the Postgres page, you'll also notice that Postgres won't have Phantom Reads in this isolation level. In case you're using another database, that may be a different story.

I mentioned the single transaction for the diff update, as it prevents your read process from reading parts of a single minutely diff. This is relevant for the case when your very first SELECT statement coincides with a minutely diff update.

@mgcuthbert
Copy link
Contributor Author

So the PBF extraction is run through Osmosis as is the replication piece. I don't disagree that using the database isolation level REPEATABLE READ probably would work (I haven't tested, but it certainly seems like it would), but it would require quite a few changes to Osmosis to make it work although I have no idea.

All in all, I completely understand if the community sees this as over-engineered and doesn't think this PR should be accepted, I do think the database approach would be a solid good one. So I would be fine with closing the PR if that is the case, I unfortunately don't have any bandwidth to look into the database approach at this time.

@migurski
Copy link
Collaborator

My inclination is to run with it. I’m going to merge this, we can always revisit.

@migurski migurski merged commit 107d52a into openstreetmap:master Jun 10, 2020
@mmd-osm
Copy link

mmd-osm commented Jun 10, 2020

t would require quite a few changes to Osmosis to make it work although I have no idea.

In the best case that would be a single SQL statement begin transaction isolation level repeatable read; right before the very first SQL statement your PBF extraction job issues. That's all. Really.

if you run a very large pbf extract it could take multiple hours to complete

So during that timeframe the minutely diff process would be stopped, and you wouldn't get any more updates?

Now that you've mentioned a runtime of multiple hours, I'm curious as to the size of those extracts. Just to mention a few data points, I'm running extracts with 150 Mio+ nodes in 30 minutes via Overpass. If you take a look at tools like OSMExpress, you get similar speeds on a minutely updated database. Many people also use osmium these days for extracts because it's much faster (that would be the third alternative to osmosis).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants