Thread: Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.
This check is odd (tablecmds.c ATExecAttachPartition line 13861):
/* Temp parent cannot have a partition that is itself not a temp */ if (rel->rd_rel->relpersistence ==
RELPERSISTENCE_TEMP&& attachrel->rd_rel->relpersistence != RELPERSISTENCE_TEMP) ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot attach a permanent relation as partition of
temporaryrelation \"%s\"", RelationGetRelationName(rel))));
This seems to work (i.e. it's allowed to create a temp partition on a
permanent parent and not vice-versa, which you'd think makes sense) but
it's illusory, because if two sessions try to create temp partitions for
overlapping values, the second session fails with a silly error message.
To be more precise, do this in one session:
CREATE TABLE p (a int, b int) PARTITION BY RANGE (a);
CREATE TEMP TABLE p1 PARTITION OF p FOR VALUES FROM (0) TO (10);
then without closing that one, in another session repeat the second
command:
alvherre=# CREATE TEMP TABLE p1 PARTITION OF p FOR VALUES FROM (0) TO (10);
ERROR: partition "p1" would overlap partition "p1"
which is not what I would have expected.
Maybe there are combinations of different persistence values that can be
allowed to differ (an unlogged partition is probably OK with a permanent
parent), but I don't think the current check is good enough.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Oct 18, 2017 at 11:27 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:
> Maybe there are combinations of different persistence values that can be
> allowed to differ (an unlogged partition is probably OK with a permanent
> parent), but I don't think the current check is good enough.
This is also a sort of long-standing historical problem, I think.
This comment in expand_inherited_rtentry has been with us for a while:
/* * It is possible that the parent table has children that are temp * tables of other backends.
Wecannot safely access such tables * (because of buffering issues), and the best thing to do seems to be
*to silently ignore them. */
I do not find that to be a particularly sensible approach, and it's
probably even less sensible in the partitioning world than it was with
table inheritance. I think what we ought to do is prohibit it, but it
wasn't the job of the table partitioning commit to whack this around.
This is not the first example of a case where we've failed to put in
place sufficiently-strong checks to prohibit references to temp tables
in places where they don't make sense. See e.g.
16925c1e1fa236e4d7d6c8b571890e7c777f75d7,
948d6ec90fd35d6e1a59d0b1af8d6efd8442f0ad,
0688d84041f7963a2a00468c53aec7bb6051ef5c,
a13b01853084b6c6f9c34944bc19b3dd7dc4ceb2,
5fa3418304b06967fa54052b183bf96e1193d85e,
7d6e6e2e9732adfb6a93711ca6a6e42ba039fbdb,
82eed4dba254b8fda71d429b29d222ffb4e93fca. We really did not do a good
job plugging all of the cases where temp tables ought to be forbidden
when that feature went in, and IMHO this is more of the tail end of
that work than anything specific to partitioning. Your opinion may
differ, of course.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas wrote: > On Wed, Oct 18, 2017 at 11:27 AM, Alvaro Herrera > <alvherre@alvh.no-ip.org> wrote: > > Maybe there are combinations of different persistence values that can be > > allowed to differ (an unlogged partition is probably OK with a permanent > > parent), but I don't think the current check is good enough. > > This is also a sort of long-standing historical problem, I think. Sure. Actually, the code I'm calling attention to is ATExecAttachPartition() which was specifically written for partitioning. Looks like it was copied verbatim from ATExecAddInherit, but there's no shared code there AFAICS. I'm okay with prohibiting the case of different persistence values as you suggest. And I do suggest to back-patch that prohibition to pg10. Let me add that I'm not looking to blame anyone for what I report here. I'm very excited about the partitioning stuff and I'm happy of what was done for pg10. I'm now working on more partitioning-related changes which means I review the existing code as I go along, so I just report things that look wrong to me as I discover them, just with an interest in seeing them fixed, or documented, or at least discussed and explicitly agreed upon. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Oct 18, 2017 at 1:18 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > I'm okay with prohibiting the case of different persistence values as > you suggest. And I do suggest to back-patch that prohibition to pg10. I disagree. There's nothing any more broken about the way this works with partitioning in v10 than the way it works with inheritance in 9.6 or prior. Table inheritance has had warts for years, and the fact that we now have table partitioning doesn't make all of those same warts into must-fix-now bugs. They are still just warts, and they should be cleaned up through future development as we find them and have the time to do something about them. They should be documented as incompatibilities where appropriate. They should not be jammed into stable branches because users don't like it when DDL works one way in 10.1 and another way in 10.2. They don't even really like it when 10.0 works differently from 11.0, but you have to be willing to see bad decisions revisited at some point if you want progress to happen, and I certainly do. > Let me add that I'm not looking to blame anyone for what I report here. > I'm very excited about the partitioning stuff and I'm happy of what was > done for pg10. I'm now working on more partitioning-related changes > which means I review the existing code as I go along, so I just report > things that look wrong to me as I discover them, just with an interest > in seeing them fixed, or documented, or at least discussed and > explicitly agreed upon. Fair enough, but when you reply on the thread where I committed the patch and propose back-patching to the release that contained it, you make it sound like it's a bug in the patch, and I don't think either of the two things you just raised are. My complaint is not that I think you are accusing me of any sort of wrongdoing but that you're trying to justify back-patching what I think is new development by characterizing it as a bug fix. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers