Re: Dropped index on table preventing rule creation

Hi,

I don't use rules, but in a bit of experimentation on Git master, I
discovered the following behaviour:

CREATE TABLE test1 (id serial primary key, things text);
CREATE TABLE test2 (id serial primary key, things text);
ALTER TABLE test1 DROP CONSTRAINT test1_pkey;
ALTER TABLE test2 DROP CONSTRAINT test2_pkey;
CREATE RULE "_RETURN" AS ON SELECT TO test1 DO INSTEAD select * from test2;

This produces the error message: could not convert table "test1" to a
view because it has indexes

There are no indexes or primary keys on either table by the time the
rule creation statement runs. If creating the same 2 tables without
originally giving them primary keys, this rule creates successfully.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thom Brown <thom(at)linux(dot)com> writes:
> I don't use rules, but in a bit of experimentation on Git master, I
> discovered the following behaviour:

> CREATE TABLE test1 (id serial primary key, things text);
> CREATE TABLE test2 (id serial primary key, things text);
> ALTER TABLE test1 DROP CONSTRAINT test1_pkey;
> ALTER TABLE test2 DROP CONSTRAINT test2_pkey;
> CREATE RULE "_RETURN" AS ON SELECT TO test1 DO INSTEAD select * from test2;

> This produces the error message: could not convert table "test1" to a
> view because it has indexes

IIRC, this is because the check is just checking relhasindex. You
should be able to recover and create the rule if you VACUUM the table.

We could no doubt add more code to make that more transparent, but I
don't see the point. The entire exercise of converting a table to a
view is only meant to support loading of pg_dump output from versions
that are probably ten years obsolete at this point. We don't even
document that you can do the above, do we?

(IOW, rather than "fix" this I'd prefer to rip out the code altogether.
But maybe we should wait a couple more years for that.)

regards, tom lane

On Sep 10, 2011, at 11:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thom Brown <thom(at)linux(dot)com> writes:
>> I don't use rules, but in a bit of experimentation on Git master, I
>> discovered the following behaviour:
>
>> CREATE TABLE test1 (id serial primary key, things text);
>> CREATE TABLE test2 (id serial primary key, things text);
>> ALTER TABLE test1 DROP CONSTRAINT test1_pkey;
>> ALTER TABLE test2 DROP CONSTRAINT test2_pkey;
>> CREATE RULE "_RETURN" AS ON SELECT TO test1 DO INSTEAD select * from test2;
>
>> This produces the error message: could not convert table "test1" to a
>> view because it has indexes
>
> IIRC, this is because the check is just checking relhasindex. You
> should be able to recover and create the rule if you VACUUM the table.
>
> We could no doubt add more code to make that more transparent, but I
> don't see the point. The entire exercise of converting a table to a
> view is only meant to support loading of pg_dump output from versions
> that are probably ten years obsolete at this point. We don't even
> document that you can do the above, do we?
>
> (IOW, rather than "fix" this I'd prefer to rip out the code altogether.
> But maybe we should wait a couple more years for that.)

IIRC, it's not dead code. I think you can still generate such a dump if you use CREATE OR REPLACE VIEW to manufacture a pair of mutually recursive views.

Now we should probably disallow that, but we currently don't.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sep 10, 2011, at 11:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> (IOW, rather than "fix" this I'd prefer to rip out the code altogether.
>> But maybe we should wait a couple more years for that.)

> IIRC, it's not dead code. I think you can still generate such a dump if you use CREATE OR REPLACE VIEW to manufacture a pair of mutually recursive views.

Oh, yeah, I'd forgotten about that. In general that's pg_dump's
strategy for breaking a circular dependency loop that involves a view.

> Now we should probably disallow that, but we currently don't.

Losing that particular case isn't problematic, but I'm not sure that
that's the only possible circularity involving a view. One idea that
comes to mind is

create table foo (list-of-columns);

create function foofunc () returns setof foo as ...;

create rule .... as select * from foofunc();

