Обсуждение: ADD/DROP INHERITS

I've implemented most of ADD/DROP INHERITS but it's my first significant piece
of code at this level. I would appreciate any feedback about it. In particular
I'm worried I may be on the wrong track about how some low level operations
work like memory management, syscache lookups, heap tuple creation etc. Also,
I'm not at all clear what kind of locks are really necessary for this
operation. I may be taking excessively strong or weak locks or have deadlock
risks.

The main thing remaining to be done is implementing default column
expressions. Those would require an Alter Table "Pass 3" operation I believe.
Also I haven't looked at table constraints at all yet, I'm not clear what's
supposed to happen there.

I made some decisions on some semantic issues that I believe are correct but
could stand some double checking. Specifically If the parent has oids then the
child must have oids and if a column in the parent is NOT NULL then the column
in the child must be NOT NULL as well.

I can send the actual patch to psql-patches, it includes some other changes to
refactor StoreCatalogInheritance and add the syntax to gram.y. But it's still
not quite finished because of default values.




static void
ATExecAddInherits(Relation rel, RangeVar *parent)
{Relation relation, catalogRelation;SysScanDesc scan;ScanKeyData key;HeapTuple inheritsTuple;int4 inhseqno = 0;ListCell
 *child;List       *children;
 
relation = heap_openrv(parent, AccessShareLock); /* XXX is this enough locking? */if (relation->rd_rel->relkind !=
RELKIND_RELATION)   ereport(ERROR,            (errcode(ERRCODE_WRONG_OBJECT_TYPE),             errmsg("inherited
relation\"%s\" is not a table",                    parent->relname)));
 

/* Permanent rels cannot inherit from temporary ones */if (!rel->rd_istemp &&
isTempNamespace(RelationGetNamespace(relation)))   ereport(ERROR,            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
     errmsg("cannot inherit from temporary relation \"%s\"",                    parent->relname)));if
(!pg_class_ownercheck(RelationGetRelid(relation),GetUserId()))    aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
              RelationGetRelationName(relation));
 
/* If parent has OIDs then all children must have OIDs */if (relation->rd_rel->relhasoids && !rel->rd_rel->relhasoids)
 ereport(ERROR,            (errcode(ERRCODE_WRONG_OBJECT_TYPE),             errmsg("table \"%s\" without OIDs cannot
inheritfrom table \"%s\" with OIDs",                    RelationGetRelationName(rel), parent->relname)));
 
/* * Reject duplications in the list of parents. -- this is the same check as * when creating a table, but maybe we
shouldcheck for the parent anywhere * higher in the inheritance structure? */catalogRelation =
heap_open(InheritsRelationId,RowExclusiveLock);ScanKeyInit(&key,            Anum_pg_inherits_inhrelid,
BTEqualStrategyNumber,F_OIDEQ,            ObjectIdGetDatum(RelationGetRelid(rel)));scan =
systable_beginscan(catalogRelation,InheritsRelidSeqnoIndexId, true, SnapshotNow, 1, &key);while
(HeapTupleIsValid(inheritsTuple= systable_getnext(scan))){    Form_pg_inherits inh = (Form_pg_inherits)
GETSTRUCT(inheritsTuple);   if (inh->inhparent == RelationGetRelid(relation))        ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_TABLE),                errmsg("inherited relation \"%s\" duplicated",
parent->relname)));   if (inh->inhseqno > inhseqno)        inhseqno =
inh->inhseqno;}systable_endscan(scan);heap_close(catalogRelation,RowExclusiveLock);
 
/* Get children because we have to manually recurse and also because we * have to check for recursive inheritance
graphs*/
 
/* this routine is actually in the planner */children = find_all_inheritors(RelationGetRelid(rel));
if (list_member_oid(children, RelationGetRelid(relation)))    ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_TABLE),            errmsg("Circular inheritance structure found")));
 
foreach(child, children)    {        Oid            childrelid = lfirst_oid(child);        Relation    childrel;
        childrel = relation_open(childrelid, AccessExclusiveLock);        MergeAttributesIntoExisting(childrel,
relation);       relation_close(childrel, NoLock);    }catalogRelation = heap_open(InheritsRelationId,
RowExclusiveLock);StoreCatalogInheritance1(RelationGetRelid(rel),RelationGetRelid(relation), inhseqno+1,
catalogRelation);heap_close(catalogRelation,RowExclusiveLock);heap_close(relation, AccessShareLock);
 
}


static void
MergeAttributesIntoExisting(Relation rel, Relation relation) 
{Relation     attrdesc;AttrNumber    parent_attno, child_attno;TupleDesc    tupleDesc;TupleConstr *constr;HeapTuple
tuple;
child_attno = RelationGetNumberOfAttributes(rel);
tupleDesc = RelationGetDescr(relation);constr = tupleDesc->constr;
for (parent_attno = 1; parent_attno <= tupleDesc->natts;     parent_attno++){    Form_pg_attribute attribute =
tupleDesc->attrs[parent_attno- 1];    char       *attributeName = NameStr(attribute->attname);
 
    /* Ignore dropped columns in the parent. */    if (attribute->attisdropped)        continue;
    /* Does it conflict with an existing column? */    attrdesc = heap_open(AttributeRelationId, RowExclusiveLock);
    tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), attributeName);    if (HeapTupleIsValid(tuple)) {
/*        * Yes, try to merge the two column definitions. They must         * have the same type and typmod.         */
      Form_pg_attribute childatt = (Form_pg_attribute) GETSTRUCT(tuple);        ereport(NOTICE,
(errmsg("mergingcolumn \"%s\" with inherited definition",                        attributeName)));        if
(attribute->atttypid != childatt->atttypid ||            attribute->atttypmod != childatt->atttypmod ||
(attribute->attnotnull&& !childatt->attnotnull))            ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),                    errmsg("child table \"%s\" has different type for column
\"%s\"",                           RelationGetRelationName(rel), NameStr(attribute->attname))));
 
        childatt->attinhcount++;        simple_heap_update(attrdesc, &tuple->t_self, tuple);
CatalogUpdateIndexes(attrdesc,tuple); /* XXX strength reduce openindexes to outside loop? */
heap_freetuple(tuple);               /* XXX defaults */
 
    } else {        /*         * No, create a new inherited column         */                FormData_pg_attribute
attributeD;       HeapTuple attributeTuple = heap_addheader(Natts_pg_attribute,
        false,                                                  ATTRIBUTE_TUPLE_SIZE,
              (void *) &attributeD);        Form_pg_attribute childatt = (Form_pg_attribute)
GETSTRUCT(attributeTuple);
        if (attribute->attnotnull)            ereport(ERROR,                    (errcode(ERRCODE_DATATYPE_MISMATCH),
                errmsg("Cannot add new inherited NOT NULL column \"%s\"",
NameStr(attribute->attname))));
        childatt->attrelid = RelationGetRelid(rel);        namecpy(&childatt->attname, &attribute->attname);
childatt->atttypid= attribute->atttypid;        childatt->attstattarget = -1;        childatt->attlen =
attribute->attlen;       childatt->attcacheoff = -1;        childatt->atttypmod = attribute->atttypmod;
childatt->attnum= ++child_attno;        childatt->attbyval = attribute->attbyval;        childatt->attndims =
attribute->attndims;       childatt->attstorage = attribute->attstorage;        childatt->attalign =
attribute->attalign;       childatt->attnotnull = false;        childatt->atthasdef = false; /* XXX */
childatt->attisdropped= false;        childatt->attislocal = false;        childatt->attinhcount =
attribute->attinhcount+1;
        simple_heap_insert(attrdesc, attributeTuple);        CatalogUpdateIndexes(attrdesc, attributeTuple);
heap_freetuple(attributeTuple);
        /* XXX Defaults */
    }    heap_close(attrdesc, RowExclusiveLock);}
}

