Fix for high concurrency issue #4
Conversation
There was a problem hiding this comment.
Thanks for this PR! The jsonb and connection pooling stuff looks great, but I'm not ready to merge the change to the commit query. If you want to split those other two changes out into a separate PR, I'll merge those, but if you'd rather work on the query too and merge it all together then that's fine too.
README.md
Outdated
| ## Usage | ||
|
|
||
| `sharedb-postgres` wraps native [node-postgres](https://github.com/brianc/node-postgres), and it supports the same configuration options. | ||
| `sharedb-postgres-jsonb` wraps native [node-postgres](https://github.com/brianc/node-postgres), and it supports the same configuration options. |
index.js
Outdated
| @@ -10,6 +10,7 @@ function PostgresDB(options) { | |||
| this.closed = false; | |||
|
|
|||
| this.pg_config = options; | |||
| this.pool = new pg.Pool(this.pg_config) | |||
There was a problem hiding this comment.
Looks like the indentation is off here, please use 2-space indentation :)
structure.sql
Outdated
| @@ -14,3 +14,13 @@ CREATE TABLE snapshots ( | |||
| data json not null, | |||
| PRIMARY KEY (collection, doc_id) | |||
| ); | |||
|
|
|||
| ALTER TABLE ops | |||
There was a problem hiding this comment.
Just fold the data type back into the CREATE TABLE statements
index.js
Outdated
| callback(err); | ||
| return; | ||
| } | ||
| /*const*/ var query = { |
There was a problem hiding this comment.
I think it's fine to use const directly here, especially since you're using string interpolation 2 lines below--this is ES6
index.js
Outdated
| Select n.* From ( select $1 c, $2 t, $3::integer v, $6::jsonb daa) | ||
| n | ||
| where (v = (select max(version)+1 v from ops where collection = $1 and doc_id = $2) or not exists (select 1 from ops where collection = $1 and doc_id = $2 for update)) and exists (select 1 from snaps) | ||
| Returning version`, |
There was a problem hiding this comment.
I found this query really hard to follow, even after reformatting and reindenting it. It's doing a lot of stuff, but I don't know if it's correct--could you explain (perhaps in a comment in the file) what this is doing and why?
Here's the reformatted version I made:
WITH snaps AS (
INSERT INTO snapshots (collection, doc_id, doc_type, version, data)
SELECT n.*
FROM (SELECT $1 c, $2 d, $4 t, $3::integer v, $5::jsonb daa) n
WHERE v = (
SELECT version+1 v
FROM snapshots
WHERE collection = $1 AND doc_id = $2
FOR UPDATE
) OR NOT EXISTS (
SELECT 1
FROM snapshots
WHERE collection = $1 AND doc_id = $2
FOR UPDATE
)
ON CONFLICT (collection, doc_id) DO UPDATE SET version = $3, data = $5, doc_type = $4
RETURNING version
)
INSERT INTO ops (collection, doc_id, version, operation)
SELECT n.* FROM (SELECT $1 c, $2 t, $3::integer v, $6::jsonb daa) n
WHERE (
v = (
SELECT max(version)+1 v
FROM ops
WHERE collection = $1 AND doc_id = $2
) OR NOT EXISTS (
SELECT 1
FROM ops
WHERE collection = $1 AND doc_id = $2
FOR UPDATE
)
) AND EXISTS (SELECT 1 FROM snaps)
RETURNING versionThere was a problem hiding this comment.
Yeah it non-trivial sql. I've now added the following comment
This query uses common table expression to upsert the snapshot table
(iff the new version is exactly 1 more than the latest table or if
the document id does not exists)
It will then Insert into the ops table if it is exactly 1 more than the
latest table or it the first operation and iff the previous insert into
the snapshot table is successful.
This result of this query the version of the newly inserted operation
If either the ops or the snapshot insert fails then 0 rows are returned
If 0 zeros are return then the callback must return false
index.js
Outdated
| }) | ||
| client.query(query, (err, res) => { | ||
| if (err) { | ||
| console.log(err.stack) |
There was a problem hiding this comment.
don't log anything from this library—that's up to the client.
|
I've this pull request with the required changes. Additionally
|
|
Hey @billwashere and @nornagon, what is the state of this? Do you need any help? |
|
I believe that it working correctly, however due to the fact that it is such a big change more testing may be required (I'll leave how much testing to @nornagon). If you want to try it out and see if you can break it, that would be great. |
|
Would you be able to rebase this on top of master? It looks like mostly minor conflicts. Also retitle the PR, since this is now only addressing the concurrency issue. Thanks! If this is reported working for a few people, I'm happy to merge it (at least, once I sit down and actually understand what's going on with the query...). The code as it is is not particularly well tested, so new untested code at least won't be a regression, but it'd be great to have better test coverage! especially cool would be if you could construct a test that fails with the old code but passes with the new. |
billwashere
changed the title
|
@billwashere can you merge this to master and update npm package? |
I think you can use this package: https://github.com/plotdb/sharedb-postgres |
|
if @zbryikt or someone wants to take over maintenance of this repo, I'm happy to grant access. |
This pull request does two changes
Changes the datatype on the ops and snapshots tables to JSONB
Attempts to resolve the high concurrency issue but committing the snapshot and operation in one transaction