This only saves somebody from citing the list of column types twice,
so maybe we could blow off this case too; but who's to say there are
not more-useful cases that would create circularities?

regards, tom lane

On Tue, Sep 13, 2011 at 4:31 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sep 10, 2011, at 11:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> (IOW, rather than "fix" this I'd prefer to rip out the code altogether.
>>> But maybe we should wait a couple more years for that.)
>
>> IIRC, it's not dead code. I think you can still generate such a dump if you use CREATE OR REPLACE VIEW to manufacture a pair of mutually recursive views.
>
> Oh, yeah, I'd forgotten about that.  In general that's pg_dump's
> strategy for breaking a circular dependency loop that involves a view.
>
>> Now we should probably disallow that, but we currently don't.
>
> Losing that particular case isn't problematic, but I'm not sure that
> that's the only possible circularity involving a view.  One idea that
> comes to mind is
>
>        create table foo (list-of-columns);
>
>        create function foofunc () returns setof foo as ...;
>
>        create rule .... as select * from foofunc();
>
> This only saves somebody from citing the list of column types twice,
> so maybe we could blow off this case too; but who's to say there are
> not more-useful cases that would create circularities?

I spent some more time looking at this tonight. I am wondering if
perhaps we should just get rid of relhasindex. At the time that was
added - which predates our commit history - we didn't use the relcache
to cache the results of scanning pg_index, and we scanned pg_index
using a heap scan rather than an index scan. So at that point being
able to tell from the pg_class tuple that no indexes could exist was
probably a rather large win. It's likely a lot smaller now - and,
really, how many tables don't have indexes anyway? Even so, I'm not
100% sure whether it's worth doing: the existing code isn't really
hurting us, and I think we could fix Thom's complaint by changing
DefineQueryRewrite() to call RelationGetIndexList() rather than
blindly believing relhasindex, which would be maybe a five line
code-change. We'd probably also want to change
SetRelationRuleStatus() to clear relhasindex, which would be one more
line of code.

One related thing that seems worth doing is ripping out relhaspkey,
which is used for absolutely nothing at all. It looks to me like
doing so will save four bytes per row in pg_class due to alignment
padding. Patch for that attached. (Yeah, I know... this could break
client code... but is it really worth keeping this cruft around
forever? Especially since the column can easily say there's a primary
key when there really isn't?)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I spent some more time looking at this tonight. I am wondering if
> perhaps we should just get rid of relhasindex.

-1, there is absolutely no reason to believe that's a good idea.

> ... I think we could fix Thom's complaint by changing
> DefineQueryRewrite() to call RelationGetIndexList() rather than
> blindly believing relhasindex, which would be maybe a five line
> code-change. We'd probably also want to change
> SetRelationRuleStatus() to clear relhasindex, which would be one more
> line of code.

Yeah, that's about what it would take, but what I'm asking is why
bother. The *only* case that we support here is turning a just-created,
not-fooled-with table into a view, and I don't feel a need to promise
that we will handle other cases (which are inevitably going to be poorly
tested). See for example the adjacent relhassubclass test, which has
got exactly the same issue.

> One related thing that seems worth doing is ripping out relhaspkey,

Having a hard time getting excited about that either ...

regards, tom lane

On Wed, Sep 14, 2011 at 10:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Yeah, that's about what it would take, but what I'm asking is why
> bother.  The *only* case that we support here is turning a just-created,
> not-fooled-with table into a view, and I don't feel a need to promise
> that we will handle other cases (which are inevitably going to be poorly
> tested).  See for example the adjacent relhassubclass test, which has
> got exactly the same issue.
>
>> One related thing that seems worth doing is ripping out relhaspkey,
>
> Having a hard time getting excited about that either ...

The fact that this code is poorly tested is exactly why I don't think
we should be complicating it with doodads of doubtful utility. The
existence of these Booleans causes people to use them. This probably
doesn't save any appreciable amount of performance, but it does make
it easier to write wrong code.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company