Thread: 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