static void
ATExecDropInherits(Relation rel, RangeVar *parent)
{

Relation    catalogRelation;SysScanDesc scan;ScanKeyData key;HeapTuple    inheritsTuple, attributeTuple;Oid
inhparent;Oid           dropparent;int         found = 0;/* Get the OID of parent -- if no schema is specified use the
regular* search path and only drop the one table that's found. We could try to be * clever and look at each parent and
seeif it matches but that would be * inconsistent with other operations I think. */Assert(rel);Assert(parent);
 
dropparent = RangeVarGetRelid(parent, false);
/* Search through the direct parents of rel looking for dropparent oid */
catalogRelation = heap_open(InheritsRelationId, RowExclusiveLock);ScanKeyInit(&key,
Anum_pg_inherits_inhrelid,           BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(RelationGetRelid(rel)));scan= systable_beginscan(catalogRelation, InheritsRelidSeqnoIndexId, true,
SnapshotNow,1, &key);while (!found && HeapTupleIsValid(inheritsTuple = systable_getnext(scan))){    inhparent =
((Form_pg_inherits)GETSTRUCT(inheritsTuple))->inhparent;    if (inhparent == dropparent) {
simple_heap_delete(catalogRelation,&inheritsTuple->t_self);        found = 1;
}}systable_endscan(scan);heap_close(catalogRelation,RowExclusiveLock);
 

if (!found) {    /* would it be better to look up the actual schema of dropparent and     * make the error message
explicitlyname the qualified name it's     * trying to drop ?*/    if (parent->schemaname)        ereport(ERROR,
       (errcode(ERRCODE_UNDEFINED_TABLE),                 errmsg("relation \"%s.%s\" is not a parent of relation
\"%s\"",                       parent->schemaname, parent->relname, RelationGetRelationName(rel))));    else
ereport(ERROR,               (errcode(ERRCODE_UNDEFINED_TABLE),                 errmsg("relation \"%s\" is not a parent
ofrelation \"%s\"",                        parent->relname, RelationGetRelationName(rel))));}/* Search through columns
lookingfor matching columns from parent table */
 
catalogRelation = heap_open(AttributeRelationId, RowExclusiveLock);ScanKeyInit(&key,
Anum_pg_attribute_attrelid,           BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(RelationGetRelid(rel)));scan= systable_beginscan(catalogRelation, AttributeRelidNumIndexId, true,
SnapshotNow,1, &key);while (HeapTupleIsValid(attributeTuple = systable_getnext(scan))) {    Form_pg_attribute att =
((Form_pg_attribute)GETSTRUCT(attributeTuple));   /* Not an inherited column at all     * (do NOT use islocal for this
test--itcan be true for inherited columns)     */    if (att->attinhcount == 0)         continue;     if
(att->attisdropped)/* XXX Is this right? */        continue;    if (SearchSysCacheExistsAttName(dropparent,
NameStr(att->attname))){        /* Decrement inhcount and possibly set islocal to 1 */        HeapTuple copyTuple =
heap_copytuple(attributeTuple);       Form_pg_attribute copy_att = ((Form_pg_attribute)GETSTRUCT(copyTuple));
 
        copy_att->attinhcount--;        if (copy_att->attinhcount == 0)            copy_att->attislocal = 1;
        simple_heap_update(catalogRelation, ©Tuple->t_self, copyTuple);        /* XXX "Avoid using it for multiple
tuples,since opening the         * indexes and building the index info structures is moderately         * expensive."
Perhapsthis can be moved outside the loop or else         * at least the CatalogOpenIndexes/CatalogCloseIndexes moved
     * outside the loop but when I try that it seg faults?!*/        CatalogUpdateIndexes(catalogRelation, copyTuple);
     heap_freetuple(copyTuple);    }}systable_endscan(scan);heap_close(catalogRelation, RowExclusiveLock);
 
}




-- 
greg

Greg Stark wrote:
>
> I can send the actual patch to psql-patches, it includes some other changes to
> refactor StoreCatalogInheritance and add the syntax to gram.y. But it's still
> not quite finished because of default values.
>
>   
You can send what you've got, and note that it's not for application 
yet. Post early and post often ;-) There are a surprising number of 
things to be done when you play with the syntax, as I found out not too 
long ago.

cheers

andrew

Greg Stark <gsstark@mit.edu> writes:
> I've implemented most of ADD/DROP INHERITS but it's my first significant piece
> of code at this level. I would appreciate any feedback about it.

I thought we had agreed that the semantics of ADD INHERITS would be to
reject the command if the child wasn't already suitable to be a child
of the parent.  Not to modify it by adding columns or constraints or
whatever.  For the proposed uses of ADD INHERITS (in particular,
linking and unlinking partition tables) an incompatibility in schema
almost certainly means you made a mistake, and you don't really want
the system helpfully "fixing" your table to match the parent.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> I thought we had agreed that the semantics of ADD INHERITS would be to
> reject the command if the child wasn't already suitable to be a child
> of the parent.  Not to modify it by adding columns or constraints or
> whatever.  For the proposed uses of ADD INHERITS (in particular,
> linking and unlinking partition tables) an incompatibility in schema
> almost certainly means you made a mistake, and you don't really want
> the system helpfully "fixing" your table to match the parent.

I didn't see any discussion like that and I find it pretty surprising.

Personally I would have agreed. For partitioned tables you certainly don't
want it to create new columns without warning you.

But that's entirely inconsistent with the way inherited tables work in
general. It seems to go against the grain of Postgres's general style to
implement just the use case that's useful for a particular application rather
than keep the features logically consistent with each other. 

Perhaps there should be an option when issuing the ADD INHERITS to indicate
whether you want it to create new columns or only match existing columns. That
would also give me a convenient excuse to skip all those NOTICEs about merging
column definitions.


Actually I think in the long term for partitioned tables Postgres will have to
implement a special syntax just like Oracle and other databases. The user
doesn't really want to have to manually manage all the partitions as tables.
That imposes a lot of extra work to have to define the tables with the right
syntax, maintain the constraints properly, etc.

For the user it would be better to have a single property of the partitioned
table that specified the partition key. Then when adding a partition you would
only have to specify the key range it covers, not write an arbitrary
constraint from scratch. Nor would you have to create an empty table with the
proper definition first then add it in.

-- 
greg

Greg Stark <gsstark@mit.edu> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> I thought we had agreed that the semantics of ADD INHERITS would be to
>> reject the command if the child wasn't already suitable to be a child
>> of the parent.

> I didn't see any discussion like that and I find it pretty surprising.

I'm pretty sure it was mentioned somewhere along the line.

> But that's entirely inconsistent with the way inherited tables work in
> general.

I don't see any basis for that conclusion.  The properties of a table
are set when it's created and you need to do pretty explicit ALTERs to
change them.  We do not for example automatically make a unique index
for a table when someone tries to reference a foreign key to a column
set that doesn't already have such an index.

