CREATE CAST code review

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: CREATE CAST code review
Date: 2002-07-21 00:11:33
Message-ID: 15458.1027210293@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2002-07-21 02:00:01 Re: [PATCHES] prepareable statements
Previous Message Tom Lane 2002-07-20 21:51:13 Re: Demo patch for DROP COLUMN