Re: BUG #15565: truncate bug with tables which have temp tableinherited

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