Subject: Re: BUG #15565: truncate bug with tables which have temp table
 inherited
References: <15565-ce67a48d0244436a@postgresql.org>
 <0285c6d1-2d11-2f2c-6fae-d4575cdc3c7b@lab.ntt.co.jp>
 <CAKJS1f_pDqUvyWeiUufRUJ2nUHvD4-rYvjHr2ZiPLsvQ6UQkUg@mail.gmail.com>
 <20181225080314.GF2334@paquier.xyz>
 <ab9d096e-2b9e-8588-4b61-6607cf43da62@lab.ntt.co.jp>
 <20181226015127.GB2234@paquier.xyz>
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Message-ID: <c77362d8-227e-0276-ab4b-eab20ceca0e9@lab.ntt.co.jp>
Date: Wed, 26 Dec 2018 15:53:35 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.3.3
MIME-Version: 1.0
In-Reply-To: <20181226015127.GB2234@paquier.xyz>
Content-Type: multipart/mixed;
 boundary="------------464A5A432BA84F8CAE81E45C"
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: David Rowley <david.rowley@2ndquadrant.com>, digoal@126.com,
        pgsql-bugs@lists.postgresql.org,
        PG Bug reporting form <noreply@postgresql.org>
Precedence: bulk

This is a multi-part message in MIME format.
--------------464A5A432BA84F8CAE81E45C
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit

On 2018/12/26 10:51, Michael Paquier wrote:
> On Tue, Dec 25, 2018 at 05:46:28PM +0900, Amit Langote wrote:
>> On 2018/12/25 17:03, Michael Paquier wrote:
>>> I am wondering as well if we could take this occasion for
>>> having better isolation testing when it comes to inheritance trees
>>> mixing relation persistency.  At least for the TRUNCATE case it would
>>> be nice to have something.
>>
>> Yeah, perhaps.
> 
> Let's bite the bullet then.  Attached is a more advanced patch which
> is based on what you previously sent,

Thank you for taking care of adding the isolation tests.

> except that I don't like much
> the fact of copying AccessExclusiveLock around, so let's save it into
> a separate variable.  I hope that's clearer.

Yep, that's better.

Comment on the comment added by the patch:

+         ...  Note that this
+	 * check overlaps with truncate_check_activity()'s inner checks
+	 * done below, but an extra one is kept here for simplicity.

"truncate_check_activity()'s inner checks" sounds a bit odd to me.  Maybe
write the sentence as:

Note that this check is same as one of the checks performed by
truncate_check_activity() that's called below, but it errors out on this
being true, whereas we'd like to ignore such child tables.  It makes sense
to repeat the check here instead of complicating the code in
truncate_check_activity() to get the behavior we want.

A bit long but clearer I think.

> I have also designed a
> set of isolation tests which adds more coverage for inheritance trees
> mixing persistence across sessions which I also used to check the
> patch.  This test suite could always be expanded later on, but I think
> that's already a step in the good direction.

I looked at the tests.  I still need to get used to reading the outputs of
these, here are some suggestions:

* Maybe, you should rename inh_temp_parent to inh_parent (or
inh_perm_parent if you want to), because the "temp" in the name makes
someone looking at this think that the parent not accessible from both
sessions or that both sessions have their own temporary parent.  Of
course, they can look at setup() and know that that's not the case, but
it's better to convey that using the naming.

* Related, maybe rename inh_temp_s1/s2 to inh_temp_child_s1/s2.

* Tests s1/s2_update/delete_c should be written such that the query
appears to try to update/delete other session's data, which doesn't work
because the other session's child will be skipped.  For example:

change: "s1_update_c" { UPDATE inh_temp_parent SET a = 13 WHERE a = 3; }
to: "s1_update_c" { UPDATE inh_temp_parent SET a = 13 WHERE a IN (3, 5); }

selecting from inh_parent afterwords will show that 5 isn't changed by
s1_update_c, because it's in inh_temp_child_s2.

Attached is a delta diff showing the changes to isolation tests that I'm
suggesting.

Thanks,
Amit

--------------464A5A432BA84F8CAE81E45C
Content-Type: text/plain; charset=UTF-8;
 name="v3-isolation-test-amit-delta.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="v3-isolation-test-amit-delta.patch"

diff --git a/src/test/isolation/specs/inherit-temp.spec b/src/test/isolation/specs/inherit-temp.spec
index 31961be63d..5cd251d0aa 100644
--- a/src/test/isolation/specs/inherit-temp.spec
+++ b/src/test/isolation/specs/inherit-temp.spec
@@ -8,12 +8,12 @@
 
 setup
 {
-  CREATE TABLE inh_temp_parent (a int);
+  CREATE TABLE inh_parent (a int);
 }
 
 teardown
 {
-  DROP TABLE inh_temp_parent;
+  DROP TABLE inh_parent;
 }
 
 # Session 1 executes actions which act directly on both the parent and
@@ -22,39 +22,39 @@ teardown
 session "s1"
 setup
 {
-  CREATE TEMPORARY TABLE inh_temp_s1 () INHERITS (inh_temp_parent);
+  CREATE TEMPORARY TABLE inh_temp_child_s1 () INHERITS (inh_parent);
 }
 step "s1_begin" { BEGIN; }
-step "s1_truncate_p" { TRUNCATE inh_temp_parent; }
-step "s1_select_p" { SELECT a FROM inh_temp_parent; }
-step "s1_select_c" { SELECT a FROM inh_temp_s1; }
-step "s1_insert_p" { INSERT INTO inh_temp_parent VALUES (1), (2); }
-step "s1_insert_c" { INSERT INTO inh_temp_s1 VALUES (3), (4); }
-step "s1_update_p" { UPDATE inh_temp_parent SET a = 11 WHERE a = 1; }
-step "s1_update_c" { UPDATE inh_temp_parent SET a = 13 WHERE a = 3; }
-step "s1_delete_p" { DELETE FROM inh_temp_parent WHERE a = 2; }
-step "s1_delete_c" { DELETE FROM inh_temp_parent WHERE a = 4; }
+step "s1_truncate_p" { TRUNCATE inh_parent; }
+step "s1_select_p" { SELECT a FROM inh_parent; }
+step "s1_select_c" { SELECT a FROM inh_temp_child_s1; }
+step "s1_insert_p" { INSERT INTO inh_parent VALUES (1), (2); }
+step "s1_insert_c" { INSERT INTO inh_temp_child_s1 VALUES (3), (4); }
+step "s1_update_p" { UPDATE inh_parent SET a = 11 WHERE a = 1; }
+step "s1_update_c" { UPDATE inh_parent SET a = 13 WHERE a IN (3, 5); }
+step "s1_delete_p" { DELETE FROM inh_parent WHERE a = 2; }
+step "s1_delete_c" { DELETE FROM inh_parent WHERE a IN (4, 6); }
 step "s1_commit" { COMMIT; }
 teardown
 {
-  DROP TABLE inh_temp_s1;
+  DROP TABLE inh_temp_child_s1;
 }
 
 # Session 2 executes actions on the parent which act only on the child.
 session "s2"
 setup
 {
-  CREATE TEMPORARY TABLE inh_temp_s2 () INHERITS (inh_temp_parent);
+  CREATE TEMPORARY TABLE inh_temp_child_s2 () INHERITS (inh_parent);
 }
-step "s2_truncate_p" { TRUNCATE inh_temp_parent; }
-step "s2_select_p" { SELECT a FROM inh_temp_parent; }
-step "s2_select_c" { SELECT a FROM inh_temp_s2; }
-step "s2_insert_c" { INSERT INTO inh_temp_s2 VALUES (5), (6); }
-step "s2_update_c" { UPDATE inh_temp_parent SET a = 15 WHERE a = 5; }
-step "s2_delete_c" { DELETE FROM inh_temp_parent WHERE a = 5; }
+step "s2_truncate_p" { TRUNCATE inh_parent; }
+step "s2_select_p" { SELECT a FROM inh_parent; }
+step "s2_select_c" { SELECT a FROM inh_temp_child_s2; }
+step "s2_insert_c" { INSERT INTO inh_temp_child_s2 VALUES (5), (6); }
+step "s2_update_c" { UPDATE inh_parent SET a = 15 WHERE a IN (3, 5); }
+step "s2_delete_c" { DELETE FROM inh_parent WHERE a IN (4, 6); }
 teardown
 {
-  DROP TABLE inh_temp_s2;
+  DROP TABLE inh_temp_child_s2;
 }
 
 # Check INSERT behavior across sessions

--------------464A5A432BA84F8CAE81E45C--