Thread: Refactoring of command.c
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
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
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
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
> 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, Pennsylvania19026
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
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, Pennsylvania19026
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
pgman wrote: > 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. Good point about keeping CVS logs. Can't we just move the CVS file to another name, create an empty file in its place, then delete it. I thought that would move the history. We have never done it but I would think there is a way 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, Pennsylvania19026
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
A simple copy works quite well. Bit of a waste of space but you preserve the history in both locations. An empty file will not work. If someone were to check out an older version it would be broken. -- Rod Taylor This message represents the official view of the voices in my head ----- Original Message ----- From: "Bruce Momjian" <pgman@candle.pha.pa.us> To: <pgman@candle.pha.pa.us> Cc: "Peter Eisentraut" <peter_e@gmx.net>; "John Gray" <jgray@azuli.co.uk>; <pgsql-hackers@postgresql.org> Sent: Wednesday, February 27, 2002 12:31 AM Subject: Re: [HACKERS] Refactoring of command.c > pgman wrote: > > 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. > > Good point about keeping CVS logs. Can't we just move the CVS file to > another name, create an empty file in its place, then delete it. I > thought that would move the history. We have never done it but I would > think there is a way 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 > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) >
It will, but I've always been able to make the assumption it won't be compiled in as the make files always specified exactly which ones to compile (and would ignore the new 'stray'). Perhaps the best way to approach this would be to ask the FreeBSD 'repo' people what they do -- as they do this kind of stuff quite frequently. -- Rod Taylor This message represents the official view of the voices in my head ----- Original Message ----- From: "Tom Lane" <tgl@sss.pgh.pa.us> To: "Rod Taylor" <rbt@zort.ca> Cc: "Bruce Momjian" <pgman@candle.pha.pa.us>; "Peter Eisentraut" <peter_e@gmx.net>; "John Gray" <jgray@azuli.co.uk>; <pgsql-hackers@postgresql.org> Sent: Wednesday, February 27, 2002 10:13 AM Subject: Re: [HACKERS] Refactoring of command.c > "Rod Taylor" <rbt@zort.ca> writes: > > A simple copy works quite well. Bit of a waste of space but you > > preserve the history in both locations. > > If you do that, the copied file will appear to CVS to be part of older > versions, no? > > regards, tom lane >
Dug the below out of googles cache -- it's what the BSDs do for moving files in cvs. What is a repo-copy? A repo-copy (which is a short form of ``repository copy'') refers to the direct copying of files within the CVS repository. Without a repo-copy, if a file needed to be copied or moved to another place in the repository, the committer would run cvs add to put the file in its new location, and then cvs rm on the old file if the old copy was being removed. The disadvantage of this method is that the history (i.e. the entries in the CVS logs) of the file would not be copied to the new location. As the FreeBSD Project considers this history very useful, a repository copy is often used instead. This is a process where one of the repository meisters will copy the files directly within the repository, rather than using the cvs program. -- Rod Taylor This message represents the official view of the voices in my head ----- Original Message ----- From: "Tom Lane" <tgl@sss.pgh.pa.us> To: "Rod Taylor" <rbt@zort.ca> Cc: "Bruce Momjian" <pgman@candle.pha.pa.us>; "Peter Eisentraut" <peter_e@gmx.net>; "John Gray" <jgray@azuli.co.uk>; <pgsql-hackers@postgresql.org> Sent: Wednesday, February 27, 2002 10:13 AM Subject: Re: [HACKERS] Refactoring of command.c > "Rod Taylor" <rbt@zort.ca> writes: > > A simple copy works quite well. Bit of a waste of space but you > > preserve the history in both locations. > > If you do that, the copied file will appear to CVS to be part of older > versions, no? > > regards, tom lane >
"Rod Taylor" <rbt@zort.ca> writes:
> A simple copy works quite well. Bit of a waste of space but you
> preserve the history in both locations.
If you do that, the copied file will appear to CVS to be part of older
versions, no?
regards, tom lane
"Rod Taylor" <rbt@zort.ca> writes:
> Dug the below out of googles cache -- it's what the BSDs do for moving
> files in cvs.
> What is a repo-copy?
> A repo-copy (which is a short form of ``repository copy'') refers to
> the direct copying of files within the CVS repository.
Yeah, I think that's what we discussed the last time the question came
up.
It seems awfully wrongheaded to me. IMHO, the entire point of a CVS
repository is to store past states of your software, not only the
current state. Destroying the accurate representation of your historical
releases is a poor tradeoff for making it a little easier to find the
log entries for code that's been moved around. What's the point
of having history, if it's not accurate?
regards, tom lane
> > What is a repo-copy? > > A repo-copy (which is a short form of ``repository copy'') refers to > > the direct copying of files within the CVS repository. > > Yeah, I think that's what we discussed the last time the question came > up. > > It seems awfully wrongheaded to me. IMHO, the entire point of a CVS > repository is to store past states of your software, not only the > current state. Destroying the accurate representation of your historical > releases is a poor tradeoff for making it a little easier to find the > log entries for code that's been moved around. What's the point > of having history, if it's not accurate? Sounds like it's time to move to using 'arch': http://www.regexps.com/#arch Supports everything that CVS doesn't, including rename events... BTW - I'm not _seriously_ suggesting this change - but it would be cool, wouldn't it? People could start their own local branches which are part of the global namespace, easily merge them in, etc... Chris
On Thu, 28 Feb 2002, Christopher Kings-Lynne wrote: [Shame on Christopher for breaking attributions] > > > What is a repo-copy? > > > A repo-copy (which is a short form of ``repository copy'') refers to > > > the direct copying of files within the CVS repository. > > > > Yeah, I think that's what we discussed the last time the question came > > up. > > > > It seems awfully wrongheaded to me. IMHO, the entire point of a CVS > > repository is to store past states of your software, not only the > > current state. Destroying the accurate representation of your historical > > releases is a poor tradeoff for making it a little easier to find the > > log entries for code that's been moved around. What's the point > > of having history, if it's not accurate? > > Sounds like it's time to move to using 'arch': I see this going down the road of a religious debate, and to prove the point, I bring up BitKeeper: http://www.bitkeeper.com > http://www.regexps.com/#arch > > Supports everything that CVS doesn't, including rename events... So does BitKeeper :) > BTW - I'm not _seriously_ suggesting this change - but it would be cool, > wouldn't it? > > People could start their own local branches which are part of the global > namespace, easily merge them in, etc... This seems quite pointless for PostgreSQL's development. C'est la vie. -- Dominic J. Eidson "Baruk Khazad! Khazad ai-menu!" - Gimli ------------------------------------------------------------------------------- http://www.the-infinite.org/ http://www.the-infinite.org/~dominic/
"Dominic J. Eidson" <sauron@the-infinite.org> writes:
> On Thu, 28 Feb 2002, Christopher Kings-Lynne wrote:
>> Sounds like it's time to move to using 'arch':
> I see this going down the road of a religious debate, and to prove the
> point, I bring up BitKeeper:
Hmm. I'd surely be the last to claim that CVS is the be-all and end-all
of software archiving systems. But is BitKeeper, or arch, or anything
else enough better to justify the pain of switching? This is intended
as an honest question, not flamebait --- I haven't looked closely at
the alternatives.
regards, tom lane
> I see this going down the road of a religious debate, and to prove the > point, I bring up BitKeeper: > > http://www.bitkeeper.com I admit I don't know much about bitkeeper, except its license is a bit weird... > > http://www.regexps.com/#arch > > > > Supports everything that CVS doesn't, including rename events... > > So does BitKeeper :) > > > BTW - I'm not _seriously_ suggesting this change - but it would be cool, > > wouldn't it? > > > > People could start their own local branches which are part of the global > > namespace, easily merge them in, etc... > > This seems quite pointless for PostgreSQL's development. NOT TRUE!!! Imagine you want to develop a massive new feature for Postgres. You just create a branch on your own machine, do all your changes, commits, etc. and keep it current with the main branch. Then, you can merge it back into the main tree... That way you can have a history of commits on your own branch of the repo! Disclaimer: Have only read docs, not actually _used_ 'arch'... :( Chris
On Thu, 2002-02-28 at 06:44, Tom Lane wrote: > Hmm. I'd surely be the last to claim that CVS is the be-all and end-all > of software archiving systems. But is BitKeeper, or arch, or anything > else enough better to justify the pain of switching? This is intended > as an honest question, not flamebait --- I haven't looked closely at > the alternatives. There was a discussion on Slashdot a couple of weeks ago, with (as usual) some interesting comments out of the lot. It's at http://slashdot.org/comments.pl?sid=27540&threshold=4&mode=flat including comments from Subversion's Karl Fogel. -- Alessio F. Bragadini alessio@albourne.com APL Financial Services http://village.albourne.com Nicosia, Cyprus phone: +357-22-755750 "It is more complicated than you think" -- The Eighth Networking Truth from RFC 1925
> Imagine you want to develop a massive new feature for Postgres. You just > create a branch on your own machine, do all your changes, commits, etc. and > keep it current with the main branch. Then, you can merge it back into the > main tree... That way you can have a history of commits on your own branch > of the repo! The same thing can be accomplished with CVS as well -- it's just not as pretty. There is a reason that the FreeBSD group uses $FreeBSD$ and leaves $Id$ untouched. Basically, check out of one, drop CVS directories, check into the second, check out of the second, and when doing work with either repository you specify which repo with the -D flag. Coupled with the -j (merge) flag you can accomplish most tasks. That said, if the work was thought through and beneficial you may be able to obtain a branch in postgresql cvs to work with.