On Tue, Jun 7, 2011 at 3:53 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@gmail.com> writes: >> I found a few other holes in my previous patch as well. I think this >> plugs them all, but it's hard to be sure there aren't any other calls >> to RelationGetNumberOfBlocks() that could bomb out. > > [ squint... ] Do we need those additional tests in plancat.c? I > haven't paid attention to whether we support unlogged indexes on logged > tables, but if we do, protecting the RelationGetNumberOfBlocks() call is > the least of your worries. You ought to be fixing things so the planner > won't consider the index valid at all (cf. the indisvalid test at line > 165).
Right now, RelationNeedsWAL() is always the same for a table and for an index belonging to that table. That is, indexes on temporary tables are temporary; indees on unlogged tables are unlogged; indexes on permanent tables are permanent. But I agree that's something we'll have to deal with if and when someone implements unlogged indexes on logged tables. (Though frankly I hope someone will come up with a better name for that; else it's going to be worse than constraint_exclusion vs. exclusion constraints.) > Similarly, the change in estimate_rel_size seems to be at an > awfully low level, akin to locking the barn door after the horses are > out. What code path are you thinking will reach there on an unlogged > table? Well, it gets there; I found this out empirically. get_relation_info() calls it in two different places. Actually, I see now that the v3 patch has a few leftovers: the test in estimate_relation_size() makes the first of the two checks in get_relaton_info() redundant -- but the second hunk in get_relation_info() is needed, because there it calls RelationGetNumberOfBlocks() directly. This is why I thought it might be better to provide a version of RelationGetNumberOfBlocks() that doesn't fail if the file is missing, instead of trying to plug these holes one by one. > It might be that it'd be best just to have both the planner and executor > throwing errors on unlogged tables, rather than rejiggering pieces of > the planner to sort-of not fail on an unlogged table. Mmm, that's not a bad thought either. Although I think if we can be certain that the planner will error out, the executor checks aren't necessary. It would disallow preparing a statement and then executing it after promotion, but that doesn't seem terribly important. Any idea where to put the check? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
