Re: BUG #6041: Unlogged table was created bad in slave node

On Tue, Jun 7, 2011 at 3:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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