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

[Kyotaro HORIGUCHI]

Hello, sorry for my late reply.

At Wed, 10 Jan 2018 14:56:49 -0500, Tom Lane  wrote in 
<26017.1515614...@sss.pgh.pa.us>
> I think that there might be a much simpler solution to this, which
> is to just remove make_inh_translation_list's tests of attinhcount,
> as per attached.  Those are really pretty redundant: since we are
> matching by column name, the unique index on pg_attribute already
> guarantees there is at most one matching column.  I have a feeling

My thought were restricted to the same behavior as the drop case,
but Tom's solution is also fine for me. I agree to the point that
the colums with the same name in a inheritance tree are safely
assumed to be in a inheritance relationship. (Assuming everything
is consistent.)

At Fri, 12 Jan 2018 15:52:08 -0500, Tom Lane  wrote in 
<15881.1515790...@sss.pgh.pa.us>
> I wrote:
> > I think that there might be a much simpler solution to this, which
> > is to just remove make_inh_translation_list's tests of attinhcount,
> > as per attached.
> 
> I went ahead and pushed that, with an isolation test based on the
> one Horiguchi-san submitted but covering a few related cases.

Thank you for commiting it.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

[Tom Lane]

I wrote:
> I think that there might be a much simpler solution to this, which
> is to just remove make_inh_translation_list's tests of attinhcount,
> as per attached.

I went ahead and pushed that, with an isolation test based on the
one Horiguchi-san submitted but covering a few related cases.

regards, tom lane

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

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> [ 0002-Lock-parent-on-ALTER-TABLE-NO-INHERIT.patch ]

I don't especially like any of the patches proposed on this thread.
The one with rechecking inheritance seems expensive, duplicative,
and complicated.  The approach of taking a lock on the parent will
create deadlocks in usage patterns that did not encounter such
deadlocks before --- in particular, in the scenarios in which this
behavior matters at all, the ALTER will deadlock against ordinary
queries that lock the parent before the child.  So I can't believe
that anyone who's hitting the problem in the field will think the
extra lock is an improvement.

I think that there might be a much simpler solution to this, which
is to just remove make_inh_translation_list's tests of attinhcount,
as per attached.  Those are really pretty redundant: since we are
matching by column name, the unique index on pg_attribute already
guarantees there is at most one matching column.  I have a feeling
that those tests are my fault and I put them in on the theory that
they could save a few strcmp executions --- but if they're causing
problems, we can certainly drop them.  In any case they'd only save
something meaningful if most of the child's columns are non-inherited,
which doesn't seem like the way to bet.

In this way, if the child gets past the initial check on whether it's
been dropped, we'll still succeed in matching its columns, even if
they are no longer marked as inherited.  For me, this works fine with
either the ALTER NO INHERIT case (c1 does get scanned, with no error)
or the DROP TABLE case (c1 doesn't get scanned).  Now, you can break
it if you really try hard: make the concurrent transaction do both
ALTER NO INHERIT and then DROP COLUMN.  But that's not a scenario
that's been complained of, so I don't want to add a ton of mechanism
to fix it.

regards, tom lane

diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index 95557d7..5c4d113 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -1832,7 +1832,7 @@ make_inh_translation_list(Relation oldrelation, Relation newrelation,
 		 */
 		if (old_attno < newnatts &&
 			(att = TupleDescAttr(new_tupdesc, old_attno)) != NULL &&
-			!att->attisdropped && att->attinhcount != 0 &&
+			!att->attisdropped &&
 			strcmp(attname, NameStr(att->attname)) == 0)
 			new_attno = old_attno;
 		else
@@ -1840,7 +1840,7 @@ make_inh_translation_list(Relation oldrelation, Relation newrelation,
 			for (new_attno = 0; new_attno < newnatts; new_attno++)
 			{
 att = TupleDescAttr(new_tupdesc, new_attno);
-if (!att->attisdropped && att->attinhcount != 0 &&
+if (!att->attisdropped &&
 	strcmp(attname, NameStr(att->attname)) == 0)
 	break;
 			}

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

[Kyotaro HORIGUCHI]

Hello,

At Wed, 29 Nov 2017 14:04:01 +0900, Michael Paquier  
wrote in 
> On Tue, Sep 19, 2017 at 3:04 PM, Kyotaro HORIGUCHI
>  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 
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 000..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; 
+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 000..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 
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

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

[Michael Paquier]

On Tue, Sep 19, 2017 at 3:04 PM, Kyotaro HORIGUCHI
 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.
-- 
Michael