I thought I should just raise a couple of points -I think I may have
been the person Tom Lane was referring with respect to refactoring
command.c. I am still happy to go through and do this, though I think
that Neil Conway may be active there at present.

1) Division between files.

I imagined that the ALTER TABLE would move to alter.c (but maybe this
should be altertable.c?), the portal support would live in portal.c and
I wasn't sure where LOCK TABLE would go. If we're going to have a
command for showing current locks (I seem to remember that from some
time back, maybe it would go there too?)

2) Permissions checking and inheritance.

Much of the permissions checking and setup on the alter table variants
is the same. In my original patch I'd extracted this into a helper
routine. Also, all the commands which support inheritance do so using a
piece of boilerplate code -I had thought this could become a pair of
macros, either side of whatever action was to be performed on the
descendant. 

3) ALTER TABLE CREATE TOAST TABLE

I don't see any need for this in the grammar! AlterTableAddColumn calls
it after a catalogue change, it is called (from ProcessUtility
tcop/utility.c) for CREATE TABLE, and InitPlan calls it for SELECT INTO.

I can't see any useful circumstance in which the command could be
issued, which makes it seem a good candidate for removal from the
grammar. 

Any comments on these?

John


26 Feb 2002 10:32:17 +0000
John Gray <jgray@azuli.co.uk>
Refactoring of command.c
John Gray <jgray@azuli.co.uk> writes:
> 3) ALTER TABLE CREATE TOAST TABLE

> I can't see any useful circumstance in which the command could be
> issued, which makes it seem a good candidate for removal from the
> grammar. 

initdb counts as moderately useful ;-)

			regards, tom lane

Tue, 26 Feb 2002 10:17:20 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Refactoring of command.c 
John Gray writes:

> I imagined that the ALTER TABLE would move to alter.c (but maybe this
> should be altertable.c?),

Put all the table creation and altering code into the same file.

-- 
Peter Eisentraut   peter_e@gmx.net


Tue, 26 Feb 2002 14:19:17 -0500 (EST)
Peter Eisentraut <peter_e@gmx.net>
Re: Refactoring of command.c
On Tue, 2002-02-26 at 19:19, Peter Eisentraut wrote:
> John Gray writes:
> 
> > I imagined that the ALTER TABLE would move to alter.c (but maybe this
> > should be altertable.c?),
> 
> Put all the table creation and altering code into the same file.
> 

Following this, here is an overview of a possible rearrangement of the
files in the commands/ directory. Those not mentioned I intend to leave
alone. Obviously, I'd also change the include files (and any files
referencing them) to match.

The aim is that each filename will be either a) the name of a command
(explain, analyze, vacuum) or b) a type/class of database object.
(async.c would remain an exception to this)

The new table.c should be amenable to quite a bit of clearing up. I
expect that it might come out at around 9000 lines before that's done,
which is quite long, but it would at least be a well-defined subsystem.

command.c  	ALTER TABLE 	-> table.c
		portal		-> portal.c
		LOCK		-> lock.c

creatinh.c	all code 	-> table.c

dbcommands.c	rename to database.c (see below)

define.c	see below

indexcmds.c	rename to index.c (see below)

proclang.c	case_translate_language_name currently also in define.c

remove.c	see below

rename.c	update_ri_trigger_args -> trigger.c
		renameatt, renamerel   -> table.c


define.c and remove.c

These files currently support the "smaller" entities. These could
obviously be split out, so that we have function.c, operator.c,
opclass.c, aggregate.c . This adds more files, but has the merit of
making it obvious where any further future support for these entities
should go. Any feelings about this?

renamed files

I know that "renamed" files in CVS will lose their history (for
tracability we presumably wouldn't rename them in the repository), so I
accept that it may not be a great idea to rename them, but it seems a
little redundant to use commands/dbcommands.c rather than
commands/database.c. 

As I'm new to this kind of change, I assume that I'd just submit a
normal context diff for this and rely on it not getting tangled up with
any other patches to these files? Or is this *too* radical a reshuffle?
:-)

John


26 Feb 2002 23:50:01 +0000
John Gray <jgray@azuli.co.uk>
Re: Refactoring of command.c
> I know that "renamed" files in CVS will lose their history (for
> tracability we presumably wouldn't rename them in the repository), so I
> accept that it may not be a great idea to rename them, but it seems a
> little redundant to use commands/dbcommands.c rather than
> commands/database.c. 
> 
> As I'm new to this kind of change, I assume that I'd just submit a
> normal context diff for this and rely on it not getting tangled up with
> any other patches to these files? Or is this *too* radical a reshuffle?
> :-)

Post the patch and we will try to get it approved quickly for
application.  That's about the only clean way I know to do it.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tue, 26 Feb 2002 22:15:25 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Refactoring of command.c
John Gray writes:

> dbcommands.c	rename to database.c (see below)
> indexcmds.c	rename to index.c (see below)

Might as well keep these.  They don't hurt anyone just because they spell
a little differently.

-- 
Peter Eisentraut   peter_e@gmx.net


Tue, 26 Feb 2002 22:39:43 -0500 (EST)
Peter Eisentraut <peter_e@gmx.net>
Re: Refactoring of command.c
Peter Eisentraut wrote:
> John Gray writes:
> 
> > dbcommands.c	rename to database.c (see below)
> > indexcmds.c	rename to index.c (see below)
> 
> Might as well keep these.  They don't hurt anyone just because they spell
> a little differently.

I disagree.  If we are cleaning, let's clean.  *cmd* is redundant.  The
contents of the files will be quite different anyway.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tue, 26 Feb 2002 22:47:34 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Refactoring of command.c
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Peter Eisentraut wrote:
>> John Gray writes:
> dbcommands.c	rename to database.c (see below)
> indexcmds.c	rename to index.c (see below)
>> 
>> Might as well keep these.  They don't hurt anyone just because they spell
>> a little differently.

> I disagree.  If we are cleaning, let's clean.  *cmd* is redundant.  The
> contents of the files will be quite different anyway.

But if we rename them, we will lose CVS history for the files (unless we
jump through hoops that typically are not jumped through).  I agree with
Peter: do not rename files simply to have slightly simpler file names.

			regards, tom lane

Wed, 27 Feb 2002 00:03:17 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Refactoring of command.c 
John Gray <jgray@azuli.co.uk> writes:
> As I'm new to this kind of change, I assume that I'd just submit a
> normal context diff for this and rely on it not getting tangled up with
> any other patches to these files? Or is this *too* radical a reshuffle?

As far as mechanics go, the main problem is that you can't expect those
files to hold still while you contemplate what to do with them; there's
probably a commit every day or two that hits at least one.

Your sketch of what-goes-where is a good first cut for discussion.  Once
you've learned what you can at this level, I'd suggest preparing a much
more detailed sketch, at the per-routine level, and posting that for
discussion.  Once people are happy with that, you can coordinate making
the actual patch and passing it to one of the committers for immediate
application.

Once it does get down to the actual diff, I'd suggest a context diff
plus specific notes to the committer that "this patch adds files a,b,c
and removes files x,y,z".  The cvs adds and cvs removes are extra steps,
of course, so it's good to point them out to be sure they're not missed.

			regards, tom lane

Wed, 27 Feb 2002 01:37:17 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Refactoring of command.c 