Обсуждение: Getting ERROR: could not open file "base/13164/t3_16388" withpartition table with ON COMMIT

On 2018/09/12 19:29, Rajkumar Raghuwanshi wrote:
> Hi,
> 
> I am getting below error while creating temp root partition table with on
> commit. getting same error from v10 onwards.
> 
> [edb@localhost bin]$ ./psql postgres
> psql (10.5)
> Type "help" for help.
> 
> postgres=# CREATE TEMP TABLE test ( c1 varchar, c2 int) PARTITION BY RANGE
> (c1) ON COMMIT DELETE ROWS;
> ERROR:  could not open file "base/13164/t3_16388": No such file or directory

Oops, good catch.

The infamous missing-relkind-check in heap_truncate() seems to be behind
this.  Perhaps, a patch like the attached will do?

Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> The infamous missing-relkind-check in heap_truncate() seems to be behind
> this.  Perhaps, a patch like the attached will do?

That seems excessively restrictive.  Anything that has storage (e.g.
matviews) ought to be truncatable, no?

I thought we had a macro or utility function somewhere that knew which
relkinds have storage, though I can't find it right now.  I'd be
inclined to instantiate that if it doesn't exist, and then the code
here ought to read something like

    if (RelkindHasStorage(rel->rd_rel->relkind))
        heap_truncate_one_rel(rel);

Also, possibly the test ought to be inside heap_truncate_one_rel
rather than its callers?

            regards, tom lane

On Wed, Sep 12, 2018 at 12:14:00PM -0400, Tom Lane wrote:
> I thought we had a macro or utility function somewhere that knew which
> relkinds have storage, though I can't find it right now.  I'd be
> inclined to instantiate that if it doesn't exist, and then the code
> here ought to read something like

Perhaps you are thinking about heap_create() which decides if a relkind
can have storage created by setting create_storage.  If you introduce a
new macro, I would think that refactoring as well heap.c so as it makes
use of it could make sense.
--
Michael

On 2018/09/13 1:14, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> The infamous missing-relkind-check in heap_truncate() seems to be behind
>> this.  Perhaps, a patch like the attached will do?
> 
> That seems excessively restrictive.  Anything that has storage (e.g.
> matviews) ought to be truncatable, no?

Not by heap_truncate it seems.  The header comment of heap_truncate says
that it concerns itself only with ON COMMIT truncation of temporary tables:

/*
 *   heap_truncate
 *
 *   This routine deletes all data within all the specified relations.
 *
 * This is not transaction-safe!  There is another, transaction-safe
 * implementation in commands/tablecmds.c.  We now use this only for
 * ON COMMIT truncation of temporary tables, where it doesn't matter.
 */

ON COMMIT clause can only be used with temporary tables, so the only two
possible relkind values that can be encountered here are RELKIND_RELATION
and RELKIND_PARTITIONED_TABLE.  Of the two, only the RELKIND_RELATION can
have storage.

> I thought we had a macro or utility function somewhere that knew which
> relkinds have storage, though I can't find it right now.  I'd be
> inclined to instantiate that if it doesn't exist, and then the code
> here ought to read something like
> 
>     if (RelkindHasStorage(rel->rd_rel->relkind))
>         heap_truncate_one_rel(rel);

There have been discussions (such as [1]), but none that led to some patch
being committed.  Might be a good idea to revive that discussion again, or
perhaps there is already some solution being discussed on the pluggable
storage thread.

> Also, possibly the test ought to be inside heap_truncate_one_rel
> rather than its callers?

Hmm, perhaps.  ExecuteTruncateGuts, the only other caller of
heap_truncate_one_rel, also checks the relkind to skip partitioned tables.
 There seem to be reasons to do it in the caller in that case though,
other than heap_truncate_one_rel being incapable of handling them.

Thanks,
Amit


[1] Macros bundling RELKIND_* conditions
https://www.postgresql.org/message-id/CAFjFpRcfzs%2Byst6YBCseD_orEcDNuAr9GUTraZ5GC%3DAvCYh55Q%40mail.gmail.com

On 2018/09/13 12:37, Michael Paquier wrote:
> On Wed, Sep 12, 2018 at 12:14:00PM -0400, Tom Lane wrote:
>> I thought we had a macro or utility function somewhere that knew which
>> relkinds have storage, though I can't find it right now.  I'd be
>> inclined to instantiate that if it doesn't exist, and then the code
>> here ought to read something like
> 
> Perhaps you are thinking about heap_create() which decides if a relkind
> can have storage created by setting create_storage.  If you introduce a
> new macro, I would think that refactoring as well heap.c so as it makes
> use of it could make sense.

Ashutosh Bapat had proposed patches in this area last year [1], but it
seems the discussion didn't lead to any development.

Thanks,
Amit

[1] Macros bundling RELKIND_* conditions
https://www.postgresql.org/message-id/CAFjFpRcfzs%2Byst6YBCseD_orEcDNuAr9GUTraZ5GC%3DAvCYh55Q%40mail.gmail.com

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/09/13 1:14, Tom Lane wrote:
>> That seems excessively restrictive.  Anything that has storage (e.g.
>> matviews) ought to be truncatable, no?

> Not by heap_truncate it seems.  The header comment of heap_truncate says
> that it concerns itself only with ON COMMIT truncation of temporary tables:

Ah.  Well, in that case I'm OK with just a simple test for
RELKIND_RELATION, but I definitely feel that it should be inside
heap_truncate.  Callers don't need to know about the limited scope
of what that does.

            regards, tom lane

On 2018/09/13 23:13, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> On 2018/09/13 1:14, Tom Lane wrote:
>>> That seems excessively restrictive.  Anything that has storage (e.g.
>>> matviews) ought to be truncatable, no?
> 
>> Not by heap_truncate it seems.  The header comment of heap_truncate says
>> that it concerns itself only with ON COMMIT truncation of temporary tables:
> 
> Ah.  Well, in that case I'm OK with just a simple test for
> RELKIND_RELATION, but I definitely feel that it should be inside
> heap_truncate.  Callers don't need to know about the limited scope
> of what that does.

I guess you meant inside heap_truncate_one_rel.  I updated the patch that
way, but I wonder if we shouldn't also allow other relkinds that have
storage which RelationTruncate et al can technically deal with.

Thanks,
Amit

On 2018/09/14 10:53, Amit Langote wrote:
> On 2018/09/13 23:13, Tom Lane wrote:
>> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>>> On 2018/09/13 1:14, Tom Lane wrote:
>>>> That seems excessively restrictive.  Anything that has storage (e.g.
>>>> matviews) ought to be truncatable, no?
>>
>>> Not by heap_truncate it seems.  The header comment of heap_truncate says
>>> that it concerns itself only with ON COMMIT truncation of temporary tables:
>>
>> Ah.  Well, in that case I'm OK with just a simple test for
>> RELKIND_RELATION, but I definitely feel that it should be inside
>> heap_truncate.  Callers don't need to know about the limited scope
>> of what that does.
> 
> I guess you meant inside heap_truncate_one_rel.  I updated the patch that
> way, but I wonder if we shouldn't also allow other relkinds that have
> storage which RelationTruncate et al can technically deal with.

Rajkumar pointed out off-list that the patch still remains to be applied.
Considering that there is a planned point release on Nov 8, maybe we
should do something about this?

Thanks,
Amit

On Thu, Nov 01, 2018 at 12:39:16PM +0900, Amit Langote wrote:
> Rajkumar pointed out off-list that the patch still remains to be applied.
> Considering that there is a planned point release on Nov 8, maybe we
> should do something about this?

Yes doing something about that very soon would be a good idea.  Tom,
are you planning to look at it or should I jump in?
--
Michael

On Thu, Nov 01, 2018 at 01:04:43PM +0900, Michael Paquier wrote:
> On Thu, Nov 01, 2018 at 12:39:16PM +0900, Amit Langote wrote:
>> Rajkumar pointed out off-list that the patch still remains to be applied.
>> Considering that there is a planned point release on Nov 8, maybe we
>> should do something about this?
>
> Yes doing something about that very soon would be a good idea.  Tom,
> are you planning to look at it or should I jump in?

And so I am looking at v3 now...

Adding a test case in temp.sql would be nice.

Would it make sense to support TRUNCATE on a materialized as well in the
future?  It seems to me that it is dangerous to assume that only
relations make use of heap_truncate_one_rel() anyway as modules or
external code could perfectly call it.  And the thing is documented
to work on a relation, including materialized views, not just an
ordinary table which is what RELKIND_RELATION only mentions.  On the
contrary we know that heap_truncate() works only on temporary
relations.  It is documented to do so and does so.

So it seems to me that Tom correctly mentioned to add the check in
heap_truncate, not heap_truncate_one_rel(), so v3 looks incorrect to
me.
--
Michael

On 2018/11/02 10:51, Michael Paquier wrote:
> On Thu, Nov 01, 2018 at 01:04:43PM +0900, Michael Paquier wrote:
>> On Thu, Nov 01, 2018 at 12:39:16PM +0900, Amit Langote wrote:
>>> Rajkumar pointed out off-list that the patch still remains to be applied.
>>> Considering that there is a planned point release on Nov 8, maybe we
>>> should do something about this?
>>
>> Yes doing something about that very soon would be a good idea.  Tom,
>> are you planning to look at it or should I jump in?
> 
> And so I am looking at v3 now...

Thanks for looking.

> Adding a test case in temp.sql would be nice.

Good idea, done.

When writing the test, I noticed something to be pointed out.  As of
1c7c317cd9d, partitions of a temporary partition table themselves must be
temporary, but the ON COMMIT action has to be specified for each table
separately.  Maybe, there is nothing to be concerned about here, because
there's nowhere to record what's specified for the parent to use it on the
children.  So, children's CREATE TABLE commands must specify the ON COMMIT
action for themselves.

> Would it make sense to support TRUNCATE on a materialized as well in the
> future?  It seems to me that it is dangerous to assume that only
> relations make use of heap_truncate_one_rel() anyway as modules or
> external code could perfectly call it.  And the thing is documented
> to work on a relation, including materialized views, not just an
> ordinary table which is what RELKIND_RELATION only mentions.  On the
> contrary we know that heap_truncate() works only on temporary
> relations.  It is documented to do so and does so.
> 
> So it seems to me that Tom correctly mentioned to add the check in
> heap_truncate, not heap_truncate_one_rel(), so v3 looks incorrect to
> me.

Okay, I agree that adding the preventive check in heap_truncate is a good
idea.

Updated patch attached.

Thanks,
Amit

On Fri, Nov 02, 2018 at 01:36:05PM +0900, Amit Langote wrote:
> When writing the test, I noticed something to be pointed out.  As of
> 1c7c317cd9d, partitions of a temporary partition table themselves must be
> temporary, but the ON COMMIT action has to be specified for each table
> separately.  Maybe, there is nothing to be concerned about here, because
> there's nowhere to record what's specified for the parent to use it on the
> children.  So, children's CREATE TABLE commands must specify the ON COMMIT
> action for themselves.

Well, I would actually expect callers to do so.  ON COMMIT PRESERVE or
DELETE rows does not make much sense for partitioned tables as
they have no physical presence (like today's thread about tablespaces
which you know about), but it looks better to just let the command go
through instead of complaining about it if we worry about inheritance.
And actually, your point has just made me aware of a second bug:
=# begin;
BEGIN
=# create temp table temp_parent (a int) partition by list (a) on commit
drop;
CREATE TABLE
=# create temp table temp_child_2 partition of temp_parent for values in
(2) on commit delete rows;
CREATE TABLE
=# insert into temp_parent values (2);
INSERT 0 1
=# table temp_parent;
 a
---
 2
(1 row)
=# commit;
ERROR:  XX000: could not open relation with OID 16420
LOCATION:  relation_open, heapam.c:1138

This case is funky.  The parent gets dropped at commit time, but it does
not know that it should drop the child as well per their dependencies.
This actually goes into the internals of performDeletion(), which is
scary to touch on back-branches just for such cases..
--
Michael

On Fri, Nov 02, 2018 at 02:18:04PM +0900, Michael Paquier wrote:
> This case is funky.  The parent gets dropped at commit time, but it does
> not know that it should drop the child as well per their dependencies.
> This actually goes into the internals of performDeletion(), which is
> scary to touch on back-branches just for such cases..

A bit more fun with inheritance:
=# begin;
BEGIN
=# create temp table aa_p (a int) on commit drop;
CREATE TABLE
=# create temp table aa_c (a int) inherits (aa_p) on commit delete rows;
NOTICE:  00000: merging column "a" with inherited definition
LOCATION:  MergeAttributes, tablecmds.c:2339
CREATE TABLE
=# insert into aa_p values (1);
INSERT 0 1
=# insert into aa_c values (1);
INSERT 0 1
=# commit;
NOTICE:  00000: drop cascades to table aa_c
LOCATION:  reportDependentObjects, dependency.c:995
ERROR:  XX000: could not open relation with OID 16426
LOCATION:  relation_open, heapam.c:1138

