I looked through your CREATE CAST commit a little.  Looks pretty good
but I had a few suggestions/concerns.

* The biggie is that I'm not satisfied with the permissions checking.
You have "To be able to create a cast, you must own the underlying
function. To be able to create a binary compatible cast, you must own
both the source and the target data type."  (Same to drop a cast.)
It seems to me that this is quite dangerous, as any random luser who
can access both datatypes can create a function and then a cast,
thereby changing the behavior of *both* types in rather subtle ways,
and perhaps insinuating unexpected, untrusted code into queries issued
by people who weren't expecting any cast to be applied.
(Not to mention causing a denial-of-service for the legitimate definer
of that cast, if he hadn't gotten around to making it quite yet.)
The problem would be slightly less bad if function definition required
USAGE privilege on the arg/result types ... but not much.

I think I would prefer this definition: to create/drop a cast, you must
own both datatypes, plus the underlying function if any.  What's the
rationale for having such a weak permissions scheme?


* I see that you are worried in pg_dump about which schema to associate
a cast with, if it's binary-compatible.  I'm confused too; would it work
to dump the cast iff either of the source/dest datatypes are to be
dumped?  (This might be a better rule even in the not-binary-compatible
case.)


* Various more-or-less minor coding gripes:

* pg_cast table not documented in catalogs.sgml.

* shoddy implementation of getObjectDescription() for cast.  (I have
some to-do items in that routine myself, though, and will be happy to fix
this while at it.)

* since pg_cast depends on having a unique OID, it *must* have an OID
index to enforce that uniqueness; otherwise the system can fail after
OID wraparound.

* since you must define the OID index anyway, you may as well use it in
a systable scan in DropCastById, instead of using heapscan.

* in CreateCast, there's no need to use the relatively expensive 
get_system_catalog_relid() lookup; you've got pg_cast open and so
you can just do RelationGetRelid on it.

			regards, tom lane

PS: I also want to raise a flag that we still haven't resolved the
issues we discussed a few months ago, about exactly *which* implicit
casts should be provided.  I think that's a different thread though.

Sat, 20 Jul 2002 20:11:33 -0400
Tom Lane <tgl@sss.pgh.pa.us>
CREATE CAST code review
Tom Lane writes:

> I looked through your CREATE CAST commit a little.  Looks pretty good
> but I had a few suggestions/concerns.
>
> * The biggie is that I'm not satisfied with the permissions checking.

Me neither.  I had sent a message earlier about this but it went
unnoticed, but I had to implement something that was a little more
restrictive that the previous first come first serve scheme.

> I think I would prefer this definition: to create/drop a cast, you must
> own both datatypes, plus the underlying function if any.  What's the
> rationale for having such a weak permissions scheme?

That doesn't quite work, because then no ordinary user can define a cast
from some built-in type to his own type.  What I'm thinking about is to
implement the USAGE privilege on types, and then you need to have that to
be allowed to create casts.  (Possibly there should be an even more
restrictive privilege invented here, but that would be a second step.)

> * I see that you are worried in pg_dump about which schema to associate
> a cast with, if it's binary-compatible.  I'm confused too; would it work
> to dump the cast iff either of the source/dest datatypes are to be
> dumped?  (This might be a better rule even in the not-binary-compatible
> case.)

I'm not sure about the implications of associating objects with schemas in
pg_dump.  I suppose there might be an option to dump only certain schemas,
in which case it's tricky to associate a cast to any one schema.

> PS: I also want to raise a flag that we still haven't resolved the
> issues we discussed a few months ago, about exactly *which* implicit
> casts should be provided.  I think that's a different thread though.

Yes, I have some thoughts on that which I plan to present soon.

-- 
Peter Eisentraut   peter_e@gmx.net


Mon, 22 Jul 2002 20:35:56 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: CREATE CAST code review
Peter Eisentraut <peter_e@gmx.net> writes:
> That doesn't quite work, because then no ordinary user can define a cast
> from some built-in type to his own type.  What I'm thinking about is to
> implement the USAGE privilege on types, and then you need to have that to
> be allowed to create casts.

Still seems too weak.  What about requiring ownership of at least one
of the types?

> I'm not sure about the implications of associating objects with schemas in
> pg_dump.  I suppose there might be an option to dump only certain schemas,

That is the intention (it's not implemented yet).

			regards, tom lane

Mon, 22 Jul 2002 16:06:22 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: CREATE CAST code review 
Tom Lane writes:

> What about requiring ownership of at least one of the types?

Yes, that would work.

There would be a somewhat bizzare consequence, though:  User U1 creates
type T1, user U2 creates type T2.  Then user U1 creates a cast from T1 to
T2.  Now user U2 would be allowed to drop that cast (unless we store a
cast owner).  I guess that lies in the nature of things.

A much more complex yet powerful alternative would be to associate casts
with schemas.  For example, this would allow an ordinary user to create a
cast from numeric to text in his own little world.  But that might be
going too far.

> > I'm not sure about the implications of associating objects with schemas in
> > pg_dump.  I suppose there might be an option to dump only certain schemas,
>
> That is the intention (it's not implemented yet).

My concern was that if you, say, have two schemas and a cast that involves
types from both schemas.  If you dump all of them, you have a consistent
dump.  But if you dump both schemas separately, do you dump the cast in
both of them (thus making each schema's dump self-contained) or in only
one of them (thus allowing concatenation the dumps).  This issue
generalizes to every kind of dependency in pg_dump.

-- 
Peter Eisentraut   peter_e@gmx.net


Sat, 27 Jul 2002 20:05:32 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: CREATE CAST code review 