Обсуждение: BUG #15565: truncate bug with tables which have temp table inherited

The following bug has been logged on the website:

Bug reference:      15565
Logged by:          Zhou Digoal
Email address:      digoal@126.com
PostgreSQL version: 11.1
Operating system:   CentOS 7.x x64
Description:

```
create table public.a (id int);
```

sesssion a:

```
create temp table a (like public.a) inherits(public.a);
```

session b:
```
create temp table a (like public.a) inherits(public.a);
```

any session:

```
postgres=# truncate public.a;
ERROR:  cannot truncate temporary tables of other sessions
```

but delete,select,update is ok;

```
postgres=# select * from public.
postgres-# a;
 id 
----
(0 rows)

postgres=# delete from public.a
postgres-# ;
DELETE 0
postgres=# update public.a set id=1;
UPDATE 0
postgres=# \d+ public.a
                                     Table "public.a"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description 
--------+---------+-----------+----------+---------+---------+--------------+-------------
 id     | integer |           |          |         | plain   |
| 
Child tables: a,
              pg_temp_3.a
```

On Mon, Dec 24, 2018 at 09:17:46AM +0000, PG Bug reporting form wrote:
> create table public.a (id int);
> create temp table a (like public.a) inherits(public.a);

This issue is way older than 11.

A couple of months ago there has been a discussion about mixing
temporary and permanent tables in a partition tree:
https://www.postgresql.org/message-id/CAKJS1f94Ojk0og9GMkRHGt8wHTW=ijq5KzJKuoBoqWLwSVwGmw@mail.gmail.com

The conclusion of this time was that we don't want to allow mixing
relations with different persistencies in the same tree for
partitions because those rely on relation cache lookups for their
description (PartitionDesc), however inheritance trees have been able
to accidently work this way for a long time.  This shares a little bit
of history with this report as partition trees and inheritance share
the same lookups at pg_inherits.

So there could be an argument to restrict the definition of
inheritance trees with trees mixing relation persistencies.

It is hard to say as well that truncate silently bypasses the
truncation of a temporary relation from another session if a given
session wants to truncate a whole tree.

Another approach would be to just live with this issue, as the use
case is pretty narrow anyway.
--
Michael

Hi,

On 2018/12/24 18:17, PG Bug reporting form wrote:
> The following bug has been logged on the website:
> 
> Bug reference:      15565
> Logged by:          Zhou Digoal
> Email address:      digoal@126.com
> PostgreSQL version: 11.1
> Operating system:   CentOS 7.x x64
> Description:        
> 
> ```
> create table public.a (id int);
> ```
> 
> sesssion a:
> 
> ```
> create temp table a (like public.a) inherits(public.a);
> ```
> 
> session b:
> ```
> create temp table a (like public.a) inherits(public.a);
> ```
> 
> any session:
> 
> ```
> postgres=# truncate public.a;
> ERROR:  cannot truncate temporary tables of other sessions
> ```
> 
> but delete,select,update is ok;

I'm not sure if the error being shown is a bug, but maybe we could remove
the error by making truncate do the same thing as select/update/delete,
that is, ignore child tables that are temporary of tables of other
sessions.  Attached patch to do that.

Thanks,
Amit

On 2018/12/25 9:45, Michael Paquier wrote:
> On Mon, Dec 24, 2018 at 09:17:46AM +0000, PG Bug reporting form wrote:
>> create table public.a (id int);
>> create temp table a (like public.a) inherits(public.a);
> 
> This issue is way older than 11.
> 
> A couple of months ago there has been a discussion about mixing
> temporary and permanent tables in a partition tree:
> https://www.postgresql.org/message-id/CAKJS1f94Ojk0og9GMkRHGt8wHTW=ijq5KzJKuoBoqWLwSVwGmw@mail.gmail.com
> 
> The conclusion of this time was that we don't want to allow mixing
> relations with different persistencies in the same tree for
> partitions because those rely on relation cache lookups for their
> description (PartitionDesc), however inheritance trees have been able
> to accidently work this way for a long time.  This shares a little bit
> of history with this report as partition trees and inheritance share
> the same lookups at pg_inherits.
> 
> So there could be an argument to restrict the definition of
> inheritance trees with trees mixing relation persistencies.
> 
> It is hard to say as well that truncate silently bypasses the
> truncation of a temporary relation from another session if a given
> session wants to truncate a whole tree.

Hmm, why don't we just ignore them like queries do?

Thanks,
Amit

On Tue, 25 Dec 2018 at 05:11, PG Bug reporting form
<noreply@postgresql.org> wrote:
> ```
> create table public.a (id int);
> ```
>
> sesssion a:
>
> ```
> create temp table a (like public.a) inherits(public.a);
> ```
>
> session b:
> ```
> create temp table a (like public.a) inherits(public.a);
> ```
>
> any session:
>
> ```
> postgres=# truncate public.a;
> ERROR:  cannot truncate temporary tables of other sessions

Is this blocking a real-world use case? Or did you just happen to
stumble on this?

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Tue, 25 Dec 2018 at 13:50, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> I'm not sure if the error being shown is a bug, but maybe we could remove
> the error by making truncate do the same thing as select/update/delete,
> that is, ignore child tables that are temporary of tables of other
> sessions.  Attached patch to do that.

I glanced at this and was confused at where "newrelation" comes from
and also the single parameter heap_close(). It does not look like it
would compile.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Dec 25, 2018 at 08:27:19PM +1300, David Rowley wrote:
> I glanced at this and was confused at where "newrelation" comes from
> and also the single parameter heap_close(). It does not look like it
> would compile.

Nope, it doesn't.  heap_close ought to not normally release the lock
either until the transaction has committed.  The patch clobbers
something that truncate_check_activity() already checks, which is not
elegant.  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.
--
Michael

On 2018/12/25 16:27, David Rowley wrote:
> On Tue, 25 Dec 2018 at 13:50, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> I'm not sure if the error being shown is a bug, but maybe we could remove
>> the error by making truncate do the same thing as select/update/delete,
>> that is, ignore child tables that are temporary of tables of other
>> sessions.  Attached patch to do that.
> 
> I glanced at this and was confused at where "newrelation" comes from
> and also the single parameter heap_close(). It does not look like it
> would compile.

Sorry, that was pretty negligent of me.  Here's the updated patch.

Thanks,
Amit

On 2018/12/25 17:03, Michael Paquier wrote:
> On Tue, Dec 25, 2018 at 08:27:19PM +1300, David Rowley wrote:
>> I glanced at this and was confused at where "newrelation" comes from
>> and also the single parameter heap_close(). It does not look like it
>> would compile.
> 
> Nope, it doesn't.  heap_close ought to not normally release the lock
> either until the transaction has committed.

Note that expand_inherited_rtentry does release the lock.

        /*
         * It is possible that the parent table has children that are temp
         * tables of other backends.  We cannot safely access such tables
         * (because of buffering issues), and the best thing to do seems
         * to be to silently ignore them.
         */
        if (childOID != parentOID && RELATION_IS_OTHER_TEMP(newrelation))
        {
            heap_close(newrelation, lockmode);
            continue;
        }

> The patch clobbers
> something that truncate_check_activity() already checks, which is not
> elegant.

Indeed, I missed truncate_check_activity.  Although, if we want to fix
this behavior like I'm proposing (ignore child tables that are temporary
tables of other sessions), it may not be such a good idea to do it by
modifying truncate_check_activity to deal specially with such tables,
because that would unnecessarily complicate its interface.

> 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.

Thanks,
Amit

On Tue, Dec 25, 2018 at 05:46:28PM +0900, Amit Langote wrote:
> On 2018/12/25 17:03, Michael Paquier wrote:
>> Nope, it doesn't.  heap_close ought to not normally release the lock
>> either until the transaction has committed.
>
> Note that expand_inherited_rtentry does release the lock.
>
>         /*
>          * It is possible that the parent table has children that are temp
>          * tables of other backends.  We cannot safely access such tables
>          * (because of buffering issues), and the best thing to do seems
>          * to be to silently ignore them.
>          */
>         if (childOID != parentOID && RELATION_IS_OTHER_TEMP(newrelation))
>         {
>             heap_close(newrelation, lockmode);
>             continue;
>         }

Oh, good point here.  Both David and you have been touching this area
of the code way more than myself lately.

>> The patch clobbers
>> something that truncate_check_activity() already checks, which is not
>> elegant.
>
> Indeed, I missed truncate_check_activity.  Although, if we want to fix
> this behavior like I'm proposing (ignore child tables that are temporary
> tables of other sessions), it may not be such a good idea to do it by
> modifying truncate_check_activity to deal specially with such tables,
> because that would unnecessarily complicate its interface.

I got to think more about that point, and indeed I agree with your
point to complicate truncate_check_activity more than necessary as it
still gets used for CASCADE and parent relations, so what you are
proposing is acceptable to me.

>> 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, 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.  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.

Thoughts?
--
Michael

On Wed, Dec 26, 2018 at 02:06:22PM +0800, 德哥 wrote:
> We can use this feature emulate Oracle-style Global temp table.
> https://github.com/digoal/blog/blob/master/201807/20180715_01.md

I am not sure what's the point you are trying to make here regarding
to this bug.  Anyway, it would be a good idea to put the important
quotes you would like folks here to know about, so as if github.com
goes away the archives of Postgres are still aware of what you are
trying to explain without an external source...
--
Michael

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

On Wed, Dec 26, 2018 at 03:53:35PM +0900, Amit Langote wrote:
> 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.

No objections in doing something like that.

> * 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); }

Okay, let's do as you suggest for all these things.  Thanks for
looking at the patch set.  I will try to finish wrapping that stuff
tomorrow, back-patching the tests on the way.
--
Michael

On Wed, Dec 26, 2018 at 04:54:08PM +0900, Michael Paquier wrote:
> Okay, let's do as you suggest for all these things.  Thanks for
> looking at the patch set.  I will try to finish wrapping that stuff
> tomorrow, back-patching the tests on the way.

And committed down to 9.4.
--
Michael

On 2018/12/27 10:20, Michael Paquier wrote:
> On Wed, Dec 26, 2018 at 04:54:08PM +0900, Michael Paquier wrote:
>> Okay, let's do as you suggest for all these things.  Thanks for
>> looking at the patch set.  I will try to finish wrapping that stuff
>> tomorrow, back-patching the tests on the way.
> 
> And committed down to 9.4.

Thank you.

Regards,
Amit