POC: Lock updated tuples in tuple_update() and tuple_delete()

Started by Alexander Korotkovalmost 4 years ago59 messageshackers

aekorotkov@gmail.com

almost 4 years ago

Hackers,

When working in the read committed transaction isolation mode
(default), we have the following sequence of actions when
tuple_update() or tuple_delete() find concurrently updated tuple.

1. tuple_update()/tuple_delete() returns TM_Updated
2. tuple_lock()
3. Re-evaluate plan qual (recheck if we still need to update/delete
and calculate the new tuple for update)
4. tuple_update()/tuple_delete() (this time should be successful,
since we've previously locked the tuple).

I wonder if we should merge steps 1 and 2. We could save some efforts
already done during tuple_update()/tuple_delete() for locking the
tuple. In heap table access method, we've to start tuple_lock() with
the first tuple in the chain, but tuple_update()/tuple_delete()
already visited it. For undo-based table access methods,
tuple_update()/tuple_delete() should start from the last version, why
don't place the tuple lock immediately once a concurrent update is
detected. I think this patch should have some performance benefits on
high concurrency.

Also, the patch simplifies code in nodeModifyTable.c getting rid of
the nested case. I also get rid of extra
table_tuple_fetch_row_version() in ExecUpdate. Why re-fetch the old
tuple, when it should be exactly the same tuple we've just locked.

I'm going to check the performance impact. Thoughts and feedback are welcome.

------
Regards,
Alexander Korotkov

Aleksander Alekseev

aleksander@timescale.com

almost 4 years ago

In reply to: Alexander Korotkov (#1)

Re: POC: Lock updated tuples in tuple_update() and tuple_delete()

Hi Alexander,

Thoughts and feedback are welcome.

I took some preliminary look at the patch. I'm going to need more time
to meditate on the proposed changes and to figure out the performance
impact.

So far I just wanted to let you know that the patch applied OK for me
and passed all the tests. The `else` branch here seems to be redundant
here:

+        if (!updated)
+        {
+            /* Should not encounter speculative tuple on recheck */
+            Assert(!HeapTupleHeaderIsSpeculative(tuple->t_data));
-             ReleaseBuffer(buffer);
+            ReleaseBuffer(buffer);
+        }
+        else
+        {
+            updated = false;
+        }

Also I wish there were a little bit more comments since some of the
proposed changes are not that straightforward.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev

aleksander@timescale.com

almost 4 years ago

In reply to: Aleksander Alekseev (#2)

Re: POC: Lock updated tuples in tuple_update() and tuple_delete()

Hi again,

+        if (!updated)
+        {
+            /* Should not encounter speculative tuple on recheck */
+            Assert(!HeapTupleHeaderIsSpeculative(tuple->t_data));
-             ReleaseBuffer(buffer);
+            ReleaseBuffer(buffer);
+        }
+        else
+        {
+            updated = false;
+        }

OK, I got confused here. I suggest changing the if(!...) { .. } else {
.. } code to if() { .. } else { .. } here.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev

aleksander@timescale.com

almost 4 years ago

In reply to: Aleksander Alekseev (#3)

Re: POC: Lock updated tuples in tuple_update() and tuple_delete()

Hi Alexander,

I'm going to need more time to meditate on the proposed changes and to figure out the performance impact.

OK, turned out this patch is slightly more complicated than I
initially thought, but I think I managed to get some vague
understanding of what's going on.

I tried to reproduce the case with concurrently updated tuples you
described on the current `master` branch. I created a new table:

```
CREATE TABLE phonebook(
"id" SERIAL PRIMARY KEY NOT NULL,
"name" NAME NOT NULL,
"phone" INT NOT NULL);

INSERT INTO phonebook ("name", "phone")
VALUES ('Alice', 123), ('Bob', 456), ('Charlie', 789);
```

Then I opened two sessions and attached them with LLDB. I did:

```
(lldb) b heapam_tuple_update
(lldb) c
```

... in both cases because I wanted to see two calls (steps 2 and 4) to
heapam_tuple_update() and check the return values.

Then I did:

```
session1 =# BEGIN;
session2 =# BEGIN;
session1 =# UPDATE phonebook SET name = 'Alex' WHERE name = 'Alice';
```

This update succeeds and I see heapam_tuple_update() returning TM_Ok.

```
session2 =# UPDATE phonebook SET name = 'Alfred' WHERE name = 'Alice';
```

This update hangs on a lock.

```
session1 =# COMMIT;
```

Now session2 unfreezes and returns 'UPDATE 0'. table_tuple_update()
was called once and returned TM_Updated. Also session2 sees an updated
tuple now. So apparently the visibility check (step 3) didn't pass.

At this point I'm slightly confused. I don't see where a performance
improvement is expected, considering that session2 gets blocked until
session1 commits.

Could you please walk me through here? Am I using the right test case
or maybe you had another one in mind? Which steps do you consider
expensive and expect to be mitigated by the patch?

--
Best regards,
Aleksander Alekseev

POC: Lock updated tuples in tuple_update() and tuple_delete()

Attachments:

Attachments:

Attachments:

Attachments:

Attachments:

Attachments:

Attachments:

Attachments:

Attachments:

Attachments: