Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Wed, Oct 18, 2017 at 1:18 PM, Alvaro Herrera  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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Alvaro Herrera]

Robert Haas wrote:
> On Wed, Oct 18, 2017 at 11:27 AM, Alvaro Herrera
>  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 Herrerahttps://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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Wed, Oct 18, 2017 at 11:27 AM, Alvaro Herrera
 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.  We cannot 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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Tom Lane]

Robert Haas  writes:
> On Wed, Oct 18, 2017 at 4:53 AM, Alvaro Herrera  
> wrote:
>> My view is that the fact that partitioning uses inheritance is just an
>> implementation detail.  We shouldn't let historical behavior for
>> inheritance dictate behavior for partitioning.  Inheritance has many
>> undesirable warts.

> I don't think it's a good idea for table partitioning and table
> inheritance to behave differently.

I'm with Robert on this one.  I'd be in favor of changing both cases
to make ALTER OWNER recurse by default.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Wed, Oct 18, 2017 at 4:53 AM, Alvaro Herrera  wrote:
> My view is that the fact that partitioning uses inheritance is just an
> implementation detail.  We shouldn't let historical behavior for
> inheritance dictate behavior for partitioning.  Inheritance has many
> undesirable warts.

I am OK with filing down warts over time, but to be clear, I think
it's too late to change things like this in v10, which has shipped.

>> > The alter table docs say that ONLY must be specified if one does not
>> > want to modify descendants, so I think this is a bug.
>>
>> Just to clarify, if we do think of it as a bug, then it will apply to the
>> inheritance case as well, right?
>
> I'd leave it alone.

I don't think it's a good idea for table partitioning and table
inheritance to behave differently.  Generally, I think we don't want
to end up with three categories of behavior: commands that recurse
always, commands that recurse to partitions but not inheritance
children, and commands that don't recurse.  If we now think that ALTER
TABLE .. OWNER TO should recurse, then we should change that to do so
in all cases and document it as a backward incompatibility.

I think this issue has very little to do with table partitioning per
se.  As Amit says, this is a longstanding behavior and it would have
been far stranger than where we are if the commit to implement table
partitioning had changed it.  I suggest starting new threads for
changes you want to make instead of tacking them all onto this one.

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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Alvaro Herrera]

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 
temporary relation \"%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 Herrerahttps://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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Alvaro Herrera]

Amit Langote wrote:
> On 2017/10/18 1:52, Alvaro Herrera wrote:
> > Alvaro Herrera wrote:
> >> Robert Haas wrote:
> >>> Implement table partitioning.
> >>
> >> Is it intentional that you can use ALTER TABLE OWNER TO on the parent
> >> table, and that this does not recurse to modify the partitions' owners?
> >> This doesn't seem to be mentioned in comments nor documentation, so it
> >> seems an oversight to me.
> 
> Hmm, I would say of it that the new partitioning didn't modify the
> behavior that existed for inheritance.
> 
> That said, I'm not sure if the lack of recursive application of ownership
> change to descendant tables is unintentional.

My view is that the fact that partitioning uses inheritance is just an
implementation detail.  We shouldn't let historical behavior for
inheritance dictate behavior for partitioning.  Inheritance has many
undesirable warts.

> > The alter table docs say that ONLY must be specified if one does not
> > want to modify descendants, so I think this is a bug.
> 
> Just to clarify, if we do think of it as a bug, then it will apply to the
> inheritance case as well, right?

I'd leave it alone.

-- 
Álvaro Herrerahttps://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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Amit Langote]

On 2017/10/18 1:52, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>> Robert Haas wrote:
>>> Implement table partitioning.
>>
>> Is it intentional that you can use ALTER TABLE OWNER TO on the parent
>> table, and that this does not recurse to modify the partitions' owners?
>> This doesn't seem to be mentioned in comments nor documentation, so it
>> seems an oversight to me.

Hmm, I would say of it that the new partitioning didn't modify the
behavior that existed for inheritance.

That said, I'm not sure if the lack of recursive application of ownership
change to descendant tables is unintentional.

> The alter table docs say that ONLY must be specified if one does not
> want to modify descendants, so I think this is a bug.

Just to clarify, if we do think of it as a bug, then it will apply to the
inheritance case as well, right?

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Alvaro Herrera]

Alvaro Herrera wrote:
> Robert Haas wrote:
> > Implement table partitioning.
> 
> Is it intentional that you can use ALTER TABLE OWNER TO on the parent
> table, and that this does not recurse to modify the partitions' owners?
> This doesn't seem to be mentioned in comments nor documentation, so it
> seems an oversight to me.

The alter table docs say that ONLY must be specified if one does not
want to modify descendants, so I think this is a bug.

-- 
Álvaro Herrerahttps://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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Alvaro Herrera]

Robert Haas wrote:
> Implement table partitioning.

Is it intentional that you can use ALTER TABLE OWNER TO on the parent
table, and that this does not recurse to modify the partitions' owners?
This doesn't seem to be mentioned in comments nor documentation, so it
seems an oversight to me.

Thoughts?

-- 
Álvaro Herrerahttps://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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Fri, Oct 13, 2017 at 12:34 PM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>> Implement table partitioning.
>
>> Currently, tables can be range-partitioned or list-partitioned.  List
>> partitioning is limited to a single column, but range partitioning can
>> involve multiple columns.  A partitioning "column" can be an
>> expression.
>
> I find the "partition strategy" thing a bit suspect code-wise.  I was a
> bit bothered by all the "default:" clauses in switches that deal with
> the possible values, and I was going to propose simply that we turn that
> into an enum -- a trivial patch, I thought.  Not so: the way it's used
> by the grammar is a bit odd.  Apparently, it starts life as a string
> (either "list" or "range"), and then transformPartitionSpec() has to go
> out of its way to return it as a char.
>
> I propose we have gram.y itself resolve the strings to either 'list' or
> 'range' and that the node contains only those values, not any string.
> Unless there is a reason why things are like this that I'm not seeing?

I don't think I feel strongly about this, but I'm also not sure
exactly what problem you are trying to solve.  Do you want to
elaborate on that a bit?

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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Alvaro Herrera]

Robert Haas wrote:
> Implement table partitioning.

> Currently, tables can be range-partitioned or list-partitioned.  List
> partitioning is limited to a single column, but range partitioning can
> involve multiple columns.  A partitioning "column" can be an
> expression.

I find the "partition strategy" thing a bit suspect code-wise.  I was a
bit bothered by all the "default:" clauses in switches that deal with
the possible values, and I was going to propose simply that we turn that
into an enum -- a trivial patch, I thought.  Not so: the way it's used
by the grammar is a bit odd.  Apparently, it starts life as a string
(either "list" or "range"), and then transformPartitionSpec() has to go
out of its way to return it as a char.

I propose we have gram.y itself resolve the strings to either 'list' or
'range' and that the node contains only those values, not any string.
Unless there is a reason why things are like this that I'm not seeing?

-- 
Álvaro Herrerahttps://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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Wed, Dec 21, 2016 at 8:00 PM, Amit Langote
 wrote:
> On 2016/12/22 0:31, Robert Haas wrote:
>> On Tue, Dec 20, 2016 at 12:22 PM, Alvaro Herrera
>>  wrote:
>>> Robert Haas wrote:
 Implement table partitioning.
>>>
>>> I thought it was odd to use rd_rel->reloftype as a boolean in
>>> ATExecAttachPartition, but apparently we do it elsewhere too, so let's
>>> leave that complaint for another day.
>>
>> Ugh.  I agree - that's bad style.
>
> Agreed, fixed in the attached patch.
>
>>> What I also found off in the same function is that we use
>>> SearchSysCacheCopyAttName() on each attribute and then don't free the
>>> result, and don't ever use the returned tuple either.  A simple fix, I
>>> thought, just remove the "Copy" and add a ReleaseSysCache().
>>
>> Or use SearchSysCachExists.
>
> Done, too.
>
>>> But then I
>>> noticed this whole thing is rather strange -- why not pass a boolean
>>> flag down to CreateInheritance() and from there to
>>> MergeAttributesIntoExisting() to implement the check?  That seems less
>>> duplicative.
>>
>> Hmm, that would be another way to do it.
>
> MergeAttributesIntoExisting() is called by ATExecAddInherit() as well,
> where we don't want to check that.  Sure, we can only check if
> child_is_partition, but I thought it'd be better to keep the shared code
> (between regular inheritance and partitioning) as close to the old close
> as possible.
>
> Attached patch also fixes a couple of other minor issues.

Committed.

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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Amit Langote]

On 2016/12/22 0:31, Robert Haas wrote:
> On Tue, Dec 20, 2016 at 12:22 PM, Alvaro Herrera
>  wrote:
>> Robert Haas wrote:
>>> Implement table partitioning.
>>
>> I thought it was odd to use rd_rel->reloftype as a boolean in
>> ATExecAttachPartition, but apparently we do it elsewhere too, so let's
>> leave that complaint for another day.
> 
> Ugh.  I agree - that's bad style.

Agreed, fixed in the attached patch.

>> What I also found off in the same function is that we use
>> SearchSysCacheCopyAttName() on each attribute and then don't free the
>> result, and don't ever use the returned tuple either.  A simple fix, I
>> thought, just remove the "Copy" and add a ReleaseSysCache().
> 
> Or use SearchSysCachExists.

Done, too.

>> But then I
>> noticed this whole thing is rather strange -- why not pass a boolean
>> flag down to CreateInheritance() and from there to
>> MergeAttributesIntoExisting() to implement the check?  That seems less
>> duplicative.
> 
> Hmm, that would be another way to do it.

MergeAttributesIntoExisting() is called by ATExecAddInherit() as well,
where we don't want to check that.  Sure, we can only check if
child_is_partition, but I thought it'd be better to keep the shared code
(between regular inheritance and partitioning) as close to the old close
as possible.

Attached patch also fixes a couple of other minor issues.

Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 115b98313e..ee79b726f2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12996,7 +12996,6 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
 			   *existConstraint;
 	SysScanDesc scan;
 	ScanKeyData skey;
-	HeapTuple	tuple;
 	AttrNumber	attno;
 	int			natts;
 	TupleDesc	tupleDesc;
@@ -13018,7 +13017,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
  errmsg("\"%s\" is already a partition",
 		RelationGetRelationName(attachRel;
 
-	if (attachRel->rd_rel->reloftype)
+	if (OidIsValid(attachRel->rd_rel->reloftype))
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
  errmsg("cannot attach a typed table as partition")));
@@ -13119,9 +13118,10 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
 		if (attribute->attisdropped)
 			continue;
 
-		/* Find same column in parent (matching on column name). */
-		tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), attributeName);
-		if (!HeapTupleIsValid(tuple))
+		/* Try to find the column in parent (matching on column name) */
+		if (!SearchSysCacheExists2(ATTNAME,
+   ObjectIdGetDatum(RelationGetRelid(rel)),
+   CStringGetDatum(attributeName)))
 			ereport(ERROR,
 	(errcode(ERRCODE_DATATYPE_MISMATCH),
 	 errmsg("table \"%s\" contains column \"%s\" not found in parent \"%s\"",
@@ -13167,7 +13167,6 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
 	 * There is a case in which we cannot rely on just the result of the
 	 * proof.
 	 */
-	tupleDesc = RelationGetDescr(attachRel);
 	attachRel_constr = tupleDesc->constr;
 	existConstraint = NIL;
 	if (attachRel_constr != NULL)

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Tue, Dec 20, 2016 at 12:22 PM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>> Implement table partitioning.
>
> I thought it was odd to use rd_rel->reloftype as a boolean in
> ATExecAttachPartition, but apparently we do it elsewhere too, so let's
> leave that complaint for another day.

Ugh.  I agree - that's bad style.

> What I also found off in the same function is that we use
> SearchSysCacheCopyAttName() on each attribute and then don't free the
> result, and don't ever use the returned tuple either.  A simple fix, I
> thought, just remove the "Copy" and add a ReleaseSysCache().

Or use SearchSysCachExists.

> But then I
> noticed this whole thing is rather strange -- why not pass a boolean
> flag down to CreateInheritance() and from there to
> MergeAttributesIntoExisting() to implement the check?  That seems less
> duplicative.

Hmm, that would be another way to do it.

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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Alvaro Herrera]

Robert Haas wrote:
> Implement table partitioning.

I thought it was odd to use rd_rel->reloftype as a boolean in
ATExecAttachPartition, but apparently we do it elsewhere too, so let's
leave that complaint for another day.

What I also found off in the same function is that we use
SearchSysCacheCopyAttName() on each attribute and then don't free the
result, and don't ever use the returned tuple either.  A simple fix, I
thought, just remove the "Copy" and add a ReleaseSysCache().  But then I
noticed this whole thing is rather strange -- why not pass a boolean
flag down to CreateInheritance() and from there to
MergeAttributesIntoExisting() to implement the check?  That seems less
duplicative.

-- 
Álvaro Herrerahttps://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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Amit Langote]

On 2016/12/10 7:55, Keith Fiske wrote:
> Working on a blog post for this feature and just found some more
> inconsistencies with the doc examples. Looks like the city_id column was
> defined in the measurements table when it should be in the cities table.
> The addition of the partition to the cities table fails since it's missing.
> 
> Examples should look like this:
> 
> CREATE TABLE measurement (
> logdate date not null,
> peaktempint,
> unitsales   int
> ) PARTITION BY RANGE (logdate);
> 
> CREATE TABLE cities (
> city_id bigserial not null,
> name text not null,
> population   int
> ) PARTITION BY LIST (initcap(name));
> 
> I actually changed my example to have city_id use bigserial to show that
> sequences are inherited automatically. May be good to show that in the docs.

Attached is a documentation patch fixing inconsistencies in the examples
that Keith reports and also improve them a bit (cities_west example sounds
a bit contrived now that I think).

Also, I posted a patch earlier [1] to mention the limitation that row
movement caused by UPDATE is treated an error.  I have combined it into
this patch, so that all the documentation fixes proposed are together.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/a4f261c2-8554-f443-05ff-d97dddc19689%40lab.ntt.co.jp
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index a6a43c4b30..333b01db36 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -715,7 +715,7 @@ ALTER TABLE [ IF EXISTS ] name

 

-ATTACH PARTITION partition_name partition_bound_spec
+ATTACH PARTITION partition_name FOR VALUES partition_bound_spec
 
  
   This form attaches an existing table (which might itself be partitioned)
@@ -1332,7 +1332,7 @@ ALTER TABLE measurement
Attach a partition to list partitioned table:
 
 ALTER TABLE cities
-ATTACH PARTITION cities_west FOR VALUES IN ('Los Angeles', 'San Francisco');
+ATTACH PARTITION cities_ab FOR VALUES IN ('a', 'b');
 
 
   
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 8bf8af302b..58f8bf6d6a 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -248,7 +248,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI

 

-PARTITION OF parent_table
+PARTITION OF parent_table FOR VALUES partition_bound_spec
 
  
   Creates the table as partition of the specified
@@ -275,7 +275,8 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
  
   Rows inserted into a partitioned table will be automatically routed to
   the correct partition.  If no suitable partition exists, an error will
-  occur.
+  occur.  Also, if updating a row in a given partition causes it to move
+  to another partition due to the new partition key, an error will occur.
  
 
  
@@ -1477,7 +1478,6 @@ CREATE TABLE employees OF employee_type (
Create a range partitioned table:
 
 CREATE TABLE measurement (
-city_id int not null,
 logdate date not null,
 peaktempint,
 unitsales   int
@@ -1488,9 +1488,10 @@ CREATE TABLE measurement (
Create a list partitioned table:
 
 CREATE TABLE cities (
+city_id  bigserial not null,
 name text not null,
-population   int,
-) PARTITION BY LIST (initcap(name));
+population   bigint,
+) PARTITION BY LIST (left(lower(name), 1));
 
 
   
@@ -1498,30 +1499,30 @@ CREATE TABLE cities (
 
 CREATE TABLE measurement_y2016m07
 PARTITION OF measurement (
-unitsales WITH OPTIONS DEFAULT 0
+unitsales DEFAULT 0
 ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
 
 
   
Create partition of a list partitioned table:
 
-CREATE TABLE cities_west
+CREATE TABLE cities_ab
 PARTITION OF cities (
 CONSTRAINT city_id_nonzero CHECK (city_id != 0)
-) FOR VALUES IN ('Los Angeles', 'San Francisco');
+) FOR VALUES IN ('a', 'b');
 
 
   
Create partition of a list partitioned table that is itself further
partitioned and then add a partition to it:
 
-CREATE TABLE cities_west
+CREATE TABLE cities_ab
 PARTITION OF cities (
 CONSTRAINT city_id_nonzero CHECK (city_id != 0)
-) FOR VALUES IN ('Los Angeles', 'San Francisco') PARTITION BY RANGE (population);
+) FOR VALUES IN ('a', 'b') PARTITION BY RANGE (population);
 
-CREATE TABLE cities_west_1_to_10
-PARTITION OF cities_west FOR VALUES FROM (1) TO (10);
+CREATE TABLE cities_ab_1_to_10
+PARTITION OF cities_ab FOR VALUES FROM (1) TO (10);
 
  
 
diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index 06f416039b..00c984d8d5 100644
--- a/doc/src/sgml/ref/insert.sgml
+++ b/doc/src/sgml/ref/insert.sgml
@@ -526,6 +526,17 @@ INSERT oid count
  
+ 
+ 
+  Notes
+
+  
+   If the 

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Jim Nasby]

On 12/10/16 1:02 PM, Christophe Pettus wrote:



On Dec 9, 2016, at 22:52, Keith Fiske  wrote:
On Fri, Dec 9, 2016 at 10:01 PM, Robert Haas  wrote:

One thing that's tricky/annoying about this is that if you have a
DEFAULT partition and then add a partition, you have to scan the
DEFAULT partition for data that should be moved to the new partition.
That makes what would otherwise be a quick operation slow.  Still, I'm
sure there's a market for that feature.


I would find that perfectly acceptable as long as a caveat about the 
performance impact was included in the documentation.


+1.  I don't think it's conceptually different from adding a column with a 
default, in that regard; you just have to know the impact.


FWIW, I can think of another option: always check the default partition 
for data, even if the data should only exist in a specific partition. If 
that proved to be too expensive in the normal case it could be optional.


Is it possible to manually (and incrementally) move data from the 
default partition to a table that will become the partition for that 
data and then do a fast cut-over once that's done? That would be akin to 
adding a field without a DEFAULT, adding the DEFAULT after that, and 
then slowly updating all the existing rows...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Christophe Pettus]

> On Dec 9, 2016, at 22:52, Keith Fiske  wrote:
> On Fri, Dec 9, 2016 at 10:01 PM, Robert Haas  wrote:
>> One thing that's tricky/annoying about this is that if you have a
>> DEFAULT partition and then add a partition, you have to scan the
>> DEFAULT partition for data that should be moved to the new partition.
>> That makes what would otherwise be a quick operation slow.  Still, I'm
>> sure there's a market for that feature.
> 
> I would find that perfectly acceptable as long as a caveat about the 
> performance impact was included in the documentation.

+1.  I don't think it's conceptually different from adding a column with a 
default, in that regard; you just have to know the impact.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Keith Fiske]

On Fri, Dec 9, 2016 at 10:01 PM, Robert Haas  wrote:

> On Fri, Dec 9, 2016 at 5:55 PM, Keith Fiske  wrote:
> > Another suggestion I had was for handling when data is inserted that
> doesn't
> > match any defined child tables. Right now it just errors out, but in
> > pg_partman I'd had it send the data to the parent instead to avoid data
> > loss. I know that's not possible here, but how about syntax to define a
> > child table as a "default" to take data that would normally be rejected?
> > Maybe something like
> >
> > CREATE TABLE measurement_default
> > PARTITION OF measurement (
> > unitsales DEFAULT 0
> > ) FOR VALUES DEFAULT;
>
> One thing that's tricky/annoying about this is that if you have a
> DEFAULT partition and then add a partition, you have to scan the
> DEFAULT partition for data that should be moved to the new partition.
> That makes what would otherwise be a quick operation slow.  Still, I'm
> sure there's a market for that feature.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

I would find that perfectly acceptable as long as a caveat about the
performance impact was included in the documentation. My intent with
putting the data in the parent in pg_partman was solely to avoid data loss
and I also included a function for monitoring if data went into the parent.
That sort of function may not have real utility in core, but I think the
intent of the DEFAULT location is a catchall "just in case" and not really
intended as a permanent data store. If people did use it that way, and a
warning was included about its cost when adding new partitions, then that's
on the user for doing that.

I recall reading in the other thread about this that you're looking to make
locking across the partition set less strict eventually. If you could make
the scan and data move not block on anything except the partitions
involved, I think the performance impact of scanning the default partition
and moving the data wouldn't even be that bad in the end.

Keith

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Fri, Dec 9, 2016 at 5:55 PM, Keith Fiske  wrote:
> Another suggestion I had was for handling when data is inserted that doesn't
> match any defined child tables. Right now it just errors out, but in
> pg_partman I'd had it send the data to the parent instead to avoid data
> loss. I know that's not possible here, but how about syntax to define a
> child table as a "default" to take data that would normally be rejected?
> Maybe something like
>
> CREATE TABLE measurement_default
> PARTITION OF measurement (
> unitsales DEFAULT 0
> ) FOR VALUES DEFAULT;

One thing that's tricky/annoying about this is that if you have a
DEFAULT partition and then add a partition, you have to scan the
DEFAULT partition for data that should be moved to the new partition.
That makes what would otherwise be a quick operation slow.  Still, I'm
sure there's a market for that feature.

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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Keith Fiske]

On Fri, Dec 9, 2016 at 1:23 PM, Keith Fiske  wrote:

>
>
> On Fri, Dec 9, 2016 at 1:13 PM, Amit Langote 
> wrote:
>
>> Hi Keith,
>>
>> On Sat, Dec 10, 2016 at 3:00 AM, Keith Fiske  wrote:
>> > Being that table partitioning is something I'm slightly interested in,
>> > figured I'd give it a whirl.
>> >
>> > This example in the docs has an extraneous comma after the second column
>> >
>> > CREATE TABLE cities (
>> > name text not null,
>> > population   int,
>> > ) PARTITION BY LIST (initcap(name));
>> >
>> > And the WITH OPTIONS clause does not appear to be working using another
>> > example from the docs. Not seeing any obvious typos.
>> >
>> > keith@keith=# CREATE TABLE measurement_y2016m07
>> > keith-# PARTITION OF measurement (
>> > keith(# unitsales WITH OPTIONS DEFAULT 0
>> > keith(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
>> > 2016-12-09 12:51:48.728 EST [11711] ERROR:  syntax error at or near
>> "WITH"
>> > at character 80
>> > 2016-12-09 12:51:48.728 EST [11711] STATEMENT:  CREATE TABLE
>> > measurement_y2016m07
>> > PARTITION OF measurement (
>> > unitsales WITH OPTIONS DEFAULT 0
>> > ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
>> > ERROR:  syntax error at or near "WITH"
>> > LINE 3: unitsales WITH OPTIONS DEFAULT 0
>> >   ^
>> > Time: 0.184 ms
>> >
>> > Removing the unit_sales default allows it to work fine
>>
>> WITH OPTIONS keyword phrase is something that was made redundant in
>> the last version of the patch, but I forgot to remove the same in the
>> example.  I've sent a doc patch to fix that.
>>
>> If you try - unitsales DEFAULT 0, it will work.  Note that I did not
>> specify WITH OPTIONS.
>>
>> Thanks,
>> Amit
>>
>
> That works. Thanks!
>
> keith@keith=# CREATE TABLE measurement_y2016m07
> PARTITION OF measurement (
> unitsales DEFAULT 0
> ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> CREATE TABLE
> Time: 4.091 ms
>
>
Working on a blog post for this feature and just found some more
inconsistencies with the doc examples. Looks like the city_id column was
defined in the measurements table when it should be in the cities table.
The addition of the partition to the cities table fails since it's missing.

Examples should look like this:

CREATE TABLE measurement (
logdate date not null,
peaktempint,
unitsales   int
) PARTITION BY RANGE (logdate);

CREATE TABLE cities (
city_id bigserial not null,
name text not null,
population   int
) PARTITION BY LIST (initcap(name));

I actually changed my example to have city_id use bigserial to show that
sequences are inherited automatically. May be good to show that in the docs.

Another suggestion I had was for handling when data is inserted that
doesn't match any defined child tables. Right now it just errors out, but
in pg_partman I'd had it send the data to the parent instead to avoid data
loss. I know that's not possible here, but how about syntax to define a
child table as a "default" to take data that would normally be rejected?
Maybe something like

CREATE TABLE measurement_default
PARTITION OF measurement (
unitsales DEFAULT 0
) FOR VALUES DEFAULT;

Keith

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Keith Fiske]

On Fri, Dec 9, 2016 at 1:13 PM, Amit Langote 
wrote:

> Hi Keith,
>
> On Sat, Dec 10, 2016 at 3:00 AM, Keith Fiske  wrote:
> > Being that table partitioning is something I'm slightly interested in,
> > figured I'd give it a whirl.
> >
> > This example in the docs has an extraneous comma after the second column
> >
> > CREATE TABLE cities (
> > name text not null,
> > population   int,
> > ) PARTITION BY LIST (initcap(name));
> >
> > And the WITH OPTIONS clause does not appear to be working using another
> > example from the docs. Not seeing any obvious typos.
> >
> > keith@keith=# CREATE TABLE measurement_y2016m07
> > keith-# PARTITION OF measurement (
> > keith(# unitsales WITH OPTIONS DEFAULT 0
> > keith(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> > 2016-12-09 12:51:48.728 EST [11711] ERROR:  syntax error at or near
> "WITH"
> > at character 80
> > 2016-12-09 12:51:48.728 EST [11711] STATEMENT:  CREATE TABLE
> > measurement_y2016m07
> > PARTITION OF measurement (
> > unitsales WITH OPTIONS DEFAULT 0
> > ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> > ERROR:  syntax error at or near "WITH"
> > LINE 3: unitsales WITH OPTIONS DEFAULT 0
> >   ^
> > Time: 0.184 ms
> >
> > Removing the unit_sales default allows it to work fine
>
> WITH OPTIONS keyword phrase is something that was made redundant in
> the last version of the patch, but I forgot to remove the same in the
> example.  I've sent a doc patch to fix that.
>
> If you try - unitsales DEFAULT 0, it will work.  Note that I did not
> specify WITH OPTIONS.
>
> Thanks,
> Amit
>

That works. Thanks!

keith@keith=# CREATE TABLE measurement_y2016m07
PARTITION OF measurement (
unitsales DEFAULT 0
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
CREATE TABLE
Time: 4.091 ms

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Amit Langote]

Hi Keith,

On Sat, Dec 10, 2016 at 3:00 AM, Keith Fiske  wrote:
> Being that table partitioning is something I'm slightly interested in,
> figured I'd give it a whirl.
>
> This example in the docs has an extraneous comma after the second column
>
> CREATE TABLE cities (
> name text not null,
> population   int,
> ) PARTITION BY LIST (initcap(name));
>
> And the WITH OPTIONS clause does not appear to be working using another
> example from the docs. Not seeing any obvious typos.
>
> keith@keith=# CREATE TABLE measurement_y2016m07
> keith-# PARTITION OF measurement (
> keith(# unitsales WITH OPTIONS DEFAULT 0
> keith(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> 2016-12-09 12:51:48.728 EST [11711] ERROR:  syntax error at or near "WITH"
> at character 80
> 2016-12-09 12:51:48.728 EST [11711] STATEMENT:  CREATE TABLE
> measurement_y2016m07
> PARTITION OF measurement (
> unitsales WITH OPTIONS DEFAULT 0
> ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> ERROR:  syntax error at or near "WITH"
> LINE 3: unitsales WITH OPTIONS DEFAULT 0
>   ^
> Time: 0.184 ms
>
> Removing the unit_sales default allows it to work fine

WITH OPTIONS keyword phrase is something that was made redundant in
the last version of the patch, but I forgot to remove the same in the
example.  I've sent a doc patch to fix that.

If you try - unitsales DEFAULT 0, it will work.  Note that I did not
specify WITH OPTIONS.

Thanks,
Amit


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Keith Fiske]

On Wed, Dec 7, 2016 at 1:30 PM, Robert Haas  wrote:

> On Wed, Dec 7, 2016 at 1:20 PM, Robert Haas  wrote:
> > Implement table partitioning.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Being that table partitioning is something I'm slightly interested in,
figured I'd give it a whirl.

This example in the docs has an extraneous comma after the second column

CREATE TABLE cities (
name text not null,
population   int,
) PARTITION BY LIST (initcap(name));

And the WITH OPTIONS clause does not appear to be working using another
example from the docs. Not seeing any obvious typos.

keith@keith=# CREATE TABLE measurement_y2016m07
keith-# PARTITION OF measurement (
keith(# unitsales WITH OPTIONS DEFAULT 0
keith(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
2016-12-09 12:51:48.728 EST [11711] ERROR:  syntax error at or near "WITH"
at character 80
2016-12-09 12:51:48.728 EST [11711] STATEMENT:  CREATE TABLE
measurement_y2016m07
PARTITION OF measurement (
unitsales WITH OPTIONS DEFAULT 0
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
ERROR:  syntax error at or near "WITH"
LINE 3: unitsales WITH OPTIONS DEFAULT 0
  ^
Time: 0.184 ms

Removing the unit_sales default allows it to work fine

keith@keith=# CREATE TABLE measurement_y2016m07
PARTITION OF measurement
 FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
CREATE TABLE
Time: 5.001 ms

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Dec 8, 2016 at 2:11 PM, Stephen Frost  wrote:
> > Yes, that makes the compiler warning go away.
> 
> Great, pushed.

Awesome, thanks!

> > ... your compiler knows that key->partnatts will always be >= 1?
> 
> :-)
> 
> I think my compiler is too dumb to notice that int x; printf("%d", x);
> is a reference to an uninitialized variable.

Made me laugh, thanks again. :)

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Thu, Dec 8, 2016 at 2:11 PM, Stephen Frost  wrote:
> Yes, that makes the compiler warning go away.

Great, pushed.

> ... your compiler knows that key->partnatts will always be >= 1?

:-)

I think my compiler is too dumb to notice that int x; printf("%d", x);
is a reference to an uninitialized variable.

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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Dec 8, 2016 at 1:49 PM, Stephen Frost  wrote:
> > * Robert Haas (rh...@postgresql.org) wrote:
> >> Implement table partitioning.
> >
> > My compiler apparently doesn't care for this:
> >
> > .../src/backend/catalog/partition.c: In function ‘partition_rbound_cmp’:
> > .../src/backend/catalog/partition.c:1787:13: warning: ‘cmpval’ may be used 
> > uninitialized in this function [-Wmaybe-uninitialized]
> >   if (cmpval == 0 && lower1 != lower2)
> 
> So, apparently your compiler doesn't recognize that the loop always
> has to execute at least once, because we don't support a table
> partitioned on zero attributes.  If you initialize cmpval to 0 at the
> top of the function, does that fix it?

Yes, that makes the compiler warning go away.

... your compiler knows that key->partnatts will always be >= 1?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Thu, Dec 8, 2016 at 1:49 PM, Stephen Frost  wrote:
> * Robert Haas (rh...@postgresql.org) wrote:
>> Implement table partitioning.
>
> My compiler apparently doesn't care for this:
>
> .../src/backend/catalog/partition.c: In function ‘partition_rbound_cmp’:
> .../src/backend/catalog/partition.c:1787:13: warning: ‘cmpval’ may be used 
> uninitialized in this function [-Wmaybe-uninitialized]
>   if (cmpval == 0 && lower1 != lower2)

So, apparently your compiler doesn't recognize that the loop always
has to execute at least once, because we don't support a table
partitioned on zero attributes.  If you initialize cmpval to 0 at the
top of the function, does that fix it?

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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Stephen Frost]

* Robert Haas (rh...@postgresql.org) wrote:
> Implement table partitioning.

My compiler apparently doesn't care for this:

.../src/backend/catalog/partition.c: In function ‘partition_rbound_cmp’:
.../src/backend/catalog/partition.c:1787:13: warning: ‘cmpval’ may be used 
uninitialized in this function [-Wmaybe-uninitialized]
  if (cmpval == 0 && lower1 != lower2)
 ^

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Amit Langote]

On 2016/12/08 3:33, Robert Haas wrote:
> On Wed, Dec 7, 2016 at 1:30 PM, Robert Haas  wrote:
>>   -- partitioned table cannot partiticipate in regular inheritance
>>   CREATE TABLE partitioned2 (
>>   a int
>> --- 392,411 
>>   c text,
>>   d text
>>   ) PARTITION BY RANGE (a oid_ops, plusone(b), c collate "default", d
>> collate "en_US");
>> + ERROR:  collation "en_US" for encoding "SQL_ASCII" does not exist
> ...
>> No idea why yet, but I'll try to figure it out.
> 
> And of course that'd be because relying on en_US isn't portable.  Sigh.

Should've thought about the non-portability of locales.  Thanks for
catching and fixing anyway!

Thanks,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Wed, Dec 7, 2016 at 3:13 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> And of course that'd be because relying on en_US isn't portable.  Sigh.
>
> You can't rely on *any* collations other than C and POSIX.

I get it; I just missed that during review, and then sent that message
before I even looked at it carefully, so that you would know I was
working on it.  I think that it's fixed now; at any rate, the
buildfarm seems happy enough.

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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Tom Lane]

Robert Haas  writes:
> And of course that'd be because relying on en_US isn't portable.  Sigh.

You can't rely on *any* collations other than C and POSIX.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Wed, Dec 7, 2016 at 1:30 PM, Robert Haas  wrote:
>   -- partitioned table cannot partiticipate in regular inheritance
>   CREATE TABLE partitioned2 (
>   a int
> --- 392,411 
>   c text,
>   d text
>   ) PARTITION BY RANGE (a oid_ops, plusone(b), c collate "default", d
> collate "en_US");
> + ERROR:  collation "en_US" for encoding "SQL_ASCII" does not exist
...
> No idea why yet, but I'll try to figure it out.

And of course that'd be because relying on en_US isn't portable.  Sigh.

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

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

[Robert Haas]

On Wed, Dec 7, 2016 at 1:20 PM, Robert Haas  wrote:
> Implement table partitioning.

Well, that didn't take long to cause problems.  The very first
buildfarm machine to report after this commit is longfin, which is
unhappy:

***
*** 392,419 
  c text,
  d text
  ) PARTITION BY RANGE (a oid_ops, plusone(b), c collate "default", d
collate "en_US");
  -- check relkind
  SELECT relkind FROM pg_class WHERE relname = 'partitioned';
   relkind
  -
!  P
! (1 row)

  -- check that range partition key columns are marked NOT NULL
  SELECT attname, attnotnull FROM pg_attribute WHERE attrelid =
'partitioned'::regclass AND attnum > 0;
!  attname | attnotnull
! -+
!  a   | t
!  b   | f
!  c   | t
!  d   | t
! (4 rows)
!
  -- prevent a function referenced in partition key from being dropped
  DROP FUNCTION plusone(int);
- ERROR:  cannot drop function plusone(integer) because other objects
depend on it
- DETAIL:  table partitioned depends on function plusone(integer)
- HINT:  Use DROP ... CASCADE to drop the dependent objects too.
  -- partitioned table cannot partiticipate in regular inheritance
  CREATE TABLE partitioned2 (
  a int
--- 392,411 
  c text,
  d text
  ) PARTITION BY RANGE (a oid_ops, plusone(b), c collate "default", d
collate "en_US");
+ ERROR:  collation "en_US" for encoding "SQL_ASCII" does not exist
  -- check relkind
  SELECT relkind FROM pg_class WHERE relname = 'partitioned';
   relkind
  -
! (0 rows)

  -- check that range partition key columns are marked NOT NULL
  SELECT attname, attnotnull FROM pg_attribute WHERE attrelid =
'partitioned'::regclass AND attnum > 0;
! ERROR:  relation "partitioned" does not exist
! LINE 1: ...me, attnotnull FROM pg_attribute WHERE attrelid = 'partition...
!  ^
  -- prevent a function referenced in partition key from being dropped
  DROP FUNCTION plusone(int);
  -- partitioned table cannot partiticipate in regular inheritance
  CREATE TABLE partitioned2 (
  a int

No idea why yet, but I'll try to figure it out.

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