Thread: disallow LOCK on a view
This patch is for the TODO item * Disallow LOCK on view src/backend/commands/command.c is the only affected file -- Mark Hollomon mhh@mindspring.com
Mark Hollomon <mhh@mindspring.com> writes:
> sprintf(rulequery, "select * from pg_views where viewname='%s'", relname);
> [ evaluate query via SPI ]
I really dislike seeing backend utility operations built atop SPI.
Quite aside from the (lack of) speed, there are all sorts of nasty
traps that can come from runtime evaluation of query strings. The
most obvious example in this case is what if relname contains a quote
mark? Or backslash?
The permanent memory leak induced by SPI_saveplan() is another good
reason not to do it this way.
Finally, once one has written a nice neat little is_view() query
function, there's a strong temptation to just use it from anywhere,
without thought for the side-effects it might have like grabbing/
releasing locks, CommandCounterIncrement(), etc. There are many
places in the backend where the side-effects of doing a full query
evaluation would be harmful.
Mark's patch is OK as is, since it's merely relocating some poorly
written code and not trying to fix it, but someone ought to think
about fixing the code.
regards, tom lane
Tom Lane wrote: > > Mark's patch is OK as is, since it's merely relocating some poorly > written code and not trying to fix it, but someone ought to think > about fixing the code. > I'll take a crack at it. Just out of curiousity, is there technical reason there isn't a (say) relisview attribute to pg_class? -- Mark Hollomon mhh@nortelnetworks.com ESN 451-9008 (302)454-9008
"Mark Hollomon" <mhh@nortelnetworks.com> writes:
> Just out of curiousity, is there technical reason there isn't
> a (say) relisview attribute to pg_class?
That might indeed be the most reasonable way to attack it, rather
than having to go messing about looking for a matching rule.
(Jan, any thoughts here?)
Adding a column to a core system table like pg_class is a good
exercise for the student ;-) ... it's not exactly automated,
and you have to find all the places that need to be updated.
You might want to keep notes and prepare a writeup for the
developer's FAQ. I thought of that the last time I did something
similar, but it was only at the end that I realized I should've
been keeping notes to start with.
regards, tom lane
Tom Lane wrote: > "Mark Hollomon" <mhh@nortelnetworks.com> writes: > > Just out of curiousity, is there technical reason there isn't > > a (say) relisview attribute to pg_class? > > That might indeed be the most reasonable way to attack it, rather > than having to go messing about looking for a matching rule. > (Jan, any thoughts here?) The right way IMHO would be to give views another relkind. Then we could easily 1. detect if the final query after rewriting still tries to INSERT/UPDATE/DELETE a view - i.e. "missing rewrite rule(s)". 2. disable things like LOCK etc. The problem here is, that the relkind must change at rule creation/drop time. Fortunately rules on SELECT aretotally restricted to VIEW's since 6.4, and I don't see any reason to change this. And it's time to make more use of the relkind attribute. For 7.2, when we want to have tuple-set returns for functions,we might want to have structures as well (we talked about that already, Tom). A structure is just a row/typedescription. A function, returning a tuple or set of tuples, can return this type or set of type as well asany other existing table/view structure. So to create a function returning a set of tuples, which have a structuredifferent from any existing table, someone creates a named structure, then the function returningtuples of that type. These structures are just entries in pg_class, pg_attribute and pg_type. There isno file or any rules, triggers etc. attached to them. They just describe a typle that can be built in memory. > Adding a column to a core system table like pg_class is a good > exercise for the student ;-) ... it's not exactly automated, > and you have to find all the places that need to be updated. > You might want to keep notes and prepare a writeup for the > developer's FAQ. I thought of that the last time I did something > similar, but it was only at the end that I realized I should've > been keeping notes to start with. Meetoo :-} Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck wrote: > > Tom Lane wrote: > > "Mark Hollomon" <mhh@nortelnetworks.com> writes: > > > Just out of curiousity, is there technical reason there isn't > > > a (say) relisview attribute to pg_class? > > > > That might indeed be the most reasonable way to attack it, rather > > than having to go messing about looking for a matching rule. > > (Jan, any thoughts here?) > > The right way IMHO would be to give views another relkind. > Then we could easily > > 1. detect if the final query after rewriting still tries to > INSERT/UPDATE/DELETE a view - i.e. "missing rewrite > rule(s)". This appeals to me. The current silent no-op behavior of INSERT/DELETE on a view is annoying. -- Mark Hollomon mhh@nortelnetworks.com ESN 451-9008 (302)454-9008
>> The right way IMHO would be to give views another relkind.
> This appeals to me.
I like it too. Aside from the advantages Jan mentioned, we could also
refrain from creating an underlying file for a view, which would be
nice to avoid cluttering the database directory.
regards, tom lane
Tom Lane wrote: > >> The right way IMHO would be to give views another relkind. > > > This appeals to me. > > I like it too. Aside from the advantages Jan mentioned, we could also > refrain from creating an underlying file for a view, which would be > nice to avoid cluttering the database directory. From memory I think views are created as CREATE TABLE, with an internal DefineRuleStmt, and dumped as CREATE TABLE, CREATE RULE for sure. So the CREATE/DROP RULE would need to remove/recreate the tables file (plus toastfile and index) if you want it to be consistent. Don't think you want that - do you? Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck <janwieck@Yahoo.com> writes:
> From memory I think views are created as CREATE TABLE, with
> an internal DefineRuleStmt, and dumped as CREATE TABLE,
> CREATE RULE for sure. So the CREATE/DROP RULE would need to
> remove/recreate the tables file (plus toast file and index)
> if you want it to be consistent. Don't think you want that -
> do you?
But that's only true because it's such a pain in the neck for pg_dump
to discover that a table is a view. If this could easily be told from
inspection of pg_class, then it'd be no problem to dump views as
CREATE VIEW statements in the first place --- obviously better, no?
However the initial version upgrade would be a problem, since dump
files out of existing releases would contain CREATE TABLE & RULE
commands instead of CREATE VIEW. I guess what would happen is that
views reloaded that way wouldn't really be views, they'd be tables
with rules attached. Grumble.
How about this:CREATE RULE of an on-select-instead rule changes table'srelkind to 'view'. We don't need to drop the
underlyingtablefile, though (just leave it be, it'll go away atnext initdb).
DROP RULE of a view's on-select-instead is not allowed.You have to drop the whole view instead.
regards, tom lane
Tom Lane wrote: > > Jan Wieck <janwieck@Yahoo.com> writes: > > From memory I think views are created as CREATE TABLE, with > > an internal DefineRuleStmt, and dumped as CREATE TABLE, > > CREATE RULE for sure. So the CREATE/DROP RULE would need to > > remove/recreate the tables file (plus toast file and index) > > if you want it to be consistent. Don't think you want that - > > do you? > > But that's only true because it's such a pain in the neck for pg_dump > to discover that a table is a view. If this could easily be told from > inspection of pg_class, then it'd be no problem to dump views as > CREATE VIEW statements in the first place --- obviously better, no? The fact that views can be created by a separate table/rule sequence allows pg_dump to properly dump views which are based upon functions, or views which may have dependencies on other tables/views. The new pg_dump dumps in oid order in an attempt to resolve 95% of the dependency problems, but it could never solve a circular dependency. I was thinking that with: (a) The creation of an ALTER FUNCTION name(args) SET ... and (b) Allow for functions to be created like: CREATE FUNCTION foo(int) RETURNS int AS NULL; which would return NULL as a result. A complex schema with views based upon functions, tables, and other views, and functions based upon views could be properly dumped by dumping: 1. Function Prototypes (CREATE FUNCTION ... AS NULL)2. Types3. Aggregates4. Operators5. Sequences6. Tables ...DATA... 7. Triggers8. Function Implementations (ALTER FUNCTION ... SET)9. Rules (including Views) 10. Indexes 11. Comments :-) Wouldn't this be a "correct" dump? Mike Mascari
Mike Mascari wrote: > > Tom Lane wrote: > > > > Jan Wieck <janwieck@Yahoo.com> writes: > > > From memory I think views are created as CREATE TABLE, with > > > an internal DefineRuleStmt, and dumped as CREATE TABLE, > > > CREATE RULE for sure. So the CREATE/DROP RULE would need to > > > remove/recreate the tables file (plus toast file and index) > > > if you want it to be consistent. Don't think you want that - > > > do you? > > > > But that's only true because it's such a pain in the neck for pg_dump > > to discover that a table is a view. If this could easily be told from > > inspection of pg_class, then it'd be no problem to dump views as > > CREATE VIEW statements in the first place --- obviously better, no? > > The fact that views can be created by a separate table/rule > sequence allows pg_dump to properly dump views which are based > upon functions, or views which may have dependencies on other > tables/views. I don't see this. a 'CREATE VIEW' cannot reference a function that did not exist at the time it was executed. The only way to get in trouble, that I see, is a DROP/CREATE RULE. But I think the proposal is not to allow this to happen if the rule is the select rule for a view. The reason that pg_dump used the table/rule sequence was that historically it was hard to figure out that a tuple in pg_class really represented a view. But I could be mistaken. -- Mark Hollomon mhh@nortelnetworks.com ESN 451-9008 (302)454-9008
Some idiot wrote: > > The fact that views can be created by a separate table/rule > sequence allows pg_dump to properly dump views which are based > upon functions, or views which may have dependencies on other > tables/views. The new pg_dump dumps in oid order in an attempt to > resolve 95% of the dependency problems, but it could never solve > a circular dependency. I was thinking that with: > > (a) The creation of an ALTER FUNCTION name(args) SET ... > > and > > (b) Allow for functions to be created like: > > CREATE FUNCTION foo(int) RETURNS int AS NULL; > > which would return NULL as a result. > > A complex schema with views based upon functions, tables, and > other views, and functions based upon views could be properly > dumped by dumping: > > 1. Function Prototypes (CREATE FUNCTION ... AS NULL) > 2. Types > 3. Aggregates > 4. Operators > 5. Sequences > 6. Tables > > ...more idiocy follows... Sorry. I forgot about function prototypes with arguments of user-defined types. Seems there's no magic bullet. :-( Mike Mascari
"Hollomon, Mark" wrote: > > Do we still want to be able to inherit from views? Also: Currently a view may be dropped with either 'DROP VIEW' or 'DROP TABLE'. Should this be changed? -- Mark Hollomon mhh@nortelnetworks.com ESN 451-9008 (302)454-9008
Mark Hollomon wrote: > Mike Mascari wrote: > > > > Tom Lane wrote: > > > > > > Jan Wieck <janwieck@Yahoo.com> writes: > > > > From memory I think views are created as CREATE TABLE, with > > > > an internal DefineRuleStmt, and dumped as CREATE TABLE, > > > > CREATE RULE for sure. So the CREATE/DROP RULE would need to > > > > remove/recreate the tables file (plus toast file and index) > > > > if you want it to be consistent. Don't think you want that - > > > > do you? > > > > > > But that's only true because it's such a pain in the neck for pg_dump > > > to discover that a table is a view. If this could easily be told from > > > inspection of pg_class, then it'd be no problem to dump views as > > > CREATE VIEW statements in the first place --- obviously better, no? > > > > The fact that views can be created by a separate table/rule > > sequence allows pg_dump to properly dump views which are based > > upon functions, or views which may have dependencies on other > > tables/views. > > I don't see this. a 'CREATE VIEW' cannot reference a function that > did not exist at the time it was executed. The only way to get in > trouble, that I see, is a DROP/CREATE RULE. But I think > the proposal is not to allow this to happen if the rule is the > select rule for a view. > > The reason that pg_dump used the table/rule sequence was that historically > it was hard to figure out that a tuple in pg_class really represented a > view. > > But I could be mistaken. Yep, you are. The reason why we dump views as table+rule is that historically we wheren't able to dump views and rulesat all. We only store the parsetree representation of rules, since epoch. Then, someone wrote a little backend function that's able to backparse these rule actions. It got enhanced by a couple of other smart guys andgot used by pg_dump. At that time, it was right to dump views as table+rule, because pg_dump didn't do anythingin OID order. So views using sql functions using views in turn wouldn't be dumpable otherwise. And it was easiertoo because it was already done after dumping rules at the end. No need to do anything else for views:-) So far about history, now the future. Dumping views as CREATE VIEW is cleaner. It is possible now, since we dump the objects in OID order. So I like it. I see no problem with Tom's solution, changing the relkind and removing the files at CREATE RULE time for a couple of releases. And yes, dropping the SELECT rule from a view must be forbidden. As defining triggers,constraints and the like for them should be. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Mike Mascari <mascarm@mascari.com> writes:
>> 1. Function Prototypes (CREATE FUNCTION ... AS NULL)
>> 2. Types
> Sorry. I forgot about function prototypes with arguments of
> user-defined types. Seems there's no magic bullet. :-(
Not necessarily --- there's a shell-type (or type forward reference,
if you prefer) feature that exists to handle exactly that apparent
circularity. Otherwise you could never define a user-defined type at
all, since you have to define its I/O procedures before you can do
CREATE TYPE.
I didn't study your proposal in detail, but it might work.
regards, tom lane
Tom Lane wrote: > > >> The right way IMHO would be to give views another relkind. > > > This appeals to me. > > I like it too. Aside from the advantages Jan mentioned, we could also > refrain from creating an underlying file for a view, which would be > nice to avoid cluttering the database directory. Excellent. I think we have a consensus. I'll start coding in that direction. Anybody have any thoughts on the upgrade ramification of this change? -- Mark Hollomon mhh@nortelnetworks.com ESN 451-9008 (302)454-9008
Jan Wieck wrote: > > Mark Hollomon wrote: > > But I could be mistaken. > > Yep, you are. D'oh. > So far about history, now the future. > > Dumping views as CREATE VIEW is cleaner. It is possible now, > since we dump the objects in OID order. So I like it. I see > no problem with Tom's solution, changing the relkind and > removing the files at CREATE RULE time for a couple of > releases. And yes, dropping the SELECT rule from a view must > be forbidden. As defining triggers, constraints and the like > for them should be. Alright. To recap. 1. CREATE VIEW sets relkind to RELKIND_VIEW 2. CREATE RULE ... AS ON SELECT DO INSTEAD ... sets relkind to RELKIND_VIEWand deletes any relation files. q: If we find an index, should we drop it, or complain, or ignore it? q: Should the code check to see if the relation isempty (no valid tuples)? 3. DROP RULE complains if dropping the select rule for a view. 4. ALTER TABLE complains if run against a view. Anything else? -- Mark Hollomon mhh@nortelnetworks.com ESN 451-9008 (302)454-9008
"Mark Hollomon" <mhh@nortelnetworks.com> writes:
> 2. CREATE RULE ... AS ON SELECT DO INSTEAD ... sets relkind to RELKIND_VIEW
> and deletes any relation files.
> q: If we find an index, should we drop it, or complain, or ignore it?
> q: Should the code check to see if the relation is empty (no valid tuples)?
I think we can ignore indexes. However, it seems like a wise move to
refuse to convert a nonempty table to view status, *especially* if we
are going to blow away the physical file. Otherwise mistyping the
relation name in a CREATE RULE could be disastrous (what? you wanted
that data?)
regards, tom lane
Applied. Thanks. > This patch is for the TODO item > > * Disallow LOCK on view > > src/backend/commands/command.c is the only affected file > > -- > Mark Hollomon > mhh@mindspring.com [ Attachment, skipping... ] -- 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
> "Hollomon, Mark" wrote: > > > > Do we still want to be able to inherit from views? > > Also: > > Currently a view may be dropped with either 'DROP VIEW' > or 'DROP TABLE'. Should this be changed? I say let them drop it with either one. -- 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
On Mon, 16 Oct 2000, Bruce Momjian wrote: > > "Hollomon, Mark" wrote: > > > > > > Do we still want to be able to inherit from views? > > > > Also: > > > > Currently a view may be dropped with either 'DROP VIEW' > > or 'DROP TABLE'. Should this be changed? > > I say let them drop it with either one. I kinda like the 'drop index with drop index', 'drop table with drop table' and 'drop view with drop view' groupings ... at least you are pretty sure you haven't 'oopsed' in the process :)
> On Mon, 16 Oct 2000, Bruce Momjian wrote: > > > > "Hollomon, Mark" wrote: > > > > > > > > Do we still want to be able to inherit from views? > > > > > > Also: > > > > > > Currently a view may be dropped with either 'DROP VIEW' > > > or 'DROP TABLE'. Should this be changed? > > > > I say let them drop it with either one. > > I kinda like the 'drop index with drop index', 'drop table with drop > table' and 'drop view with drop view' groupings ... at least you are > pretty sure you haven't 'oopsed' in the process :) Good point. Oops is bad. -- 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
On Mon, Oct 16, 2000 at 08:41:43PM -0300, The Hermit Hacker wrote: > On Mon, 16 Oct 2000, Bruce Momjian wrote: > > > > "Hollomon, Mark" wrote: > > > > > > > > Do we still want to be able to inherit from views? > > > > > > Also: > > > > > > Currently a view may be dropped with either 'DROP VIEW' > > > or 'DROP TABLE'. Should this be changed? > > > > I say let them drop it with either one. > > I kinda like the 'drop index with drop index', 'drop table with drop > table' and 'drop view with drop view' groupings ... at least you are > pretty sure you haven't 'oopsed' in the process :) > > So the vote is now tied. Any other opinions -- Mark Hollomon mhh@mindspring.com
Mark Hollomon <mhh@mindspring.com> writes:
>>>> I say let them drop it with either one.
>>
>> I kinda like the 'drop index with drop index', 'drop table with drop
>> table' and 'drop view with drop view' groupings ... at least you are
>> pretty sure you haven't 'oopsed' in the process :)
> So the vote is now tied. Any other opinions
I vote for the fascist approach (command must agree with actual type
of object). Seems safest. Please make sure the error message is
helpful though, like "Use DROP SEQUENCE to drop a sequence".
regards, tom lane
On Monday 16 October 2000 20:56, Tom Lane wrote: > Mark Hollomon <mhh@mindspring.com> writes: > >>>> I say let them drop it with either one. > >> > >> I kinda like the 'drop index with drop index', 'drop table with drop > >> table' and 'drop view with drop view' groupings ... at least you are > >> pretty sure you haven't 'oopsed' in the process :) > > > > So the vote is now tied. Any other opinions > > I vote for the fascist approach (command must agree with actual type > of object). Seems safest. Please make sure the error message is > helpful though, like "Use DROP SEQUENCE to drop a sequence". > Since Bruce changed his vote, it is now 3 to 0 for fascism. I'll see what I can do. -- Mark Hollomon