Let's treat that as a separate issue, as this happens also with an
unpatched build.  The only reason why you cannot trigger it with
partitions is that ON COMMIT is currently broken for them, so we should
fix the reported case first.  In consequence, I would tend to commit the
patch proposed and take care of the first, except if of course anybody
has an objection.
--
Michael

Hi,

On 2018/11/02 14:27, Michael Paquier wrote:
> On Fri, Nov 02, 2018 at 02:18:04PM +0900, Michael Paquier wrote:
>> This case is funky.

Interesting indeed.

>>  The parent gets dropped at commit time, but it does
>> not know that it should drop the child as well per their dependencies.
>> This actually goes into the internals of performDeletion(), which is
>> scary to touch on back-branches just for such cases..

Well, performDeletion *does* drop the child, because when the parent is
dropped due to its ON COMMIT DROP action, it's done using:

                /*
                 * Since this is an automatic drop, rather than one
                 * directly initiated by the user, we pass the
                 * PERFORM_DELETION_INTERNAL flag.
                 */
                performDeletion(&object,
                                DROP_CASCADE, PERFORM_DELETION_INTERNAL);

Note the DROP_CASCADE, which means its children will be deleted as part of
this.

> A bit more fun with inheritance:
> =# begin;
> BEGIN
> =# create temp table aa_p (a int) on commit drop;
> CREATE TABLE
> =# create temp table aa_c (a int) inherits (aa_p) on commit delete rows;
> NOTICE:  00000: merging column "a" with inherited definition
> LOCATION:  MergeAttributes, tablecmds.c:2339
> CREATE TABLE
> =# insert into aa_p values (1);
> INSERT 0 1
> =# insert into aa_c values (1);
> INSERT 0 1
> =# commit;
> NOTICE:  00000: drop cascades to table aa_c
> LOCATION:  reportDependentObjects, dependency.c:995
> ERROR:  XX000: could not open relation with OID 16426
> LOCATION:  relation_open, heapam.c:1138

I think the problem here is that the ON COMMIT DROP action on the parent
is executed before the ON COMMIT DELETE ROWS on the child.  The first
action drops the child along with its parent, causing the 2nd one to fail
upon not finding the table to apply heap_truncate() to.  It's
heap_truncate() that produces "ERROR: could not open relation with OID
xxx".  So, the NOTICE comes from the *successful* dropping of the child
and the ERROR comes from failure of heap_truncate() to find it.

By the way, this error is same as in the partitioned case.  It's simply
that the dependency type is different for regular inheritance child tables
than it is for partitions.  The former requires a user command to specify
CASCADE when dropping via parent, whereas the latter doesn't.  So, you see
the NOTICE in the regular inheritance case, whereas you don't in the
partitioning case.  The error itself is same in both cases.

> Let's treat that as a separate issue, as this happens also with an
> unpatched build.  The only reason why you cannot trigger it with
> partitions is that ON COMMIT is currently broken for them, so we should
> fix the reported case first.  In consequence, I would tend to commit the
> patch proposed and take care of the first, except if of course anybody
> has an objection. 

Agreed that they're two independent issues, although it wouldn't be such a
bad idea to fix them in one go, as they're both issues related to the
handling of ON COMMIT actions on tables in inheritance trees.

I've created a patch for the other issue and attached both.

Thanks,
Amit

On Fri, Nov 02, 2018 at 04:39:07PM +0900, Amit Langote wrote:
> Agreed that they're two independent issues, although it wouldn't be such a
> bad idea to fix them in one go, as they're both issues related to the
> handling of ON COMMIT actions on tables in inheritance trees.

I have pushed 0001 which fixes the bug reported on this thread down to
v10, after tweaking a bit the patch after more review.  I was testing
heap_truncate_one_rel() a bit more deeply and it actually happens that
it can work with matviews.  Re-reading the thread and sleeping on it,
Tom was been actually suggesting to move the check one level down to
heap_truncate_one_rel(), which actually makes more sense.  So I have
changed the patch so as a check on RELKIND_PARTITIONED_TABLE is done
instead of RELKIND_RELATION which is what has been proposed until now.

Regarding the second patch, could you start a second thread?  The scope
is not only related to partitioned tables but also to inheritance trees
so this goes way back in time, and I think that we could attract a
better audience about the problem.  I don't mind starting a thread
myself, not without your authorization as you wrote a patch to deal with
the problem.  My apologies, I have not looked at what you are proposing
yet.
--
Michael

On 2018/11/05 9:19, Michael Paquier wrote:
> On Fri, Nov 02, 2018 at 04:39:07PM +0900, Amit Langote wrote:
>> Agreed that they're two independent issues, although it wouldn't be such a
>> bad idea to fix them in one go, as they're both issues related to the
>> handling of ON COMMIT actions on tables in inheritance trees.
> 
> I have pushed 0001 which fixes the bug reported on this thread down to
> v10, after tweaking a bit the patch after more review.  I was testing
> heap_truncate_one_rel() a bit more deeply and it actually happens that
> it can work with matviews.  Re-reading the thread and sleeping on it,
> Tom was been actually suggesting to move the check one level down to
> heap_truncate_one_rel(), which actually makes more sense.  So I have
> changed the patch so as a check on RELKIND_PARTITIONED_TABLE is done
> instead of RELKIND_RELATION which is what has been proposed until now.

Okay, it's good that heap_truncate_one_rel() continues to work for all
relation types that can have storage.  Thanks for making the changes
yourself and committing.

> Regarding the second patch, could you start a second thread?  The scope
> is not only related to partitioned tables but also to inheritance trees
> so this goes way back in time, and I think that we could attract a
> better audience about the problem.  I don't mind starting a thread
> myself, not without your authorization as you wrote a patch to deal with
> the problem.  My apologies, I have not looked at what you are proposing
> yet.

Okay, no problem.  I will post that patch in a new thread detailing what
the problem is and how the proposed patch fixes it.

Thanks,
Amit

On 2018-Nov-02, Amit Langote wrote:

> Well, performDeletion *does* drop the child, because when the parent is
> dropped due to its ON COMMIT DROP action, it's done using:
> 
>                 /*
>                  * Since this is an automatic drop, rather than one
>                  * directly initiated by the user, we pass the
>                  * PERFORM_DELETION_INTERNAL flag.
>                  */
>                 performDeletion(&object,
>                                 DROP_CASCADE, PERFORM_DELETION_INTERNAL);
> 
> Note the DROP_CASCADE, which means its children will be deleted as part of
> this.

I think this code should collect all the OIDs to be dropped, then use a
single performMultipleDeletions() at the end, after the heap_truncate
call is done.  That seems better to me than a relkind check.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Nov 05, 2018 at 04:37:25PM -0300, Alvaro Herrera wrote:
> I think this code should collect all the OIDs to be dropped, then use a
> single performMultipleDeletions() at the end, after the heap_truncate
> call is done.  That seems better to me than a relkind check.

Yes, using a relkind checks seems error-prone to me, and you worked hard
to make sure that the drop machinery handles dependencies by itself,
(because you authored that work, right? :) ).

Amit has started a new thread about this problem as that's not only
related to partitions, but also to inheritance:
https://www.postgresql.org/message-id/68f17907-ec98-1192-f99f-8011400517f5@lab.ntt.co.jp

Could it be possible to move the thread there?  I have some other
comments about the patch, which I am going to provide there.
--
Michael

On 2018/11/06 4:37, Alvaro Herrera wrote:
> On 2018-Nov-02, Amit Langote wrote:
> 
>> Well, performDeletion *does* drop the child, because when the parent is
>> dropped due to its ON COMMIT DROP action, it's done using:
>>
>>                 /*
>>                  * Since this is an automatic drop, rather than one
>>                  * directly initiated by the user, we pass the
>>                  * PERFORM_DELETION_INTERNAL flag.
>>                  */
>>                 performDeletion(&object,
>>                                 DROP_CASCADE, PERFORM_DELETION_INTERNAL);
>>
>> Note the DROP_CASCADE, which means its children will be deleted as part of
>> this.
> 
> I think this code should collect all the OIDs to be dropped, then use a
> single performMultipleDeletions() at the end, after the heap_truncate
> call is done.  That seems better to me than a relkind check.

I've posted in a new thread about this:

* ON COMMIT actions and inheritance *
https://www.postgresql.org/message-id/68f17907-ec98-1192-f99f-8011400517f5%40lab.ntt.co.jp

I've to say that what you suggest seems to be a more elegant way to fix
this issue.  My patch fixes it by reconstructing the oids_to_truncate list
by removing the OIDs of children that were dropped via the ON COMMIT DROP
action of the parent using a SearchSysCacheExists1 test.

Thanks,
Amit