Thread: Inheritance, CREATE TABLE LIKE, and partitioned tables
Currently analyze.c and tablecmds.c both have some very similar code to handle copying columns from "parent" tables. As long as they were just copying columns and in the case of tablecmds.c copying check constraints that wasn't really that bad an idea, the code is pretty simple. However with partitioned tables it becomes important to copy more table attributes like foreign key constraints and hopefully one day indexes. And it would be awfully convenient to have CREATE TABLE LIKE have options to copy the same things that inherited tables get. And have exactly the same semantics. So I'm suggesting refactoring the code from analyze.c and tablecmds.c into functions to copy the columns, constraints, indexes etc. For example I see a functions like: List *CopyTableColumns(relation source, List *target_schema) List *CopyTableCheckConstraints(relation source, List *target_schema) ... To do this though might require some changes in the algorithm used for inherited tables. Currently it builds up the list of merged columns incrementally. I'm thinking it would be more natural to accumulate all the columns from parents and then remove duplicates in a single pass. I think it should be possible to maintain precisely the same semantics doing this though. I may be able to make AddInherits a consumer for these functions as well, though it would be a bit awkward since it would have to construct a fake list of ColumnDefs to act as the target schema. It would have the side effect of making the constraint comparison use change_varattnos_of_a_node and then compare the binary representations rather than decompiling the constraints to do the comparison. I'm not sure if that's the same semantics. To a certain degree I feel like this is just make-work. The existing code works fine and I can just happily keep additing functionality to both analyze.c and tablecmds.c. And it's possible we won't always want to have the two match. Has anyone looked at applying the ADD INHERITS patch yet? Would it be more or less likely to be accepted if it were a bigger patch that refactored all this stuff like I'm talking about? -- greg
On Mon, Jun 26, 2006 at 12:31:24PM -0400, Greg Stark wrote: > > Currently analyze.c and tablecmds.c both have some very similar code to handle > copying columns from "parent" tables. As long as they were just copying > columns and in the case of tablecmds.c copying check constraints that wasn't > really that bad an idea, the code is pretty simple. > > However with partitioned tables it becomes important to copy more table > attributes like foreign key constraints and hopefully one day indexes. And it > would be awfully convenient to have CREATE TABLE LIKE have options to copy the > same things that inherited tables get. And have exactly the same semantics. People will turn around and immediately as for create table like without us making the assumptions it wanted all of the extras that come with inheritance. COPY is a copy of the table, not additional functionality. The added flexibility of adding only what is necessary makes more sense than going in to guess what was added and removing it later if it is not needed. This does not preclude adding a copy table like (with extras) though if you must. > > So I'm suggesting refactoring the code from analyze.c and tablecmds.c into > functions to copy the columns, constraints, indexes etc. > > For example I see a functions like: > > List *CopyTableColumns(relation source, List *target_schema) > List *CopyTableCheckConstraints(relation source, List *target_schema) > ... > > To do this though might require some changes in the algorithm used for > inherited tables. Currently it builds up the list of merged columns > incrementally. I'm thinking it would be more natural to accumulate all the > columns from parents and then remove duplicates in a single pass. I think it > should be possible to maintain precisely the same semantics doing this though. Be careful because the code may be taking into account multiple inheritance here. I'm not sure about this. It should be looked at carefully. > > I may be able to make AddInherits a consumer for these functions as well, > though it would be a bit awkward since it would have to construct a fake list > of ColumnDefs to act as the target schema. It would have the side effect of > making the constraint comparison use change_varattnos_of_a_node and then > compare the binary representations rather than decompiling the constraints to > do the comparison. I'm not sure if that's the same semantics. Again, be sure it is the same semantics before going that way. > > To a certain degree I feel like this is just make-work. The existing code > works fine and I can just happily keep additing functionality to both > analyze.c and tablecmds.c. And it's possible we won't always want to have the > two match. > > Has anyone looked at applying the ADD INHERITS patch yet? Would it be more or > less likely to be accepted if it were a bigger patch that refactored all this > stuff like I'm talking about? > > -- > greg > ---elein elein@varlena.com
elein <elein@varlena.com> writes: > People will turn around and immediately as for create table like without us > making the assumptions it wanted all of the extras that come with inheritance. > COPY is a copy of the table, not additional functionality. The added flexibility > of adding only what is necessary makes more sense than going in to guess > what was added and removing it later if it is not needed. > > This does not preclude adding a copy table like (with extras) though if you must. I'm not too sure what you're saying here. But just to be clear, the spec specifies what CREATE TABLE LIKE does and does not copy. Any behaviour different from the spec would have to be explicitly requested with an optional clause. -- greg
If you want to merge those functions, please do it as a separate patch now that the patch has been applied. Having too much unrelated stuff in a patch does confuse things. --------------------------------------------------------------------------- Greg Stark wrote: > > Currently analyze.c and tablecmds.c both have some very similar code to handle > copying columns from "parent" tables. As long as they were just copying > columns and in the case of tablecmds.c copying check constraints that wasn't > really that bad an idea, the code is pretty simple. > > However with partitioned tables it becomes important to copy more table > attributes like foreign key constraints and hopefully one day indexes. And it > would be awfully convenient to have CREATE TABLE LIKE have options to copy the > same things that inherited tables get. And have exactly the same semantics. > > So I'm suggesting refactoring the code from analyze.c and tablecmds.c into > functions to copy the columns, constraints, indexes etc. > > For example I see a functions like: > > List *CopyTableColumns(relation source, List *target_schema) > List *CopyTableCheckConstraints(relation source, List *target_schema) > ... > > To do this though might require some changes in the algorithm used for > inherited tables. Currently it builds up the list of merged columns > incrementally. I'm thinking it would be more natural to accumulate all the > columns from parents and then remove duplicates in a single pass. I think it > should be possible to maintain precisely the same semantics doing this though. > > I may be able to make AddInherits a consumer for these functions as well, > though it would be a bit awkward since it would have to construct a fake list > of ColumnDefs to act as the target schema. It would have the side effect of > making the constraint comparison use change_varattnos_of_a_node and then > compare the binary representations rather than decompiling the constraints to > do the comparison. I'm not sure if that's the same semantics. > > To a certain degree I feel like this is just make-work. The existing code > works fine and I can just happily keep additing functionality to both > analyze.c and tablecmds.c. And it's possible we won't always want to have the > two match. > > Has anyone looked at applying the ADD INHERITS patch yet? Would it be more or > less likely to be accepted if it were a bigger patch that refactored all this > stuff like I'm talking about? > > -- > greg > > > ---------------------------(end of broadcast)--------------------------- > TIP 5: don't forget to increase your free space map settings > -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +