Re: 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 inherit from 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 should check 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("merging column \"%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 see if 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 explicitly name 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 of relation \"%s\"",
parent->relname, RelationGetRelationName(rel))));
}

/* Search through columns looking for 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--it can 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, &copyTuple->t_self, copyTuple);
/* XXX "Avoid using it for multiple tuples, since opening the
* indexes and building the index info structures is moderately
* expensive." Perhaps this 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(at)mit(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)mit(dot)edu> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Greg Stark <gsstark(at)mit(dot)edu> writes:
> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)mit(dot)edu> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)pervasive(dot)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(at)sss(dot)pgh(dot)pa(dot)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

Greg Stark said:
> Greg Stark <gsstark(at)MIT(dot)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

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(at)MIT(dot)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

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

> Greg Stark <gsstark(at)MIT(dot)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(at)dunslane(dot)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(at)MIT(dot)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

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

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(at)commandprompt(dot)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

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

Greg Stark <gsstark(at)mit(dot)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(at)commandprompt(dot)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(at)mit(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)mit(dot)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(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Greg Stark <gsstark(at)mit(dot)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(at)mit(dot)edu> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> writes:
>
>
>> Greg Stark <gsstark(at)mit(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Greg Stark <gsstark(at)mit(dot)edu> writes:
> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Greg Stark <gsstark(at)mit(dot)edu> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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, 2006-06-08 at 16:47 -0400, Tom Lane wrote:
> Greg Stark <gsstark(at)mit(dot)edu> writes:
> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)com> writes:

> On Thu, 2006-06-08 at 16:47 -0400, Tom Lane wrote:
> > Greg Stark <gsstark(at)mit(dot)edu> writes:
> > > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Thu, 2006-06-08 at 17:23 -0400, Greg Stark wrote:
> Simon Riggs <simon(at)2ndquadrant(dot)com> writes:
>
> > On Thu, 2006-06-08 at 16:47 -0400, Tom Lane wrote:
> > > Greg Stark <gsstark(at)mit(dot)edu> writes:
> > > > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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