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

Postgres' bump_timestamp function should round timestamps to epoch precision #2472

Closed
dim opened this issue Mar 27, 2020 · 2 comments
Closed

Comments

@dim
Copy link

dim commented Mar 27, 2020

Under heavy concurrent load, two records may end in Postgres with effectively the same epoch. Here's an example:

> SELECT * FROM objects ORDER BY last_modified;

                id                    | parent_id | resource_name |       last_modified        | data | deleted 
--------------------------------------+-----------+---------------+----------------------------+------+---------
 4fd08eed-4532-462e-a4b3-deb120317bde | /foo      | object        | 2020-03-27 16:09:52.388    | {}   | f
 b3d05073-e345-438d-9992-79b75a3fb8c7 | /foo      | object        | 2020-03-27 16:09:52.389    | {}   | f
 03db7f27-bf88-4848-9ef2-292a7dcf8cc0 | /foo      | object        | 2020-03-27 16:09:52.389808 | {}   | f
 a524e6ad-eac2-4abd-a83e-ade58a7c58d0 | /foo      | object        | 2020-03-27 16:09:52.389928 | {}   | f
 cb791368-80e7-4dae-9daa-d267a3d04a87 | /foo      | object        | 2020-03-27 16:09:52.39     | {}   | f
 b25378de-e6bf-46a1-a2de-94190ca32e33 | /foo      | object        | 2020-03-27 16:09:52.391    | {}   | f

Here's my fix:

--- schema.sql
+++ schema.sql
@@ -64,8 +64,8 @@ DROP TRIGGER IF EXISTS tgr_objects_last_modified ON objects;
 CREATE OR REPLACE FUNCTION bump_timestamp()
 RETURNS trigger AS $$
 DECLARE
-    previous TIMESTAMP;
-    current TIMESTAMP;
+    previous BIGINT;
+    current BIGINT;
 BEGIN
     previous := NULL;
     WITH existing_timestamps AS (
@@ -87,7 +87,7 @@ BEGIN
           AND resource_name = NEW.resource_name
       )
     )
-    SELECT MAX(last_modified) INTO previous
+    SELECT as_epoch(MAX(last_modified)) INTO previous
       FROM existing_timestamps;
 
     --
@@ -99,16 +99,16 @@ BEGIN
     -- an error (operation is cancelled).
     -- See https://github.com/mozilla-services/cliquet/issues/25
     --
-    current := clock_timestamp();
+    current := as_epoch(clock_timestamp()::TIMESTAMP);
     IF previous IS NOT NULL AND previous >= current THEN
-        current := previous + INTERVAL '1 milliseconds';
+        current := previous + 1;
     END IF;
 
     IF NEW.last_modified IS NULL OR
-       (previous IS NOT NULL AND as_epoch(NEW.last_modified) = as_epoch(previous)) THEN
+       (previous IS NOT NULL AND as_epoch(NEW.last_modified) = previous) THEN
         -- If record does not carry last-modified, or if the one specified
         -- is equal to previous, assign it to current (i.e. bump it).
-        NEW.last_modified := current;
+        NEW.last_modified := from_epoch(current);
     END IF;
 
     RETURN NEW;
@leplatrem
Copy link
Contributor

Wao! That's really cool!

We had identified this issue #602 but never found a fix!

Do you want to submit a PR or do you want us to do it? (migration etc.)

@dim
Copy link
Author

dim commented Mar 30, 2020

@leplatrem I can do a PR, but it wouldn't be a proper one, unfortunately. I can read Python but it's not my language of choice (sorry) and I would not be confident enough to include a regression test.

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

No branches or pull requests

2 participants