commands subdirectory continued -code cleanup
Dear all,
I've been looking at tidying up some of the repeated code which now
resides in tablecmds.c - in particular the ALTER TABLE ALTER COLUMN
code.
Most of these routines share common code:
1) AccessExclusive Lock on relation.
2) Relation is a table, not a system table, user is owner.
3) Recurse over child relations.
And several routines then:
4) check column exists
5) check column is not a system attribute
I would propose to combine these checks into two routines:
CanAlterTable(relid,systemOK) [systemOK is for the set statistics case]
GetTargetAttnum(relid,Attname) returns attnum
[This would bring some consistency to checking, for example fixing the
current segfault if you try ALTER TABLE test ALTER COLUMN xmin SET
DEFAULT 3;]
and two macros:
RECURSE_OVER_CHILDREN(relid);
AlterTableDoSomething(childrel,...);
RECURSE_OVER_CHILDREN_END;
(this seems more straightforward than passing the text of the function
call as a macro parameter).
ALTER COLUMN RENAME
Currently, attributes in tables, views and sequences can be renamed.
-tables and views make sense, of course.
Sequences still seem to work after they've had attributes renamed, but I
see little value in being able to do this. Is it OK to prohibit the
renaming of sequence columns?
tcop/utility.c vs. commands/
There are also permissions checks made in tcop/utility.c before
AlterTableOwner and renamerel are called. It may be best to move these
into commands/tablecmds.c. It seems that tcop/utility.c was supposed to
handle the permissions checks for statements, but the inheritance
support has pushed some of that into commands/ . Should permissions
checking for other utility statements be migrated to commands/ for
consistency? I don't propose to do this now -but it might be a later
stage in the process.
If this general outline is OK, I'll work on a patch -this shouldn't be
quite as drastic as the last one :-)
Regards
John
--
John Gray ECHOES: sponsored walks for Christian Aid to the highest
Azuli IT points of English counties, 4th-6th May 2002
www.azuli.co.uk www.stannesmoseley.care4free.net/echoes.html
John Gray <jgray@azuli.co.uk> writes:
Sequences still seem to work after they've had attributes renamed, but I
see little value in being able to do this. Is it OK to prohibit the
renaming of sequence columns?
That seems like an error to me. Setting defaults, constraints, etc on a
sequence is bogus too --- do we catch those?
There are also permissions checks made in tcop/utility.c before
AlterTableOwner and renamerel are called. It may be best to move these
into commands/tablecmds.c. It seems that tcop/utility.c was supposed to
handle the permissions checks for statements, but the inheritance
support has pushed some of that into commands/ . Should permissions
checking for other utility statements be migrated to commands/ for
consistency? I don't propose to do this now -but it might be a later
stage in the process.
Not sure. There are subroutines in utility.c that are useful for
this purpose, and I don't really see the value of having them called
from all over the place when it can be more localized. We should
probably be consistent about having tablecmds.c make all the relevant
permissions checks for its operations, but I don't think that
necessarily translates into the same choice for the rest of commands/.
AFAIR none of the rest of commands/ has the recursive-operations issue
that forces this approach for table commands.
regards, tom lane
John Gray wrote:
and two macros:
RECURSE_OVER_CHILDREN(relid);
AlterTableDoSomething(childrel,...);
RECURSE_OVER_CHILDREN_END;(this seems more straightforward than passing the text of the function
call as a macro parameter).
Suggestion:
RECURSE_OVER_CHILDREN(inh, relid)
{
AlterTableDoSomething(childrel,...);
}
--
Fernando Nasser
Red Hat - Toronto E-Mail: fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario M4P 2C9
On Fri, 2002-04-19 at 20:34, Tom Lane wrote:
John Gray <jgray@azuli.co.uk> writes:
Sequences still seem to work after they've had attributes renamed, but I
see little value in being able to do this. Is it OK to prohibit the
renaming of sequence columns?That seems like an error to me. Setting defaults, constraints, etc on a
sequence is bogus too --- do we catch those?
Yes, we catch those. The current gaps in checking are:
1) renameatt allows any type of relation to have columns renamed
(including indexes, but that case is caught by heap_open objecting to
the relation being an index)
2) add/drop default allows the addition/dropping of constraints on
system attributes (Apropos of this, I think it might also be good to add
a check into AddRelationRawConstraints that verifies that attnum in
colDef is actually positive and <natts for the relation -just in case it
is passed a bogus structure from somewhere else. This would be a cheap
check to do.)
Not sure. There are subroutines in utility.c that are useful for
this purpose, and I don't really see the value of having them called
from all over the place when it can be more localized. We should
probably be consistent about having tablecmds.c make all the relevant
permissions checks for its operations, but I don't think that
necessarily translates into the same choice for the rest of commands/.
AFAIR none of the rest of commands/ has the recursive-operations issue
that forces this approach for table commands.
OK. I can remove the duplicate system relation check from renamerel and
just leave a comment explaining that the permissions checks are
performed in utility.c. (renamerel is unlike other tablecmds.c functions
in that it doesn't recurse. It is also called from cluster.c but again,
permissions are checked for that in tcop/utility.c)
Regards
John
On Fri, 2002-04-19 at 20:38, Fernando Nasser wrote:
John Gray wrote:
and two macros:
RECURSE_OVER_CHILDREN(relid);
AlterTableDoSomething(childrel,...);
RECURSE_OVER_CHILDREN_END;(this seems more straightforward than passing the text of the function
call as a macro parameter).Suggestion:
RECURSE_OVER_CHILDREN(inh, relid)
{
AlterTableDoSomething(childrel,...);
}
Yes, that would be nicer -I just have to work out a suitable rewrite of
the code so that it could fit that kind of macro setup -at present,
there is code that is executed in each iteration of the loop, which
means there is more than one group to close after the AlterTable call.
I'll think on it...
Regards
John
--
John Gray ECHOES: sponsored walks for Christian Aid to the highest
Azuli IT points of English counties, 4th-6th May 2002
www.azuli.co.uk www.stannesmoseley.care4free.net/echoes.html
<snip>
and two macros:
RECURSE_OVER_CHILDREN(relid);
AlterTableDoSomething(childrel,...);
RECURSE_OVER_CHILDREN_END;(this seems more straightforward than passing the text of the function
call as a macro parameter).
The above all looks fine. The other stuff I wouldn't really know about.
Chris