Hi All,

With regards to the proposed command.c refactoring...

I've done it by removing command.c and replacing it with

portal.c
alter.c
lock.c
namespace.c

Is that a good idea?  Will it break too many outstanding patches?

Basically the portal fetch/destroy commands go in portal.c, all the Alter*
commands with their static helper functions go in alter.c, the single
LockTable command goes in lock.c and the CreateSchema function goes in
namespace.c.  I anticipate that a few more functions will eventually be
created to go in namespace.c

I have also broken up the command.h header file into four separate
correspondingly named header files, and removed command.h itself.

The next step after this would be to move a lot of the redundant code in
alter.c into static functions.

Thoughts?

Chris


Wed, 3 Apr 2002 16:39:39 +0800
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au>
command.c breakup
On Wed, 2002-04-03 at 09:39, Christopher Kings-Lynne wrote:
> Hi All,
> 
> With regards to the proposed command.c refactoring...
> 
..about which I should apologise as I stuck my head above the parapet
and then sat on my ideas (mixing metaphors a bit).

> I've done it by removing command.c and replacing it with
> 
> portal.c
> alter.c
> lock.c
> namespace.c
> 
> Is that a good idea?  Will it break too many outstanding patches?

The feedback I had was not to worry too much about that! However, my
scheme doesn't take account of some of the more recent changes -I had
envisaged a more radical division by "object manipulated". Here's my
current working draft (doesn't include material from the last couple of
weeks):

command.c
---------

PortalCleanup        
PerformPortalFetch   
PerformPortalClose   
	Portal support functions move to portal.c

AlterTableAddColumn  
AlterTableAlterColumnDefault
drop_default
AlterTableAlterColumnFlags
			
	These move to table.c. They share common code for permissions
	and recursion. Therefore, propose to create a short helper
	routine (AlterTableAlterColumnSetup) which checks permissions,
	existence of relation (and acquirtes lock on rel?). Also
	provide macros for recursion, to be used in form:

	RECURSE_OVER_CHILDREN(relid);
	AlterTableDoSomething(args);
	RECURSE_OVER_CHILDREN_END;


find_attribute_walker 
find_attribute_in_node
RemoveColumnReferences
AlterTableDropColumn 

	These are part of the old DROP_COLUMN_HACK. Should they go in
	the transfer? (There seems to be agreement that DROP COLUMN
	will not be implemented as it is here).

AlterTableAddConstraint 
AlterTableDropConstraint
	
	Move to table.c These also use permissions and recursion code.

AlterTableOwner      
AlterTableCreateToastTable 
needs_toast_table
	All move to table.c. (Seems a bit more drastic than necessary
	to split AlterTableCreateToastTable and move
	needs_toast_table to access/heap/tuptoaster.c). 

LockTableCommand
	Move to lock.c


creatinh.c
----------

DefineRelation
RemoveRelation
TruncateRelation
MergeAttributes
change_varattnos_walker
change_varattnos_of_a_node
StoreCatalogInheritance
findAttrByName
setRelhassubclassInRelation
	
	All move to table.c


define.c
--------

case_translate_language_name
	
	Remove this one and refer to that in proclang.c

compute_return_type
compute_full_attributes
interpret_AS_clause
CreateFunction

	Move to function.c 

DefineOperator

	Move to operator.c

DefineAggregate

	Move to aggregate.c

DefineType

	Move to type.c

defGetString
defGetNumeric
defGetTypeLength
	
	Parameter fetching support, generic to all the processing for
	define statements. Inclined to move to type.c as used most by type
	creation.

remove.c
--------

RemoveOperator

	To operator.c

SingleOpOperatorRemove
AttributeAndRelationRemove

	To operator.c (or delete altogether -NOTYET since 94!)

RemoveType

	To type.c

RemoveFunction

	To function.c

RemoveAggregate

	To aggregate.c


rename.c
--------


renameatt
renamerel
ri_trigger_type
update_ri_trigger_args

	To table.c




Thus, the change in the set of files:

Removed:

command.c
creatinh.c
define.c
remove.c
rename.c

Added:
aggregate.c
function.c
operator.c
table.c
type.c

Sorry for going slow on this - but it seems that the organisation
has dropped out of my life in the last few weeks :) (and I've been away
over Easter). 

Regards

John


03 Apr 2002 10:10:22 +0100
John Gray <jgray@azuli.co.uk>
Re: command.c breakup
John Gray <jgray@azuli.co.uk> writes:
> Here's my current working draft (doesn't include material from the
> last couple of weeks):