In this situation, I think it's entirely reasonable to expect the user
to do any needed ALTER ADD COLUMN, CONSTRAINT, etc commands before
trying to attach a child table to a parent.  Having the system do it
for you offers no functionality gain, just a way to shoot yourself in
the foot.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Greg Stark <gsstark@mit.edu> writes:
> > Tom Lane <tgl@sss.pgh.pa.us> writes:
> >> I thought we had agreed that the semantics of ADD INHERITS would be to
> >> reject the command if the child wasn't already suitable to be a child
> >> of the parent.
> 
> > I didn't see any discussion like that and I find it pretty surprising.
> 
> I'm pretty sure it was mentioned somewhere along the line.
> 
> > But that's entirely inconsistent with the way inherited tables work in
> > general.
> 
> I don't see any basis for that conclusion.  The properties of a table
> are set when it's created and you need to do pretty explicit ALTERs to
> change them. 

It just seems weird for:

CREATE TABLE foo (x,y,z) INHERITS (bar)

to not be the equivalent to:

CREATE TABLE foo (x,y,z)
ALTER TABLE foo ADD INHERITS bar

> We do not for example automatically make a unique index for a table when
> someone tries to reference a foreign key to a column set that doesn't
> already have such an index.

But that's not really the same thing. Whether you add the foreign key later or
when you initially create the table it never creates that index.

On the other hand if you add a column to the parent it doesn't complain if not
all the children already have that column -- it goes and adds it recursively.


> In this situation, I think it's entirely reasonable to expect the user
> to do any needed ALTER ADD COLUMN, CONSTRAINT, etc commands before
> trying to attach a child table to a parent.  Having the system do it
> for you offers no functionality gain, just a way to shoot yourself in
> the foot.

Well if that's the consensus feeling then it certainly makes my life easier.

-- 
greg

Greg Stark <gsstark@mit.edu> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> In this situation, I think it's entirely reasonable to expect the user
>> to do any needed ALTER ADD COLUMN, CONSTRAINT, etc commands before
>> trying to attach a child table to a parent.  Having the system do it
>> for you offers no functionality gain, just a way to shoot yourself in
>> the foot.

> Well if that's the consensus feeling then it certainly makes my life easier.

Well, one reason for my position is exactly to make your life easier.
I think that making ADD INHERITS do all these other things automagically
is lily-gilding, or at least implementing features not shown to be
needed.  Let's make it do the minimum needed for the use-cases cited so
far --- we can always add more functionality later, *after* it's proven
needed.
        regards, tom lane

On Wed, Jun 07, 2006 at 03:33:54PM -0400, Greg Stark wrote:
> Perhaps there should be an option when issuing the ADD INHERITS to indicate
> whether you want it to create new columns or only match existing columns. That
> would also give me a convenient excuse to skip all those NOTICEs about merging
> column definitions.
+1, but I also agree with Tom that this doesn't need to be in the first
pass.
> Actually I think in the long term for partitioned tables Postgres will have to
> implement a special syntax just like Oracle and other databases. The user
> doesn't really want to have to manually manage all the partitions as tables.
> That imposes a lot of extra work to have to define the tables with the right
> syntax, maintain the constraints properly, etc.
> 
> For the user it would be better to have a single property of the partitioned
> table that specified the partition key. Then when adding a partition you would
> only have to specify the key range it covers, not write an arbitrary
> constraint from scratch. Nor would you have to create an empty table with the
> proper definition first then add it in.

I think this is on the TODO list; it's just a matter of actually doing
it. A good first step would be creating an easy means to create an
inherited table that contained everything the parent did; constraints,
indexes, etc. After that's in place, it's easier to create a new
partition (constraints and all) with a single command.

Note that there's no reason this *has* to be in the backend; someone
could do it as a pgFoundry project. Of course long-term it would be best
if it was included, but that's probably more involved, especially for a
newer coder.
-- 
Jim C. Nasby, Sr. Engineering Consultant      jnasby@pervasive.com
Pervasive Software      http://pervasive.com    work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf       cell: 512-569-9461

Ühel kenal päeval, K, 2006-06-07 kell 15:33, kirjutas Greg Stark:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
> 
> > I thought we had agreed that the semantics of ADD INHERITS would be to
> > reject the command if the child wasn't already suitable to be a child
> > of the parent.  Not to modify it by adding columns or constraints or
> > whatever.  For the proposed uses of ADD INHERITS (in particular,
> > linking and unlinking partition tables) an incompatibility in schema
> > almost certainly means you made a mistake, and you don't really want
> > the system helpfully "fixing" your table to match the parent.
> 
> I didn't see any discussion like that and I find it pretty surprising.

I'm pretty sure that what was discussed was just attaching/detaching
child tables into inheritance chains with no table alterations.

Maybe it was never mentioned explicitly, but that was how I understood
the discussion.

> Personally I would have agreed. For partitioned tables you certainly don't
> want it to create new columns without warning you.

Exactly!

> But that's entirely inconsistent with the way inherited tables work in
> general. It seems to go against the grain of Postgres's general style to
> implement just the use case that's useful for a particular application rather
> than keep the features logically consistent with each other. 

There are too many conflicting definitions of "logically consistent", so
doing the bare minimum is the best way to avoid the whole problem.

> Perhaps there should be an option when issuing the ADD INHERITS to indicate
> whether you want it to create new columns or only match existing columns. That
> would also give me a convenient excuse to skip all those NOTICEs about merging
> column definitions.

nonono! the whole pg inheritance/partitioning thing is still quite
low-level and ADD/DEL INHERITS is the wrong place to start fixing it.

> Actually I think in the long term for partitioned tables Postgres will have to
> implement a special syntax just like Oracle and other databases. The user
> doesn't really want to have to manually manage all the partitions as tables.
> That imposes a lot of extra work to have to define the tables with the right
> syntax, maintain the constraints properly, etc.

Yes. Maybe. But this is something that requires much more thought and
planning than adding the simplest possible ADD/DELETE INHERITS.

> For the user it would be better to have a single property of the partitioned
> table that specified the partition key. Then when adding a partition you would
> only have to specify the key range it covers, not write an arbitrary
> constraint from scratch. Nor would you have to create an empty table with the
> proper definition first then add it in.

Don't try to solve too many problems at once. Starting with just a
possibility to move suitable ready-made partitions in and out of
inheritance chain solves a really big problem. No need to try to
obfuscate it with extra functionality, at least not initially.

-- 
----------------
Hannu Krosing
Database Architect
Skype Technologies OÜ
Akadeemia tee 21 F, Tallinn, 12618, Estonia

Skype me:  callto:hkrosing
Get Skype for free:  http://www.skype.com

How does 

ALTER TABLE table INHERITS ADD parent
ALTER TABLE table INHERITS DROP parent

sound?

I'll admit it doesn't read very well but it doesn't necessitate complicating
other rules in gram.y

-- 
greg

Greg Stark <gsstark@MIT.EDU> writes:

> How does 
> 
> ALTER TABLE table INHERITS ADD parent
> ALTER TABLE table INHERITS DROP parent
> 
> sound?
> 
> I'll admit it doesn't read very well but it doesn't necessitate complicating
> other rules in gram.y

Or alternatively if people want to keep English-like SQL style grammar:

ALTER TABLE table INHERIT parent
ALTER TABLE table NO INHERIT parent



-- 
greg

Greg Stark said:
> Greg Stark <gsstark@MIT.EDU> writes:
>
>> How does
>>
>> ALTER TABLE table INHERITS ADD parent
>> ALTER TABLE table INHERITS DROP parent
>>
>> sound?
>>
>> I'll admit it doesn't read very well but it doesn't necessitate
>> complicating other rules in gram.y
>
> Or alternatively if people want to keep English-like SQL style grammar:
>
> ALTER TABLE table INHERIT parent
> ALTER TABLE table NO INHERIT parent
>


