Is SELECT or INSERT in a function prone to race conditions?
Asked Answered
P

3

33

I wrote a function to create posts for a simple blogging engine:

CREATE FUNCTION CreatePost(VARCHAR, TEXT, VARCHAR[])
RETURNS INTEGER AS $$
    DECLARE
        InsertedPostId INTEGER;
        TagName VARCHAR;
    BEGIN
        INSERT INTO Posts (Title, Body)
        VALUES ($1, $2)
        RETURNING Id INTO InsertedPostId;

        FOREACH TagName IN ARRAY $3 LOOP
            DECLARE
                InsertedTagId INTEGER;
            BEGIN
                -- I am concerned about this part.
                BEGIN
                    INSERT INTO Tags (Name)
                    VALUES (TagName)
                    RETURNING Id INTO InsertedTagId;
                EXCEPTION WHEN UNIQUE_VIOLATION THEN
                    SELECT INTO InsertedTagId Id
                    FROM Tags
                    WHERE Name = TagName
                    FETCH FIRST ROW ONLY;
                END;

                INSERT INTO Taggings (PostId, TagId)
                VALUES (InsertedPostId, InsertedTagId);
            END;
        END LOOP;

        RETURN InsertedPostId;
    END;
$$ LANGUAGE 'plpgsql';

Is this prone to race conditions when multiple users delete tags and create posts at the same time?
Specifically, do transactions (and thus functions) prevent such race conditions from happening?
I'm using PostgreSQL 9.2.3.

Pannikin answered 11/4, 2013 at 2:51 Comment(0)
R
71

It's the recurring problem of SELECT or INSERT under possible concurrent write load, related to (but different from) UPSERT (which is INSERT or UPDATE).

This PL/pgSQL function uses UPSERT (INSERT ... ON CONFLICT ..) to INSERT or SELECT a single row:

CREATE OR REPLACE FUNCTION f_tag_id(_tag text, OUT _tag_id int)
  LANGUAGE plpgsql AS
$func$
BEGIN
   SELECT tag_id  -- only if row existed before
   FROM   tag
   WHERE  tag = _tag
   INTO   _tag_id;

   IF NOT FOUND THEN
      INSERT INTO tag AS t (tag)
      VALUES (_tag)
      ON     CONFLICT (tag) DO NOTHING
      RETURNING t.tag_id
      INTO   _tag_id;
   END IF;
END
$func$;

There is still a tiny window for a race condition. To make absolutely sure we get an ID:

CREATE OR REPLACE FUNCTION f_tag_id(_tag text, OUT _tag_id int)
  LANGUAGE plpgsql AS
$func$
BEGIN
   LOOP
      SELECT tag_id
      FROM   tag
      WHERE  tag = _tag
      INTO   _tag_id;

      EXIT WHEN FOUND;

      INSERT INTO tag AS t (tag)
      VALUES (_tag)
      ON     CONFLICT (tag) DO NOTHING
      RETURNING t.tag_id
      INTO   _tag_id;

      EXIT WHEN FOUND;
   END LOOP;
END
$func$;

fiddle

This keeps looping until either INSERT or SELECT succeeds. Call:

SELECT f_tag_id('possibly_new_tag');

About EXIT:

If subsequent commands in the same transaction rely on the existence of the row and it is actually possible that other transactions update or delete it concurrently, you can lock an existing row in the SELECT statement with FOR SHARE.
If the row gets inserted instead, it is locked (or not visible for other transactions) until the end of the transaction anyway.

Start with the common case (INSERT vs SELECT) to make it faster.

Related:

Related (pure SQL) solution to INSERT or SELECT multiple rows (a set) at once:

What's wrong with this pure SQL solution?

CREATE OR REPLACE FUNCTION f_tag_id(_tag text, OUT _tag_id int)
  LANGUAGE sql AS
$func$
WITH ins AS (
   INSERT INTO tag AS t (tag)
   VALUES (_tag)
   ON     CONFLICT (tag) DO NOTHING
   RETURNING t.tag_id
   )
SELECT tag_id FROM ins
UNION  ALL
SELECT tag_id FROM tag WHERE tag = _tag
LIMIT  1;
$func$;

Not entirely wrong, but it fails to seal a loophole, like @FunctorSalad worked out. The function can come up with an empty result if a concurrent transaction tries to do the same at the same time. The manual:

All the statements are executed with the same snapshot

If a concurrent transaction inserts the same new tag a moment earlier, but hasn't committed, yet:

  • The UPSERT part comes up empty, after waiting for the concurrent transaction to finish. (If the concurrent transaction should roll back, it still inserts the new tag and returns a new ID.)

  • The SELECT part also comes up empty, because it's based on the same snapshot, where the new tag from the (yet uncommitted) concurrent transaction is not visible.

We get nothing. Not as intended. That's counter-intuitive to naive logic (and I got caught there), but that's how the MVCC model of Postgres works - has to work.

So do not use this if multiple transactions can try to insert the same tag at the same time. Or loop until you actually get a row. The loop will hardly ever be triggered in common work loads anyway.

Postgres 9.4 or older

Given this (slightly simplified) table:

CREATE table tag (
  tag_id serial PRIMARY KEY
, tag    text   UNIQUE
);

An almost 100% secure function to insert new tag / select existing one, could look like this.

CREATE OR REPLACE FUNCTION f_tag_id(_tag text, OUT tag_id int)
  LANGUAGE plpgsql AS
$func$
BEGIN
   LOOP
      BEGIN
      WITH sel AS (SELECT t.tag_id FROM tag t WHERE t.tag = _tag FOR SHARE)
         , ins AS (INSERT INTO tag(tag)
                   SELECT _tag
                   WHERE  NOT EXISTS (SELECT 1 FROM sel)  -- only if not found
                   RETURNING tag.tag_id)       -- qualified so no conflict with param
      SELECT sel.tag_id FROM sel
      UNION  ALL
      SELECT ins.tag_id FROM ins
      INTO   tag_id;

      EXCEPTION WHEN UNIQUE_VIOLATION THEN     -- insert in concurrent session?
         RAISE NOTICE 'It actually happened!'; -- hardly ever happens
      END;

      EXIT WHEN tag_id IS NOT NULL;            -- else keep looping
   END LOOP;
END
$func$;

db<>fiddle here
Old sqlfiddle

Why not 100%? Consider the notes in the manual for the related UPSERT example:

Explanation

  • Try the SELECT first. This way you avoid the considerably more expensive exception handling 99.99% of the time.

  • Use a CTE to minimize the (already tiny) time slot for the race condition.

  • The time window between the SELECT and the INSERT within one query is super tiny. If you don't have heavy concurrent load, or if you can live with an exception once a year, you could just ignore the case and use the SQL statement, which is faster.

  • No need for FETCH FIRST ROW ONLY (= LIMIT 1). The tag name is obviously UNIQUE.

  • Remove FOR SHARE in my example if you don't usually have concurrent DELETE or UPDATE on the table tag. Costs a tiny bit of performance.

  • Never quote the language name: 'plpgsql'. plpgsql is an identifier. Quoting may cause problems and is only tolerated for backwards compatibility.

  • Don't use non-descriptive column names like id or name. When joining a couple of tables (which is what you do in a relational DB) you end up with multiple identical names and have to use aliases.

Built into your function

Using this function you could largely simplify your FOREACH LOOP to:

...
FOREACH TagName IN ARRAY $3
LOOP
   INSERT INTO taggings (PostId, TagId)
   VALUES   (InsertedPostId, f_tag_id(TagName));
END LOOP;
...

Faster, though, as a single SQL statement with unnest():

INSERT INTO taggings (PostId, TagId)
SELECT InsertedPostId, f_tag_id(tag)
FROM   unnest($3) tag;

Replaces the whole loop.

Alternative solution

This variant builds on the behavior of UNION ALL with a LIMIT clause: as soon as enough rows are found, the rest is never executed:

Building on this, we can outsource the INSERT into a separate function. Only there we need exception handling. Just as safe as the first solution.

CREATE OR REPLACE FUNCTION f_insert_tag(_tag text, OUT tag_id int)
  RETURNS int
  LANGUAGE plpgsql AS
$func$
BEGIN
   INSERT INTO tag(tag) VALUES (_tag) RETURNING tag.tag_id INTO tag_id;

   EXCEPTION WHEN UNIQUE_VIOLATION THEN  -- catch exception, NULL is returned
END
$func$;

Which is used in the main function:

CREATE OR REPLACE FUNCTION f_tag_id(_tag text, OUT _tag_id int)
   LANGUAGE plpgsql AS
$func$
BEGIN
   LOOP
      SELECT tag_id FROM tag WHERE tag = _tag
      UNION  ALL
      SELECT f_insert_tag(_tag)  -- only executed if tag not found
      LIMIT  1  -- not strictly necessary, just to be clear
      INTO   _tag_id;

      EXIT WHEN _tag_id IS NOT NULL;  -- else keep looping
   END LOOP;
END
$func$;
  • This is a bit cheaper if most of the calls only need SELECT, because the more expensive block with INSERT containing the EXCEPTION clause is rarely entered. The query is also simpler.

  • FOR SHARE is not possible here (not allowed in UNION query).

  • LIMIT 1 would not be necessary (tested in pg 9.4). Postgres derives LIMIT 1 from INTO _tag_id and only executes until the first row is found.

Reeve answered 11/4, 2013 at 13:39 Comment(13)
This is a fantastic write-up, @Erwin. I am using Postgres 9.6 and based my solution of SELECT or INSERT on your first code (along with the locking), only I am executing it as a standalone statement, not as a function. However, sometimes I am getting empty result from the statement. It's in the cases where some other transaction had already inserted a conflicting row. Subsequent select from the table yields the row, however. I thought this is not an expected behavior, or is it?Urdu
@twoflower: This would indicate that the concurrent transaction has not yet committed when your transaction tries to SELECT the row. The problem can be avoided with ON CONFLICT (tag) DO UPDATE SET tag = t.tag WHERE FALSE like suggested and explained above. Did you try that?Reeve
Yes, I am using that. I will try to prepare some minimal sample of this behavior.Urdu
@twoflower, ErwinBrandstetter: I ran into a similar problem as @twoflower; I added an example as an answer below (sorry for the comment-y answer, but this wouldn't be very readable formatted as a comment, and I didn't start a new question because this seems quite relevant to the original question). The DO UPDATE SET tag = t.tag WHERE FALSE does not seem to make a difference in this case.Acrostic
@FunctorSalad: Excellent point. Your added answer deserves more votes. I updated my answer with a fix and added explanation.Reeve
@twoflower: In my previous comment I had missed that the lock wouldn't fix your problem. Consider the updated answer.Reeve
how could we do it for returning multiple columns and not just _tag_id ?Doughy
@UjjwalGulecha: I suggest you post a question with your details. You can always link to this one for context, and you can drop a comment here to link back and get my attention.Reeve
Thanks for the suggestion @ErwinBrandstetter. I have created one with details and linked the question to this #56121969Doughy
@ErwinBrandstetter Wouldn't one solution for the query in 'What's wrong with this pure SQL solution?' be to lock the table? For your example this would be LOCK TABLE ONLY tag IN EXCLUSIVE MODE. It works for me, but I have too little experience with PostgesSQL to judge if the lock is a bad idea.Writeoff
@Kudi: Locking the whole table effectively disables concurrency, and the task becomes trivial. But that's very expensive, comparatively, so you typically want to avoid it. (It may also introduce new problems with deadlocks.)Reeve
@ErwinBrandstetter , does this solution: https://mcmap.net/q/28901/-select-or-insert-a-row-in-one-command solves the problem? Or does it also allows some race condition?Plantain
@Dikla: UPSERT is different from SELECT or INSERT. And the linked solution might still return "no row" as explained here: https://mcmap.net/q/28882/-how-to-use-returning-with-on-conflict-in-postgresqlReeve
A
5

There's still something to watch out for even when using the ON CONFLICT clause introduced in Postgres 9.5. Using the same function and example table as in @Erwin Brandstetter's answer, if we do:

Session 1: begin;

Session 2: begin;

Session 1: select f_tag_id('a');
 f_tag_id 
----------
       11
(1 row)

Session 2: select f_tag_id('a');
[Session 2 blocks]

Session 1: commit;

[Session 2 returns:]
 f_tag_id 
----------
        NULL
(1 row)

So f_tag_id returned NULL in session 2, which would be impossible in a single-threaded world!

If we raise the transaction isolation level to repeatable read (or the stronger serializable), session 2 throws ERROR: could not serialize access due to concurrent update instead. So no "impossible" results at least, but unfortunately we now need to be prepared to retry the transaction.

Edit: With repeatable read or serializable, if session 1 inserts tag a, then session 2 inserts b, then session 1 tries to insert b and session 2 tries to insert a, one session detects a deadlock:

ERROR:  deadlock detected
DETAIL:  Process 14377 waits for ShareLock on transaction 1795501; blocked by process 14363.
Process 14363 waits for ShareLock on transaction 1795503; blocked by process 14377.
HINT:  See server log for query details.
CONTEXT:  while inserting index tuple (0,3) in relation "tag"
SQL function "f_tag_id" statement 1

After the session that received the deadlock error rolls back, the other session continues. So I guess we should treat deadlock just like serialization_failure and retry, in a situation like this?

Alternatively, insert the tags in a consistent order, but this is not easy if they don't all get added in one place.

Acrostic answered 20/11, 2017 at 22:16 Comment(1)
Very useful additions! (And yes, I would treat it as serialization failure and retry in SERIALIZABLE transaction isolation. But it's considerably cheaper to handle this without exceptions in default READ COMMITTED.)Reeve
C
-1

I think there is a slight chance that when the tag already existed it might be deleted by another transaction after your transaction has found it. Using a SELECT FOR UPDATE should solve that.

Conciliator answered 11/4, 2013 at 4:53 Comment(1)
select for update only works for updates, when inserting the row does not exist so there is nothing to lock.Forras

© 2022 - 2024 — McMap. All rights reserved.