[HACKERS] Backend-internal SPI operations
Tom Lane
> 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
Mark Hollomon
>
> 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
m...@nortelnetworks.com
ESN 451-9008 (302)454-9008
Tom Lane
> 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
Jan Wieck
> "Mark Hollomon" <m...@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 are totally
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/type description. A
function, returning a tuple or set of tuples, can return this
type or set of type as well as any other existing table/view
structure. So to create a function returning a set of tuples,
which have a structure different from any existing table,
someone creates a named structure, then the function
returning tuples of that type. These structures are just
entries in pg_class, pg_attribute and pg_type. There is no
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. #
#================================================== JanW...@Yahoo.com #
Mark Hollomon
>
> Tom Lane wrote:
> > "Mark Hollomon" <m...@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.
Tom Lane
> 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
Jan Wieck
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?
Tom Lane
> 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's
relkind to 'view'. We don't need to drop the underlying
table file, though (just leave it be, it'll go away at
next initdb).
DROP RULE of a view's on-select-instead is not allowed.
You have to drop the whole view instead.
regards, tom lane
Mike Mascari
>
> Jan Wieck <janw...@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. Types
3. Aggregates
4. Operators
5. Sequences
6. Tables
...DATA...
7. Triggers
8. Function Implementations (ALTER FUNCTION ... SET)
9. Rules (including Views)
10. Indexes
11. Comments :-)
Wouldn't this be a "correct" dump?
Mike Mascari
Mark Hollomon
>
> Tom Lane wrote:
> >
> > Jan Wieck <janw...@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.
Mike Mascari
>
> 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
> 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
>
Sorry. I forgot about function prototypes with arguments of
user-defined types. Seems there's no magic bullet. :-(
Mike Mascari
Jan Wieck
> Mike Mascari wrote:
> >
> > Tom Lane wrote:
> > >
> > > Jan Wieck <janw...@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?
> >
> > sequence allows pg_dump to properly dump views which are based
> > upon functions, or views which may have dependencies on other
> > tables/views.
>
> 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 rules at 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 and got used by pg_dump. At that
time, it was right to dump views as table+rule, because
pg_dump didn't do anything in OID order. So views using sql
functions using views in turn wouldn't be dumpable otherwise.
And it was easier too 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.
Tom Lane
>> 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
Mark Hollomon
>
> 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_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)?
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
>
> >> 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?
Tom Lane
> 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