That could work ... or maybe UNINHERIT would read better than NO INHERIT.

cheers

andrew

On Jun 8, 2006, at 9:13 , Greg Stark wrote:

> Greg Stark <gsstark@MIT.EDU> writes:
>
>> How does
>>
>> ALTER TABLE table INHERITS ADD parent
>> ALTER TABLE table INHERITS DROP parent
>>
>> sound?
>>
>> I'll admit it doesn't read very well but it doesn't necessitate  
>> complicating
>> other rules in gram.y
>
> Or alternatively if people want to keep English-like SQL style  
> grammar:
>
> ALTER TABLE table INHERIT parent
> ALTER TABLE table NO INHERIT parent

ALTER TABLE table DISOWN parent?

Michael Glaesemann
grzm seespotcode net

"Andrew Dunstan" <andrew@dunslane.net> writes:

> Greg Stark said:
>
> > Or alternatively if people want to keep English-like SQL style grammar:
> >
> > ALTER TABLE table INHERIT parent
> > ALTER TABLE table NO INHERIT parent
>
> That could work ... or maybe UNINHERIT would read better than NO INHERIT.

DISINHERIT maybe?

While creating unreserved keywords isn't the end of the world it seems better
to stick to the vocabulary already there if possible. It makes it easier for
the user to remember how to spell commands. That's why I didn't suggest fixing
the DROP INHERITS ambiguity by inventing something like REMOVE INHERITS.

-- 
greg

grzm,

> ALTER TABLE table DISOWN parent?

You can't disown your parents.   ;-)

-- 
--Josh

Josh Berkus
PostgreSQL @ Sun
San Francisco

Greg Stark <gsstark@MIT.EDU> writes:
> While creating unreserved keywords isn't the end of the world it seems better
> to stick to the vocabulary already there if possible. It makes it easier for
> the user to remember how to spell commands.

+1.  Don't invent new keywords (even if unreserved) when there's no
strong reason to do so.
        regards, tom lane

> > > But that's entirely inconsistent with the way inherited tables
work
> > > in general.
> >
> > I don't see any basis for that conclusion.  The properties of a
table
> > are set when it's created and you need to do pretty explicit ALTERs
to
> > change them.
>
> It just seems weird for:
>
> CREATE TABLE foo (x,y,z) INHERITS (bar)
>
> to not be the equivalent to:
>
> CREATE TABLE foo (x,y,z)
> ALTER TABLE foo ADD INHERITS bar

Imho the op should only choose that path if he wants to fill the table
before
adding the inheritance. It makes no sense to add columns with default
values
to existing rows of the child table, especially when you inherit the
defaults
from the parent.

So I agree with Tom, that ADD INHERITS should not add columns.

Andreas

"Zeugswetter Andreas DCP SD" <ZeugswetterA@spardat.at> writes:

> Imho the op should only choose that path if he wants to fill the table
> before adding the inheritance. It makes no sense to add columns with default
> values to existing rows of the child table, especially when you inherit the
> defaults from the parent.

We already have ALTER TABLE ADD COLUMN working for columns with defaults, so I
think that horse has left the barn. It was awfully annoying for users when
that feature was missing. Any non-linearities in the user interface like this
end up being surprises and annoyances for users.

In any case there's a separate problem with defaults. We want to guarantee
that you can DROP a partition and then re-ADD it and the result should be a
noop at least from the user's perspective. We can't do that unless I
compromise on my idea that adding a child after the fact should be equivalent
to creating it with the parent in the definition.

When creating a table with the parent in the definition CREATE TABLE will copy
the parent's default if the default in the child is NULL:

postgres=# create table b (i integer default null) inherits (a);
NOTICE:  merging column "i" with inherited definition
CREATE TABLE
postgres=# \d b      Table "public.b"Column |  Type   | Modifiers 
--------+---------+-----------i      | integer | default 2
Inherits: a


The problem is that it's possible to fiddle with the defaults after the table
is created, including dropping a default. If you drop the default and then
DROP-ADD the partition it would be a problem if the default magically
reappeared.

The only way to allow DROP then ADD to be a noop would be to accept whatever
the DEFAULT is on the child table without complaint. And I'm not just saying
that because it's the easiest for me to implement :)


This is already a factor for NOT NULL constraints too. When adding a parent
after the fact your NULLable column can magically become NOT NULL if the
parent is NOT NULL. But for adding a partition after the fact we can't just
change the column to NOT NULL because there may already be NULL rows in the
table.

We could do a pass-3 check for the NOT NULL constraint but if we're not doing
other schema changes then it makes more sense to just refuse to add such a
table.

-- 
greg

I can't find any standard api to remove a single specific dependency. It seems
normally dependencies are only removed when dropping objects via
performDeletion.

Should I just put a scan of pg_depend in ATExecDropInherits or should I add a
new function to pg_depend or somewhere else to handle deleting a specific
dependency?

The only way I can see to implement it is kind of gross. It would have to do
the same scan deleteDependencyRecordsFor does and add an explicit check on
each loop to see if it matches the referenced class/object.

-- 
greg

Ühel kenal päeval, N, 2006-06-08 kell 09:32, kirjutas Greg Stark:
> "Zeugswetter Andreas DCP SD" <ZeugswetterA@spardat.at> writes:
> 
> > Imho the op should only choose that path if he wants to fill the table
> > before adding the inheritance. It makes no sense to add columns with default
> > values to existing rows of the child table, especially when you inherit the
> > defaults from the parent.
> 
> We already have ALTER TABLE ADD COLUMN working for columns with defaults, so I
> think that horse has left the barn. 

Do you mean that in newer versions ALTER TABLE ADD COLUMN will change
existing data without asking me ?

That would be evil!

Even worse if ALTER TABLE ALTER COLUMN SET DEFAULT would do the same.

Soon we will be as bad as MS Word !

> It was awfully annoying for users when that feature was missing. 
> Any non-linearities in the user interface like this
> end up being surprises and annoyances for users.

I would be *really*, *really*, *really* annoyed if an op that I expected
to take less than 1 sec takes 5 hours and then forces me to spend
another 10 hours on VACUUM FULL+REINDEX or CLUSTER to get performance
back.

And making such radical changes even between major versions should be
stricty forbidden.

> In any case there's a separate problem with defaults. We want to guarantee
> that you can DROP a partition and then re-ADD it and the result should be a
> noop at least from the user's perspective.

If DROP partition keeps defaults, and ADD does not change them then DROP
+ADD is a NOOP.

> We can't do that unless I
> compromise on my idea that adding a child after the fact should be equivalent
> to creating it with the parent in the definition.
> 
> When creating a table with the parent in the definition CREATE TABLE will copy
> the parent's default if the default in the child is NULL:
> 
> postgres=# create table b (i integer default null) inherits (a);
> NOTICE:  merging column "i" with inherited definition
> CREATE TABLE
> postgres=# \d b
>        Table "public.b"
>  Column |  Type   | Modifiers 
> --------+---------+-----------
>  i      | integer | default 2
> Inherits: a
> 
> 
> The problem is that it's possible to fiddle with the defaults after the table
> is created, including dropping a default. If you drop the default and then
> DROP-ADD the partition it would be a problem if the default magically
> reappeared.

sure. it should not magically appear.

> The only way to allow DROP then ADD to be a noop would be to accept whatever
> the DEFAULT is on the child table without complaint. And I'm not just saying
> that because it's the easiest for me to implement :)

exactly. that would be the correct behaviour. even for NULL default.

> This is already a factor for NOT NULL constraints too. When adding a parent
> after the fact your NULLable column can magically become NOT NULL if the
> parent is NOT NULL. But for adding a partition after the fact we can't just
> change the column to NOT NULL because there may already be NULL rows in the
> table.

constraints should match, that is a child table should already have all
the constraints of parent, but may have more.

> We could do a pass-3 check for the NOT NULL constraint but if we're not doing
> other schema changes then it makes more sense to just refuse to add such a
> table.

nono. the ADD/DROP INHERITS should not do any data checking, just
comparison of metainfo. the partitions could be huge and having to check
data inside them would negate most of the usefullness for ADD/DROP
INHERITS.

-- 
----------------
Hannu Krosing
Database Architect
Skype Techshould benologies OÜ
Akadeemia tee 21 F, Tallinn, 12618, Estonia

Skype me:  callto:hkrosing
Get Skype for free:  http://www.skype.com

NOTICE: This communication contains privileged or other confidential
information. If you have received it in error, please advise the sender
by reply email and immediately delete the message and any attachments
without copying or disclosing the contents.

Greg Stark wrote:
> 
> I can't find any standard api to remove a single specific dependency. It seems
> normally dependencies are only removed when dropping objects via
> performDeletion.

Huh, and can't you just drop an inheritance entry with performDeletion?
Maybe what you should do is add support for that to doDeletion (and all
dependency stuff it seems ...)

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:

> Greg Stark wrote:
> > 
> > I can't find any standard api to remove a single specific dependency. It seems
> > normally dependencies are only removed when dropping objects via
> > performDeletion.
> 
> Huh, and can't you just drop an inheritance entry with performDeletion?
> Maybe what you should do is add support for that to doDeletion (and all
> dependency stuff it seems ...)

Well I'm not actually deleting anything. The dependency is between the two
tables and I don't want to delete either of the tables.

Perhaps what should really be happening here is that there should be
dependencies from the pg_inherit entry to the two tables rather than from one
table to the other.

Then a simple performDeletion on the pg_inherit entry would take care of the
dependencies.

I'm not sure how many other changes that would entail though.

-- 
greg

Hannu Krosing <hannu@skype.net> writes:

> Do you mean that in newer versions ALTER TABLE ADD COLUMN will change
> existing data without asking me ?
> 
> That would be evil!
> 
> Even worse if ALTER TABLE ALTER COLUMN SET DEFAULT would do the same.

postgres=# alter table test add b integer default 1;
ALTER TABLE
postgres=# select * from test;a | b 
---+---0 | 1
(1 row)

> > It was awfully annoying for users when that feature was missing. 
> > Any non-linearities in the user interface like this
> > end up being surprises and annoyances for users.
> 
> I would be *really*, *really*, *really* annoyed if an op that I expected
> to take less than 1 sec takes 5 hours and then forces me to spend
> another 10 hours on VACUUM FULL+REINDEX or CLUSTER to get performance
> back.

I forget whether the developer managed to get it working without doing any
table rewriting. In theory the table just needs to know that records that are
"missing" that column in the null bitmap should behave as if they have the
default value. But I seem to recall some headaches with that approach.

> > In any case there's a separate problem with defaults. We want to guarantee
> > that you can DROP a partition and then re-ADD it and the result should be a
> > noop at least from the user's perspective.
> 
> If DROP partition keeps defaults, and ADD does not change them then DROP
> +ADD is a NOOP.
>
> > We can't do that unless I compromise on my idea that adding a child after
> > the fact should be equivalent to creating it with the parent in the
> > definition.

It does make DROP+ADD a noop which is why I'm suggesting it. I'm just noting
that it makes a second reason why:

CREATE TABLE foo (a integer) INHERITS (bar);

and:

CREATE TABLE foo (a integer);
ALTER TABLE foo INHERIT bar;

are not equivalent. Since in the first case a will acquire any defaults on a
from bar whereas in the second case it will remain with a default of NULL.


> constraints should match, that is a child table should already have all
> the constraints of parent, but may have more.

Well even that is a problem. You can drop an inherited constraint from a
child. So this would mean you wouldn't be able to re-add that partition back.

Come to think of it it's pretty strange that you can drop an inherited
constraint from a child. And doing an experiment it seems you can also DROP
NOT NULL on a child which is also pretty strange.

I don't see how to block these operations though unless we either search
parent classes for constraints to check at run-time or add additional
dependency records to block dropping these things.



> > We could do a pass-3 check for the NOT NULL constraint but if we're not doing
> > other schema changes then it makes more sense to just refuse to add such a
> > table.
> 
> nono. the ADD/DROP INHERITS should not do any data checking, just
> comparison of metainfo. the partitions could be huge and having to check
> data inside them would negate most of the usefullness for ADD/DROP
> INHERITS.

I agree that it's important to be possible to add/drop partitions in constant
time. That's the whole advantage of partitioned tables. However it might be
reasonable to support *additional* operations that aren't necessary for
partitioned tables but make sense for other applications even if these
operations are more expensive.

But it seems the priority right now is clearly on partitioned tables and these
other operations are for another day.

-- 
greg

Greg Stark <gsstark@mit.edu> writes:
> Come to think of it it's pretty strange that you can drop an inherited
> constraint from a child. And doing an experiment it seems you can also DROP
> NOT NULL on a child which is also pretty strange.

Yeah.  I think we had agreed that this is a bug.  Note the TODO entries:
o Prevent parent tables from altering or dropping constraints  like CHECK that are inherited by child tables unless
CASCADE is usedo %Prevent child tables from altering or dropping constraints         like CHECK that were inherited
fromthe parent table
 

> I don't see how to block these operations though unless we either search
> parent classes for constraints to check at run-time or add additional
> dependency records to block dropping these things.

The implementation I had in mind was to add columns similar to attinhcount
and attislocal to pg_constraint.
        regards, tom lane

On Thu, Jun 08, 2006 at 11:42:49AM -0400, Greg Stark wrote:
> > > It was awfully annoying for users when that feature was missing. 
> > > Any non-linearities in the user interface like this
> > > end up being surprises and annoyances for users.
> > 
> > I would be *really*, *really*, *really* annoyed if an op that I expected
> > to take less than 1 sec takes 5 hours and then forces me to spend
> > another 10 hours on VACUUM FULL+REINDEX or CLUSTER to get performance
> > back.
> 
> I forget whether the developer managed to get it working without doing any
> table rewriting. In theory the table just needs to know that records that are
> "missing" that column in the null bitmap should behave as if they have the
> default value. But I seem to recall some headaches with that approach.

What happens if you

ALTER TABLE ... ADD new_column int DEFAULT 1;
ALTER TABLE ... ALTER new_column SET DEFAULT 2;
-- 
Jim C. Nasby, Sr. Engineering Consultant      jnasby@pervasive.com
Pervasive Software      http://pervasive.com    work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf       cell: 512-569-9461

"Jim C. Nasby" <jnasby@pervasive.com> writes:

> On Thu, Jun 08, 2006 at 11:42:49AM -0400, Greg Stark wrote:
> > But I seem to recall some headaches with that approach.
> 
> What happens if you
> 
> ALTER TABLE ... ADD new_column int DEFAULT 1;
> ALTER TABLE ... ALTER new_column SET DEFAULT 2;

Ah yes. Keeping track of multiple old defaults and when they were in effect
would indeed be quite a headache.

-- 
greg

