Re: [HACKERS] Race between SELECT and ALTER TABLE NO INHERIT

Hello,

At Wed, 29 Nov 2017 14:04:01 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqTt-mj4S8qoO_C9CaFAcV0J3vhgg4_Kw2U1wXvyEYB0bw@mail.gmail.com>
> On Tue, Sep 19, 2017 at 3:04 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >> By the way, I will take a look at your patch when I come back from the
> >> vacation.  Meanwhile, I noticed that it needs another rebase after
> >> 0a480502b092 [1].
> 
> Moved to CF 2018-01.

Thank you. (I'll do that by myself from the next CF)

This is rebased patch and additional patch of isolation test for
this problem.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 4336b15d2c0d95e7044746aa5c3ae622712e41b3 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Tue, 12 Dec 2017 17:06:53 +0900
Subject: [PATCH 1/2] Add isolation test

---
 src/test/isolation/expected/select-noinherit.out |  9 +++++++++
 src/test/isolation/isolation_schedule            |  1 +
 src/test/isolation/specs/select-noinherit.spec   | 23 +++++++++++++++++++++++
 3 files changed, 33 insertions(+)
 create mode 100644 src/test/isolation/expected/select-noinherit.out
 create mode 100644 src/test/isolation/specs/select-noinherit.spec

diff --git a/src/test/isolation/expected/select-noinherit.out b/src/test/isolation/expected/select-noinherit.out
new file mode 100644
index 0000000..5885167
--- /dev/null
+++ b/src/test/isolation/expected/select-noinherit.out
@@ -0,0 +1,9 @@
+Parsed test spec with 2 sessions
+
+starting permutation: alt1 sel2 c1
+step alt1: ALTER TABLE c1 NO INHERIT p;
+step sel2: SELECT * FROM p; <waiting ...>
+step c1: COMMIT;
+step sel2: <... completed>
+a              
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index e41b916..6e04ea4 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -63,3 +63,4 @@ test: async-notify
 test: vacuum-reltuples
 test: timeouts
 test: vacuum-concurrent-drop
+test: select-noinherit
diff --git a/src/test/isolation/specs/select-noinherit.spec b/src/test/isolation/specs/select-noinherit.spec
new file mode 100644
index 0000000..31662a9
--- /dev/null
+++ b/src/test/isolation/specs/select-noinherit.spec
@@ -0,0 +1,23 @@
+# SELECT and ALTER TABLE NO INHERIT test
+#
+
+setup
+{
+ CREATE TABLE p (a integer);
+ CREATE TABLE c1 () INHERITS (p);
+}
+
+teardown
+{
+ DROP TABLE p CASCADE;
+}
+
+session "s1"
+setup        { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step "alt1"    { ALTER TABLE c1 NO INHERIT p; }
+step "c1"    { COMMIT; }
+
+session "s2"
+step "sel2"    { SELECT * FROM p; }
+
+permutation "alt1" "sel2" "c1"
-- 
2.9.2

From c2793d9200948d693150a5bbeb3815e3b5404be2 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Tue, 12 Dec 2017 17:38:12 +0900
Subject: [PATCH 2/2] Lock parent on ALTER TABLE NO INHERIT

NO INHERIT doesn't modify the parent at all but lock is required to
avoid error caused when a concurrent access see a false child.
---
 src/backend/commands/tablecmds.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d979ce2..a8d119f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13246,7 +13246,28 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid,
         reltype = ((AlterObjectSchemaStmt *) stmt)->objectType;
 
     else if (IsA(stmt, AlterTableStmt))
-        reltype = ((AlterTableStmt *) stmt)->relkind;
+    {
+        AlterTableStmt *alterstmt = (AlterTableStmt *)stmt;
+        ListCell *lc;
+
+        reltype = alterstmt->relkind;
+
+        foreach (lc, alterstmt->cmds)
+        {
+            AlterTableCmd *cmd = lfirst_node(AlterTableCmd, lc);
+            Assert(IsA(cmd, AlterTableCmd));
+
+            /*
+             * Though NO INHERIT doesn't modify the parent, lock on the parent
+             * is necessary so that no concurrent expansion of inheritances
+             * sees a false child and ends with ERROR.  But no need to ascend
+             * further.
+             */
+            if (cmd->subtype == AT_DropInherit)
+                RangeVarGetRelid((RangeVar *)cmd->def,
+                                 AccessExclusiveLock, false);
+        }
+    }
     else
     {
         reltype = OBJECT_TABLE; /* placate compiler */
-- 
2.9.2