From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, amul sul <sulamul(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com> |
Subject: | Re: Partitioning with temp tables is broken |
Date: | 2018-06-19 05:47:00 |
Message-ID: | 20180619054700.GB6421@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Jun 19, 2018 at 10:56:49AM +0900, Amit Langote wrote:
> On 2018/06/18 15:02, Michael Paquier wrote:
>> Those tests should be upper-case I think to keep consistency with the
>> surrounding code.
>
> As you may have seen in the changed code, the guard in MergeAttributes
> really just checks relpersistance, so the check prevents foreign tables
> from being added as a partition of a temporary parent table. Not sure how
> much sense it makes to call a foreign table's relpersistence to be
> RELPERSISTENCE_PERMANENT but that's a different matter I guess.
Its existence does not go away when the session finishes, so that makes
sense to me, at least philosophically :)
> One cannot create temporary foreign tables because of the lack of
> syntax for it, so there's no need to worry about that.
This could have its own use-cases.
> Yeah, unlike regular inheritance, we access partitions from PartitionDesc
> without checking relpersistence in some of the newly added code in PG 11
> and also in PG 10 (the latter doesn't crash but gives an unintuitive error
> as you said upthread). If a use case to mix temporary and permanent
> tables in partition tree pops up in the future, we can revisit it and
> implement it correctly.
Agreed. Let's keep things simple for now.
>> Adding a test which makes sure that partition trees made of only
>> temporary relations work would be nice.
>
> I added a test to partition_prune.sql.
I didn't think about that actually, but that's actually a good idea to
keep that around. Having a test case which checks that ATTACH works
when everything has temporary relations was still missing.
>> Documenting all those restrictions and behaviors would be nice, why not
>> adding a paragraph in ddl.sgml, under the section for declarative
>> partitioning?
>
> OK, I've tried that in the attached updated patch, but I couldn't write
> beyond a couple of sentences that I've added in 5.10.2.3. Limitations.
Adding the description in this section is a good idea.
+ <listitem>
+ <para>
+ One cannot have both temporary and permanent relations in a given
+ partition tree. That is, if the root partitioned table is permanent,
+ so must be its partitions at all levels and vice versa.
+ </para>
+ </listitem>
I have reworded a bit that part.
+ /* If the parent is permanent, so must be all of its partitions. */
+ if (is_partition &&
+ relation->rd_rel->relpersistence != RELPERSISTENCE_TEMP &&
+ relpersistence == RELPERSISTENCE_TEMP)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot create a temporary relation as partition of permanent relation \"%s\"",
+ RelationGetRelationName(relation))));
Added a note about inheritance allowing this case, and reduced the diff
noise of the patch.
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
[...]
+ERROR: cannot attach a permanent relation as partition of temporary relation "temp_parted"
+drop table temp_parted;
This set of tests does not check that trees made of only temporary
relations can work, so I added a test for that, refactoring the tests a
bit. The same applies for both create_table and alter_table.
+-- Check pruning for a partition tree containining only temporary relations
+create temp table pp_temp_parent (a int) partition by list (a);
+create temp table pp_temp_part_1 partition of pp_temp_parent for values in (1);
+create temp table pp_temp_part_def partition of pp_temp_parent default;
+explain (costs off) select * from pp_temp_parent where true;
+explain (costs off) select * from pp_temp_parent where a = 2;
+drop table pp_temp_parent;
That's a good idea. Typo here => s/containining/containing/.
Attached is what I am finishing with after a closer review. Amit, what
do you think?
--
Michael
Attachment | Content-Type | Size |
---|---|---|
partition-temp-block.patch | text/x-diff | 10.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2018-06-19 06:30:28 | Re: Excessive CPU usage in StandbyReleaseLocks() |
Previous Message | Thomas Munro | 2018-06-19 05:43:42 | Excessive CPU usage in StandbyReleaseLocks() |