On Thu, Jun 08, 2006 at 12:19:49PM -0400, Greg Stark wrote:
> 
> "Jim C. Nasby" <jnasby@pervasive.com> writes:
> 
> > On Thu, Jun 08, 2006 at 11:42:49AM -0400, Greg Stark wrote:
> > > But I seem to recall some headaches with that approach.
> > 
> > What happens if you
> > 
> > ALTER TABLE ... ADD new_column int DEFAULT 1;
> > ALTER TABLE ... ALTER new_column SET DEFAULT 2;
> 
> Ah yes. Keeping track of multiple old defaults and when they were in effect
> would indeed be quite a headache.

Probably. One possibility would be to track the table definition on an
XID basis and compare that info to the XMIN of a given row; that would
allow you to know exactly what the state of the table columns was. But
there's still a lot of pitfalls with that, such as VACUUM FREEZE. Since
ALTER TABLE on a very large table can be such a nightmare maybe some day
this will happen, but I'm not holding my breath.
-- 
Jim C. Nasby, Sr. Engineering Consultant      jnasby@pervasive.com
Pervasive Software      http://pervasive.com    work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf       cell: 512-569-9461

Greg Stark wrote:

> Well I'm not actually deleting anything. The dependency is between the two
> tables and I don't want to delete either of the tables.
> 
> Perhaps what should really be happening here is that there should be
> dependencies from the pg_inherit entry to the two tables rather than from one
> table to the other.
> 
> Then a simple performDeletion on the pg_inherit entry would take care of the
> dependencies.

Sounds like a reasonable thing to do ...  If you drop the parent table,
does that cascade to the child table as well?  Maybe what should happen
is that the child table is "disinherited".

I note that our documentation
http://www.postgresql.org/docs/8.1/static/sql-droptable.html
does not specify what happens.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Greg Stark <gsstark@mit.edu> writes:
> > Come to think of it it's pretty strange that you can drop an inherited
> > constraint from a child. And doing an experiment it seems you can also DROP
> > NOT NULL on a child which is also pretty strange.
> 
> Yeah.  I think we had agreed that this is a bug.  Note the TODO entries:

Ok, so it's definitely correct for me to require that new children have NOT
NULL if their parent has NOT NULL.

> > I don't see how to block these operations though unless we either search
> > parent classes for constraints to check at run-time or add additional
> > dependency records to block dropping these things.
> 
> The implementation I had in mind was to add columns similar to attinhcount
> and attislocal to pg_constraint.

Hm that would be simpler. That still leaves NOT NULL as a bit of a headache.


-- 
greg

Greg Stark <gsstark@mit.edu> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> The implementation I had in mind was to add columns similar to attinhcount
>> and attislocal to pg_constraint.

> Hm that would be simpler. That still leaves NOT NULL as a bit of a headache.

Yeah, I think we would want to start storing NOT NULL constraints
explicitly in pg_constraint so that we could track them.  This would
allow fixing some other things too, like the fact that we fail to
remember names for NOT NULL constraints.  attnotnull might still be
useful as an optimization, or maybe it should just go away.
        regards, tom lane

Greg Stark <gsstark@mit.edu> writes:
> Perhaps what should really be happening here is that there should be
> dependencies from the pg_inherit entry to the two tables rather than from one
> table to the other.

This seems unlikely to still have the correct semantics (DROP on child
is OK, DROP on parent is not unless CASCADE, in which case child is
dropped too, etc etc).
        regards, tom lane

Alvaro Herrera <alvherre@commandprompt.com> writes:

> Greg Stark wrote:
> 
> > Well I'm not actually deleting anything. The dependency is between the two
> > tables and I don't want to delete either of the tables.
> > 
> > Perhaps what should really be happening here is that there should be
> > dependencies from the pg_inherit entry to the two tables rather than from one
> > table to the other.
> > 
> > Then a simple performDeletion on the pg_inherit entry would take care of the
> > dependencies.
> 
> Sounds like a reasonable thing to do ...  If you drop the parent table,
> does that cascade to the child table as well?  Maybe what should happen
> is that the child table is "disinherited".

I think what should happen is:

. If you drop a child the pg_inherit line (and dependencies) silently disappears but the parent stays.
. If you drop a parent you get an error unless you use cascade in which case the pg_inherits line and the child all go
away.
. If you disown the child the pg_inherit line (and dependencies) is deleted

At least that's what partitioned table users would want. In that case the
partitions are creatures of the main table with no identity of their own. But
perhaps that's not the case for other users of inherited tables?

I'm a bit confused about what pg_depends entries would be necessary then. If
there's something like this there:

Child Table <--(AUTO)-- pg_inherit entry --(NORMAL)-> Parent Table

Then deleting the child table will correctly delete the pg_inherits line, but
deleting the parent with CASCADE will stop at the pg_inherits line without
deleting the child.

Whereas something like this:

Child Table <---(AUTO)---  pg_inherit entry --(NORMAL)-> Parent Table            --(NORMAL)-->

Would make the cascade go through but mean that I can't drop the pg_inherit
line with performDeletion() without having the child table disappear.

-- 
greg

Greg Stark <gsstark@mit.edu> writes:
> I'm a bit confused about what pg_depends entries would be necessary then. If
> there's something like this there:

> Child Table <--(AUTO)-- pg_inherit entry --(NORMAL)-> Parent Table

I think that would work, but it seems pretty baroque.  pg_inherit
entries are not separately accessible SQL objects; not in the sense
that, say, a table's rowtype is.  I think it'd be just about as easy
to leave the catalog definitions as-is and just manually drop the
child-to-parent pg_depend entry.  This would certainly be less code than
all the infrastructure needed to add pg_inherit entries as a separate
kind of dependency object.

I also note that to go in this direction, pg_inherits would need to add
an OID column, and an index on it.

BTW ... are you intending to renumber inhseqno entries of remaining
pg_inherits items after DROP INHERITS?  Which seqno will be assigned
by ADD INHERITS?  This seems like another area in which DROP/ADD will
not be a complete no-op.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> BTW ... are you intending to renumber inhseqno entries of remaining
> pg_inherits items after DROP INHERITS?  Which seqno will be assigned
> by ADD INHERITS?  This seems like another area in which DROP/ADD will
> not be a complete no-op.

I assigned inhseqno to be max(inhseqno)+1. I was already scanning the parents
to check for duplicate parents so I just accumulated a maximum seqno at the
same time.

It's not a precise noop in database internal data structures, but I don't see
any user-visible effects switching around seqnos would have. But maybe there's
something I don't know about?

The actual order only seems to be significant in that it affects the ordering
of inherited columns. But that's already thrown to the wind as soon as you
allow adding new parents anyways. I'm just matching by name regardless of
position. And in any case that is only going to match the original ordering of
the original sequno ordering.

I did wonder whether it was kosher to leave holes.

-- 
greg

Greg Stark <gsstark@mit.edu> writes:
> It's not a precise noop in database internal data structures, but I don't see
> any user-visible effects switching around seqnos would have. But maybe there's
> something I don't know about?

It'll affect the order in which pg_dump lists the parents, which will
affect the order in which the columns are created on dump and reload.
(Or at least it ought to ... right offhand I don't see anything in the
pg_dump source code that ensures the original order is preserved.  This
may be a pg_dump bug.)

> I did wonder whether it was kosher to leave holes.

Not sure.  I don't offhand see anything that requires the numbers to be
consecutive.