Please note that there's been pretty substantial revisions in command.c
and creatinh.c over the past couple of weeks for schema support.  While
I think that those two files are largely done with, define.c and
remove.c are about to get the same treatment as the schema project moves
on to schema-tizing functions and operators.  So we'll need to coordinate
just when and how to make these structural revisions; and you'll
definitely need to be working against CVS tip.  What are your plans,
time-wise?  Does it make sense for the two of you to work together?

> 	These are part of the old DROP_COLUMN_HACK. Should they go in
> 	the transfer? (There seems to be agreement that DROP COLUMN
> 	will not be implemented as it is here).

I think Hiroshi finally removed all the DROP_COLUMN_HACK code yesterday.

> 	Parameter fetching support, generic to all the processing for
> 	define statements. Inclined to move to type.c as used most by type
> 	creation.

What about leaving define.c in existence, but have it hold only common
support routines for object-definition commands?  The param fetchers
would certainly fit in this category, and maybe some of the other
support routines you've described would fit here too.

> 	To operator.c (or delete altogether -NOTYET since 94!)

NOTYET probably means NEVER; whenever that functionality is implemented,
it'll be based on some sort of generic dependency code, not
special-purpose checks.  Feel free to remove this stuff too.

> Thus, the change in the set of files:

> Removed:

> command.c
> creatinh.c
> define.c
> remove.c
> rename.c

> Added:
> aggregate.c
> function.c
> operator.c
> table.c
> type.c

Minor gripe here: I would suggest taking a cue from indexcmds.c and
choosing file names along the lines of functioncmds.c, tablecmds.c,
etc.  The above names strike me as too generic and likely to cause
confusion with similarly-named files in other directories.

> Sorry for going slow on this - but it seems that the organisation
> has dropped out of my life in the last few weeks :) (and I've been away
> over Easter). 

Not a problem.  But we'll need a concentrated burst of work whenever
you are ready to prepare the final version of the patch; otherwise the
synchronization issues will cause problems/delays for other people.

			regards, tom lane

Wed, 03 Apr 2002 10:52:41 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: command.c breakup 
On Wed, 2002-04-03 at 16:52, Tom Lane wrote:
> John Gray <jgray@azuli.co.uk> writes:
> > Here's my current working draft (doesn't include material from the
> > last couple of weeks):
> 
> Please note that there's been pretty substantial revisions in command.c
> and creatinh.c over the past couple of weeks for schema support.  While
> I think that those two files are largely done with, define.c and
> remove.c are about to get the same treatment as the schema project moves
> on to schema-tizing functions and operators.  So we'll need to coordinate
> just when and how to make these structural revisions; and you'll
> definitely need to be working against CVS tip.  What are your plans,
> time-wise?  Does it make sense for the two of you to work together?
> 
I have compiled a new version against current CVS, now also including
references to dependencies (See below). I accept that we'll need to work
round the schema project -in the week since the last message I notice
that namespace support has arrived for function, aggregate and operator
creation. Is there more to come in these files?

I'm unsure whether it is sensible to split the commands/defrem.h file to
match the actual .c files (given that there are at present only two
externally referenced functions from each entity it seems reasonable to
keep them together -as they are all referred to from tcop/utility.c
anyway.

As far as joint working goes, if Chris K-L would like to grab all or
part of it he is very welcome :) My timescale is that I have time at
present to work on it, so maybe next week for incorporation (but do
people need more notice than that?)

Obviously, I haven't given more details of the common code elimination.
That is a slightly different kind of task -I'll post some specifics on
that in the next couple of days.

> > 	Parameter fetching support, generic to all the processing for
> > 	define statements. Inclined to move to type.c as used most by type
> > 	creation.
> 
> What about leaving define.c in existence, but have it hold only common
> support routines for object-definition commands?  The param fetchers
> would certainly fit in this category, and maybe some of the other
> support routines you've described would fit here too.
>
Yes, this seems sensible -but as far as the other support code goes, it
might make sense to have a file called (say) cmdsupport.c where the
parameter fetchers, the checking and recursion code etc. all goes? 

> > 	To operator.c (or delete altogether -NOTYET since 94!)
> 
> NOTYET probably means NEVER; whenever that functionality is implemented,
> it'll be based on some sort of generic dependency code, not
> special-purpose checks.  Feel free to remove this stuff too.
> 

OK

> > Thus, the change in the set of files:
> 
> 
> Minor gripe here: I would suggest taking a cue from indexcmds.c and
> choosing file names along the lines of functioncmds.c, tablecmds.c,
> etc.  The above names strike me as too generic and likely to cause
> confusion with similarly-named files in other directories.
> 
Yes, this makes sense and I've done that too.


> > Sorry for going slow on this - but it seems that the organisation
> > has dropped out of my life in the last few weeks :) (and I've been away
> > over Easter). 
> 
> Not a problem.  But we'll need a concentrated burst of work whenever
> you are ready to prepare the final version of the patch; otherwise the
> synchronization issues will cause problems/delays for other people.
> 

That shouldn't be too much of a problem in the next couple of weeks - if
we can decide on a specific day I'll book it into my diary (Any day but
Wednesday next week would be fine for me).

Regards

John



src/backend/commands/ directory reorganisation version 2 
(including dependencies), from CVS as of 12 noon, 2002-04-11)

Dependencies were determined from LXR cross-reference database. This
will show all *usage* -it won't catch cases where a header file is included
redundantly. Recursive grep seems to provide the same answers though.

command.c
---------

PortalCleanup        
PerformPortalFetch   
PerformPortalClose   
	Portal support functions move to portalcmds.c
	
	prototype commands/command.h -> commands/portal.h
	refer executor/spi.c tcop/pquery.c tcop/utility.c


AlterTableAddColumn  
AlterTableAlterColumnDropNotNull
AlterTableAlterColumnSetNotNull
AlterTableAlterColumnDefault
drop_default
AlterTableAlterColumnFlags
AlterTableDropColumn 			
AlterTableAddConstraint 
AlterTableDropConstraint
AlterTableOwner      
AlterTableCreateToastTable 
needs_toast_table

	These move to tablecmds.c. They share common code for permissions
	and recursion. Therefore, propose to create a short helper
	routine (AlterTableAlterColumnSetup) which checks permissions,
	existence of relation (and acquirtes lock on rel?). Also
	provide macros for recursion, to be used in form:

	RECURSE_OVER_CHILDREN(relid);
	AlterTableDoSomething(args);
	RECURSE_OVER_CHILDREN_END;

	prototype commands/command.h -> commands/tablecmds.h
	refer tcop/utility.c commands/cluster.c executor/execMain.c


LockTableCommand
	Move to lockcmds.c

	prototype commands/command.h -> commands/lockcmds.h
	refer tcop/utility.c

CreateSchemaCommand
	Move to schemacmds.c

	prototype commands/command.h -> commands/schemacmds.h
	refer tcop/utility.c

creatinh.c
----------

DefineRelation 
RemoveRelation 
TruncateRelation 
MergeDomainAttributes
MergeAttributes
change_varattnos_walker
change_varattnos_of_a_node
StoreCatalogInheritance
findAttrByName
setRelhassubclassInRelation
	
	All move to tablecmds.c

	prototye commands/creatinh.h -> commands/tablecmds.h
	refer commands/sequence.c commands/view.c tcop/utility.c

define.c
--------

case_translate_language_name
	
	Remove this one and refer to that in proclang.c. If this file
	becomes a file for support functions, then the reverse should apply.

compute_return_type
compute_full_attributes
interpret_AS_clause
CreateFunction

	Move to functioncmds.c 

	prototype commands/defrem.h -> ?
	refer tcop/utility.c

DefineOperator

	Move to operatorcmds.c

	prototype commands/defrem.h -> ?
	refer tcop/utility.c


DefineAggregate

	Move to aggregatecmds.c

	prototype commands/defrem.h -> ?
	refer tcop/utility.c


DefineDomain

	Move to domaincmds.c

	prototype commands/defrem.h -> ?
	refer tcop/utility.c


DefineType

	Move to typecmds.c

	prototype commands/defrem.h -> ?
	refer tcop/utility.c

findTypeIOFunction
defGetString
defGetNumeric
defGetQualifiedName
defGetTypeName
defGetTypeLength

	Keep in define.c as general support code. If other support code is
	coming here to, there might be a good case for a new file
	"cmdutils.c", say, to hold all sorts of generic code for permissions,
	recursion, etc.

remove.c
--------

RemoveOperator

	To operatorcmds.c

SingleOpOperatorRemove
AttributeAndRelationRemove

	Propose to delete altogether -NOTYET since 94, likely
	incompatible with current workings)

RemoveType

	To typecmds.c

RemoveDomain

	To domaincmds.c

RemoveFunction

	To functioncmds.c

RemoveAggregate

	To aggregatecmds.c

prototypes and dependencies for these identical to Define commands in define.c


rename.c
--------

renameatt
renamerel
ri_trigger_type
update_ri_trigger_args

	To tablecmds.c

	prototype commands/rename.h -> commands/tablecmds.h
	refer tcop/utility.c commands/cluster.c


Thus, the change in the set of files:

Removed:

command.c
creatinh.c
remove.c
rename.c

(and include files commands/command.h, commands/creatinh.h, commands/rename.h)


Added:
aggregatecmds.c
functioncmds.c
operatorcmds.c
portalcmds.c
tablecmds.c
typecmds.c
lockcmds.c
schemacmds.c

(and include files commands/portalcmds.h, commands/lockcmds.h, 
commands/tablecmds.h, commands/schemacmds.h)

Possibly "rename"[*] residual define.c to cmdsupport.c (and create new
header file commands/cmdsupport.h) which would also hold common 
permissions checkiing and inheritance code.



11 Apr 2002 13:08:02 +0100
John Gray <jgray@azuli.co.uk>
Re: command.c breakup
John Gray <jgray@azuli.co.uk> writes:
> I have compiled a new version against current CVS, now also including
> references to dependencies (See below). I accept that we'll need to work
> round the schema project -in the week since the last message I notice
> that namespace support has arrived for function, aggregate and operator
> creation. Is there more to come in these files?

I am hoping to commit the revisions for aggregates today.  Operators are
still to come, and after that it's the mop-up stuff like rules ...

> I'm unsure whether it is sensible to split the commands/defrem.h file to
> match the actual .c files (given that there are at present only two
> externally referenced functions from each entity it seems reasonable to
> keep them together -as they are all referred to from tcop/utility.c
> anyway.

Probably can leave well enough alone there; I don't see what it would
buy us to split up that header file.

>> What about leaving define.c in existence, but have it hold only common
>> support routines for object-definition commands?  The param fetchers
>> would certainly fit in this category, and maybe some of the other
>> support routines you've described would fit here too.
>> 
> Yes, this seems sensible -but as far as the other support code goes, it
> might make sense to have a file called (say) cmdsupport.c where the
> parameter fetchers, the checking and recursion code etc. all goes? 

If you prefer --- I haven't a strong feeling one way or the other.

> That shouldn't be too much of a problem in the next couple of weeks - if
> we can decide on a specific day I'll book it into my diary (Any day but
> Wednesday next week would be fine for me).

I will try to have no uncommitted changes over this weekend; that will
give you a clear field Monday morning, or you can start on the weekend
if you like.  Sound good?

			regards, tom lane

Thu, 11 Apr 2002 10:33:34 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: command.c breakup 
On Thu, 2002-04-11 at 15:33, Tom Lane wrote:
>
> > That shouldn't be too much of a problem in the next couple of weeks - if
> > we can decide on a specific day I'll book it into my diary (Any day but
> > Wednesday next week would be fine for me).
> 
> I will try to have no uncommitted changes over this weekend; that will
> give you a clear field Monday morning, or you can start on the weekend
> if you like.  Sound good?
> 

Fine. I'll work on that basis. I'll prepare a full-blown patch which can
be applied Monday -unless anyone else is sitting on uncommitted changes
to the directory that they want me to wait for?

Regards

John



12 Apr 2002 00:10:14 +0100
John Gray <jgray@azuli.co.uk>
Re: command.c breakup
> Fine. I'll work on that basis. I'll prepare a full-blown patch which can
> be applied Monday -unless anyone else is sitting on uncommitted changes
> to the directory that they want me to wait for?

Nothing important.  Shall I suggest that you do the rearrangement first, and
then once everything's happy, we can work on removing redundant code?

Chris


Fri, 12 Apr 2002 10:33:44 +0800
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au>
Re: command.c breakup
On Fri, 2002-04-12 at 03:33, Christopher Kings-Lynne wrote:
> > Fine. I'll work on that basis. I'll prepare a full-blown patch which can
> > be applied Monday -unless anyone else is sitting on uncommitted changes
> > to the directory that they want me to wait for?
> 
> Nothing important.  Shall I suggest that you do the rearrangement first, and
> then once everything's happy, we can work on removing redundant code?
> 

I think this is the right thing to do. Rearranging files shouldn't have
any effect on behaviour, but the removal of redundant code (e.g. for
permissions checks) may result in discussions about the appropriate
permissions for different activities -ISTM that this should be open to
normal discussion and review.

John



12 Apr 2002 09:48:34 +0100
John Gray <jgray@azuli.co.uk>
Re: command.c breakup
I'm not exactly sure what you're touching, but could it wait for the
below pg_depend patch to be either accepted or rejected?  It lightly
fiddles with a number of files in the command and catalog directories.

http://archives.postgresql.org/pgsql-patches/2002-04/msg00050.php


> > That shouldn't be too much of a problem in the next couple of
weeks - if
> > we can decide on a specific day I'll book it into my diary (Any
day but
> > Wednesday next week would be fine for me).
>
> I will try to have no uncommitted changes over this weekend; that
will
> give you a clear field Monday morning, or you can start on the
weekend
> if you like.  Sound good?



Sun, 14 Apr 2002 16:30:23 -0400
"Rod Taylor" <rbt@zort.ca>
Re: command.c breakup 
On Sun, 2002-04-14 at 21:30, Rod Taylor wrote:
> I'm not exactly sure what you're touching, but could it wait for the
> below pg_depend patch to be either accepted or rejected?  It lightly
> fiddles with a number of files in the command and catalog directories.
> 
> http://archives.postgresql.org/pgsql-patches/2002-04/msg00050.php
> 

Well, I'm working on it now and it's about 75% done. I hope to post the
patch within the next few hours. I'm sorry that I wasn't aware of your
patch -but commands/ is a busy place at present :). I've scanned your
patch very briefly and the major impacts I can see are:

1) The ALTER TABLE code will be in tablecmds.c (but exactly the same
code as at present)

2) The type support will be in typecmds.c (define.c and remove.c are
essentially gone -the define and remove commands for foo are in general
now together in foocmds.c

I'm not touching anything in the catalog directory. 

Note that as I'm only shuffling code from one file to another, your
patch shouldn't need much modification to get it working afterwards -
although there is an intention to tidy up common code in the commands/
directory as a second phase, this will consist of more "ordinary"
patches...

Regards

John



14 Apr 2002 21:43:09 +0100
John Gray <jgray@azuli.co.uk>
Re: command.c breakup
Sounds fair.  I'd have brought it up earlier but was away last week.

The changes I made are very straight forward and easy enough to redo.
--
Rod Taylor

Your eyes are weary from staring at the CRT. You feel sleepy. Notice
how restful it is to watch the cursor blink. Close your eyes. The
opinions stated above are yours. You cannot imagine why you ever felt
otherwise.

----- Original Message -----
From: "John Gray" <jgray@azuli.co.uk>
To: "Rod Taylor" <rbt@zort.ca>
Cc: "Tom Lane" <tgl@sss.pgh.pa.us>; "Christopher Kings-Lynne"
<chriskl@familyhealth.com.au>; "Hackers"
<pgsql-hackers@postgresql.org>
Sent: Sunday, April 14, 2002 4:43 PM
Subject: Re: [HACKERS] command.c breakup


> On Sun, 2002-04-14 at 21:30, Rod Taylor wrote:
> > I'm not exactly sure what you're touching, but could it wait for
the
> > below pg_depend patch to be either accepted or rejected?  It
lightly
> > fiddles with a number of files in the command and catalog
directories.
> >
> > http://archives.postgresql.org/pgsql-patches/2002-04/msg00050.php
> >
>
> Well, I'm working on it now and it's about 75% done. I hope to post
the
> patch within the next few hours. I'm sorry that I wasn't aware of
your
> patch -but commands/ is a busy place at present :). I've scanned
your
> patch very briefly and the major impacts I can see are:
>
> 1) The ALTER TABLE code will be in tablecmds.c (but exactly the same
> code as at present)
>
> 2) The type support will be in typecmds.c (define.c and remove.c are
> essentially gone -the define and remove commands for foo are in
general
> now together in foocmds.c
>
> I'm not touching anything in the catalog directory.
>
> Note that as I'm only shuffling code from one file to another, your
> patch shouldn't need much modification to get it working
afterwards -
> although there is an intention to tidy up common code in the
commands/
> directory as a second phase, this will consist of more "ordinary"
> patches...
>
> Regards
>
> John
>
>


Sun, 14 Apr 2002 16:58:40 -0400
"Rod Taylor" <rbt@zort.ca>
Re: command.c breakup
On Sun, 2002-04-14 at 21:58, Rod Taylor wrote:
> Sounds fair.  I'd have brought it up earlier but was away last week.
> 
> The changes I made are very straight forward and easy enough to redo.

I've sent the patch to the -patches list -Please let me know if there
are any queries -I will be able to deal with them after ~1700 UTC
Monday.

Regards

John



15 Apr 2002 00:08:26 +0100
John Gray <jgray@azuli.co.uk>
Re: command.c breakup