If you don't compact out the holes during DROP, then ADD could use the
rule of "first unused number" instead of max+1.  This would ensure
DROP/ADD is a no-op for simple cases in which you only unlink from one
parent.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Greg Stark <gsstark@mit.edu> writes:
> > It's not a precise noop in database internal data structures, but I don't see
> > any user-visible effects switching around seqnos would have. But maybe there's
> > something I don't know about?
> 
> It'll affect the order in which pg_dump lists the parents, which will
> affect the order in which the columns are created on dump and reload.
> (Or at least it ought to ... right offhand I don't see anything in the
> pg_dump source code that ensures the original order is preserved.  This
> may be a pg_dump bug.)

Hm, if column order is important for table with multiple parents then you have
other problems already. The attislocal->1 mutation will cause any
singly-inherited columns to go to the head of the list. If you dropped any
table but the first parent then it isn't going to matter if it's in the right
place in the inheritance list or not.

If you really want to preserve column order then it might be necessary to
invent some syntax that indicates a column should be created with
attislocal=f. Then pg_dump can dump a complete list of columns including
inherited columns and CREATE TABLE can use that order merging in inherited
definitions without changing the order.

But it would be a nonstandard extension :(


-- 
greg

Greg Stark <gsstark@mit.edu> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> It'll affect the order in which pg_dump lists the parents, which will
>> affect the order in which the columns are created on dump and reload.

> Hm, if column order is important for table with multiple parents then you have
> other problems already. The attislocal->1 mutation will cause any
> singly-inherited columns to go to the head of the list.

So?  They'll get re-merged with the parent column during CREATE TABLE
anyway.
        regards, tom lane

Greg Stark wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>
>   
>> Greg Stark <gsstark@mit.edu> writes:
>>     
>>> It's not a precise noop in database internal data structures, but I don't see
>>> any user-visible effects switching around seqnos would have. But maybe there's
>>> something I don't know about?
>>>       
>> It'll affect the order in which pg_dump lists the parents, which will
>> affect the order in which the columns are created on dump and reload.
>> (Or at least it ought to ... right offhand I don't see anything in the
>> pg_dump source code that ensures the original order is preserved.  This
>> may be a pg_dump bug.)
>>     
>
> Hm, if column order is important for table with multiple parents then you have
> other problems already. The attislocal->1 mutation will cause any
> singly-inherited columns to go to the head of the list. If you dropped any
> table but the first parent then it isn't going to matter if it's in the right
> place in the inheritance list or not.
>
> If you really want to preserve column order then it might be necessary to
> invent some syntax that indicates a column should be created with
> attislocal=f. Then pg_dump can dump a complete list of columns including
> inherited columns and CREATE TABLE can use that order merging in inherited
> definitions without changing the order.
>
> But it would be a nonstandard extension :(
>
>
>   

hmm, I take it we will just select by name in some canonical order 
(presumably the parent's order)?

ISTR discussion at one time of implementing logical vs. physical 
ordering ... would that have any relevance here?

cheers

andrew

Ühel kenal päeval, N, 2006-06-08 kell 11:42, kirjutas Greg Stark:
> Hannu Krosing <hannu@skype.net> writes:
> 
> > Do you mean that in newer versions ALTER TABLE ADD COLUMN will change
> > existing data without asking me ?
> > 
> > That would be evil!
> > 
> > Even worse if ALTER TABLE ALTER COLUMN SET DEFAULT would do the same.
> 
> postgres=# alter table test add b integer default 1;
> ALTER TABLE
> postgres=# select * from test;
>  a | b 
> ---+---
>  0 | 1
> (1 row)
> 
> > > It was awfully annoying for users when that feature was missing. 
> > > Any non-linearities in the user interface like this
> > > end up being surprises and annoyances for users.
> > 
> > I would be *really*, *really*, *really* annoyed if an op that I expected
> > to take less than 1 sec takes 5 hours and then forces me to spend
> > another 10 hours on VACUUM FULL+REINDEX or CLUSTER to get performance
> > back.
> 
> I forget whether the developer managed to get it working without doing any
> table rewriting. In theory the table just needs to know that records that are
> "missing" that column in the null bitmap should behave as if they have the
> default value. But I seem to recall some headaches with that approach.

I remember that discussion, but I'm surprised that something got
implemented and accepted into core with so many unsolvable
problems/logical inconsistencies/new pitfalls.

for example - to be consistent, one should also make "ALTER TABLE ALTER
COLUMN col SET DEFAULT x" change each "default" value, no ? but how
should one know it for records which are updated, possibly in columns
newer than the one with changed DEFAULT. Or was a new default bitmap
introduced in addition to null bitmap ?

-- 
----------------
Hannu Krosing
Database Architect
Skype Technologies OÜ
Akadeemia tee 21 F, Tallinn, 12618, Estonia

Skype me:  callto:hkrosing
Get Skype for free:  http://www.skype.com

Hannu Krosing <hannu@skype.net> writes:
> I remember that discussion, but I'm surprised that something got
> implemented and accepted into core with so many unsolvable
> problems/logical inconsistencies/new pitfalls.

The current behavior of ALTER ADD COLUMN & SET DEFAULT is per SQL spec.
If you feel it's inconsistent, take it up with the standards committee.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Greg Stark <gsstark@mit.edu> writes:
> > Tom Lane <tgl@sss.pgh.pa.us> writes:
> >> It'll affect the order in which pg_dump lists the parents, which will
> >> affect the order in which the columns are created on dump and reload.
> 
> > Hm, if column order is important for table with multiple parents then you have
> > other problems already. The attislocal->1 mutation will cause any
> > singly-inherited columns to go to the head of the list.
> 
> So?  They'll get re-merged with the parent column during CREATE TABLE
> anyway.

But merged columns that are defined locally still appear in the position they
were defined locally. Not with the other inherited columns.

It's not going to matter to partitioned table users who are dropping the only
parent since that will just make *all* the columns into local columns. And
it's not going to matter to someone who drops all parents and then replaces
them in the same order.

But it will matter to the same people to whom the reordered inhseqno matters.
If you drop a parent and then readd it then that parent will both go to the
end of the list of parents which make any of multiple-inherited columns from
that parent go to the end of the list as well as mark any singly-inherited
columns from that parent as local which push them to the start of the list.

Note that if you don't re-add the parents you'll be left with a column order
that intermixes inherited and locally defined columns which *can't* be created
in postgres no matter what sequence of commands pg_dump dumps.

Basically I think if you're doing multiple inheritance and start using
add/drop inherits your column order is going to turn into chop suey quickly. I
think the only way to fix that would be to basically erase the whole
local/inherited distinction and let pg_dump specify the precise order of all
the columns.

-- 
greg

Hannu Krosing <hannu@skype.net> writes:

> for example - to be consistent, one should also make "ALTER TABLE ALTER
> COLUMN col SET DEFAULT x" change each "default" value, no ? 

er, I think that is in fact a no.

-- 
greg

Greg Stark <gsstark@mit.edu> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> So?  They'll get re-merged with the parent column during CREATE TABLE
>> anyway.

> But merged columns that are defined locally still appear in the position they
> were defined locally. Not with the other inherited columns.

Really?

regression=# create table p (p1 int, p2 int, p3 int);
CREATE TABLE
regression=# create table c (c1 int, c2 int) inherits (p);
CREATE TABLE
regression=# create table gc (gc1 int, p2 int, c1 int, gc2 int) inherits (c);
NOTICE:  merging column "p2" with inherited definition
NOTICE:  merging column "c1" with inherited definition
CREATE TABLE
regression=# \d gc     Table "public.gc"Column |  Type   | Modifiers
--------+---------+-----------p1     | integer |p2     | integer |p3     | integer |c1     | integer |c2     | integer
|gc1   | integer |gc2    | integer |
 
Inherits: c

regression=#

> Basically I think if you're doing multiple inheritance and start using
> add/drop inherits your column order is going to turn into chop suey quickly.

Very possibly, but that doesn't mean that we shouldn't take any concern
for avoiding unnecessary changes.
        regards, tom lane

On Thu, Jun 08, 2006 at 04:44:10PM -0400, Greg Stark wrote:
> Hannu Krosing <hannu@skype.net> writes:
> 
> > for example - to be consistent, one should also make "ALTER TABLE ALTER
> > COLUMN col SET DEFAULT x" change each "default" value, no ? 
> 
> er, I think that is in fact a no.

Yeah... once a default value is stored, there's no way to tell it was
stored because of the default clause; nor should there be.
-- 
Jim C. Nasby, Sr. Engineering Consultant      jnasby@pervasive.com
Pervasive Software      http://pervasive.com    work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf       cell: 512-569-9461

On Thu, 2006-06-08 at 16:47 -0400, Tom Lane wrote:
> Greg Stark <gsstark@mit.edu> writes:
> > Tom Lane <tgl@sss.pgh.pa.us> writes:
> >> So?  They'll get re-merged with the parent column during CREATE TABLE
> >> anyway.
> 
> > But merged columns that are defined locally still appear in the position they
> > were defined locally. Not with the other inherited columns.

Based on the test case Tom shows, I think we need to enforce that ADD
INHERITS will barf if the columns are not in exactly the order they
would have been in if we add done a CREATE ... INHERITS followed by a
DROP INHERITS. That wouldn't be a problem if we just say to people, if
you want to create a new partition do:

CREATE TABLE new_child ... LIKE child;

then later 

ALTER TABLE new_partition ADD INHERITS parent;

> > Basically I think if you're doing multiple inheritance and start using
> > add/drop inherits your column order is going to turn into chop suey quickly.

The column ordering is too important for other purposes. Things like
COPY, INSERT etc all depend upon specific column orderings.

If ADD INHERITS lets a wierd ordering go past that cannot ever be
re-created then everything will start to break.

--  Simon Riggs EnterpriseDB          http://www.enterprisedb.com

Simon Riggs <simon@2ndquadrant.com> writes:
> Based on the test case Tom shows, I think we need to enforce that ADD
> INHERITS will barf if the columns are not in exactly the order they
> would have been in if we add done a CREATE ... INHERITS followed by a
> DROP INHERITS.

This seems overly strong; if we enforced that policy consistently, then
it would for example be illegal to ADD COLUMN to a parent.  Consider
create table p(f1 int);create table c(f2 int) inherits (p);alter table p add column f3 int;

The column order in c will now be f1,f2,f3.  However, after a dump and
reload it'll be f1,f3,f2, because f3 will already be an inherited column
when c is created.  This is pretty much unavoidable and we've taken care
of the various loose ends needed to make it work safely.

What I'm saying is just that we should avoid *unnecessary* changes of
column order, and in particular that means taking at least a little care
to try to select a reasonable inhseqno during ADD INHERITS.

If you think the "first unused" policy wouldn't take care of enough
cases, one idea is to try to look at the columns that will be inherited
from the new parent, and to see if we can deduce a suitable inhseqno
based on those columns' positions.  I suspect this will be a pretty ugly
heuristic though ...
        regards, tom lane

Simon Riggs wrote:
> Based on the test case Tom shows, I think we need to enforce that ADD
> INHERITS will barf if the columns are not in exactly the order they
> would have been in if we add done a CREATE ... INHERITS followed by a
> DROP INHERITS. That wouldn't be a problem if we just say to people, if
> you want to create a new partition do:
>
> CREATE TABLE new_child ... LIKE child;
>
> then later 
>
> ALTER TABLE new_partition ADD INHERITS parent;
>
>   

This seems like a very reasonable restriction. I imagine in the most 
common case at least they will be exact clones.

cheers

andrew

Simon Riggs <simon@2ndquadrant.com> writes:

> On Thu, 2006-06-08 at 16:47 -0400, Tom Lane wrote:
> > Greg Stark <gsstark@mit.edu> writes:
> > > Tom Lane <tgl@sss.pgh.pa.us> writes:
> > >> So?  They'll get re-merged with the parent column during CREATE TABLE
> > >> anyway.
> > 
> > > But merged columns that are defined locally still appear in the position they
> > > were defined locally. Not with the other inherited columns.
> 
> Based on the test case Tom shows, I think we need to enforce that ADD
> INHERITS will barf if the columns are not in exactly the order they
> would have been in if we add done a CREATE ... INHERITS followed by a
> DROP INHERITS. 

Well firstly I think that rule is much too hard to explain to users. You would
have to simplify it into something that makes more sense from a user's point
of view.

But there's a bigger problem, it won't actually help. To maintain that
invariant you would never be allowed to DROP a parent unless you had no
locally defined columns at all. And if you had multiple parents you would have
further restrictions no multiply defined columns and you can only drop parents
in the reverse order they were listed on the inherits line.

So basically that rule translates into "you can only add a parent with
precisely the same definition as your child table and you can only drop a
parent if it's the last parent in the list and none of the columns are shared
with other parents". Is that what you want?

-- 
greg

> I forget whether the developer managed to get it working without doing any
> table rewriting. In theory the table just needs to know that records that are
> "missing" that column in the null bitmap should behave as if they have the
> default value. But I seem to recall some headaches with that approach.

The problem is if you then change the default.

On Thu, 2006-06-08 at 17:23 -0400, Greg Stark wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
> 
> > On Thu, 2006-06-08 at 16:47 -0400, Tom Lane wrote:
> > > Greg Stark <gsstark@mit.edu> writes:
> > > > Tom Lane <tgl@sss.pgh.pa.us> writes:
> > > >> So?  They'll get re-merged with the parent column during CREATE TABLE
> > > >> anyway.
> > > 
> > > > But merged columns that are defined locally still appear in the position they
> > > > were defined locally. Not with the other inherited columns.
> > 
> > Based on the test case Tom shows, I think we need to enforce that ADD
> > INHERITS will barf if the columns are not in exactly the order they
> > would have been in if we add done a CREATE ... INHERITS followed by a
> > DROP INHERITS. 
> 
> Well firstly I think that rule is much too hard to explain to users. You would
> have to simplify it into something that makes more sense from a user's point
> of view.
> 
> But there's a bigger problem, it won't actually help. To maintain that
> invariant you would never be allowed to DROP a parent unless you had no
> locally defined columns at all. And if you had multiple parents you would have
> further restrictions no multiply defined columns and you can only drop parents
> in the reverse order they were listed on the inherits line.
> 
> So basically that rule translates into "you can only add a parent with
> precisely the same definition as your child table and you can only drop a
> parent if it's the last parent in the list and none of the columns are shared
> with other parents". Is that what you want?

Well, what I want and what we can have are frequently separate things.

IMHO the goal here is to be able to decouple/recouple partitions; maybe
others see it differently. If there are restrictions in order to make
that possible, so be it. IMHO the main users of this feature will be
people using single inheritance with all partitions the same and that
situation seems largely unaffected by your unfortunate discoveries.

It seems straightforward to give an example in the manual of the
recommended way to create a table that is destined to become a partition
in the future - whatever that is.

I'd rather have new feature with restrictions, than no new feature.
Somebody for whom the restrictions are a problem may later solve them.

--  Simon Riggs              EnterpriseDB   http://www.enterprisedb.com