Re: Drastic performance loss in assert-enabled build in HEAD

[ sorry for being slow to respond, things are crazy this week ]

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Anyway, the immediate takeaway is that this represents a horribly
>> expensive way for pg_dump to find out which matviews aren't
>> scannable. The cheap way for it to find out would be if we had a
>> boolean flag for that in pg_class. Would you review the bidding
>> as to why things were done the way they are? Because in general,
>> having to ask the kernel something is going to suck in any case,
>> so basing it on the size of the disk file doesn't seem to me to
>> be basically a good thing.

> The early versions of the patch had a boolean in pg_class to track
> this, but I got complaints from Robert and Noah (and possibly
> others?) that this got too ugly in combination with the support for
> unlogged matviews, and they suggested the current approach.

Meh. I don't think that we should allow that corner case to drive the
design like this. I would *far* rather not have unlogged matviews at
all than this boondoggle. I'm not even convinced that the existing
semantics are desirable: who's to say that having an unlogged matview
revert to empty on crash renders it invalid? If it's a summary of
underlying unlogged tables, then that's actually the right thing.
(If someone is desperately unhappy with such a behavior, why are they
insisting on it being unlogged, anyway?)

If we go with this implementation, I think we are going to be painting
ourselves into a corner that will be difficult or impossible to get out
of, because the scannability state of a matview is not just a matter of
catalog contents but of on-disk files. That will make it difficult to
impossible for pg_upgrade to cope reasonably with any future rethinking
of scannability status.

In fact, I'm going to go further and say that I do not like the entire
concept of scannability, either as to design or implementation, and
I think we should just plain rip it out. We can rethink that for 9.4
or later, but what we've got right now is a kluge, and I don't want
to find ourselves required to be bug-compatible with it forevermore.
To take just the most salient point: assuming that we've somehow
determined that some matview is too out-of-date to be usable, why is the
appropriate response to that to cause queries on it to fail altogether?
That seems like about the least useful feature that could be devised.
Why not, say, have queries fall back to expanding the view definition as
though it were a regular view?

I think matviews without any scannability control are already a pretty
useful feature, and one I'd not be ashamed to ship just like that for
9.3. But the scannability stuff is clearly going to need to be
revisited. IMO, driving a stake in the ground at the exact spot that
this stake is placed is not a good plan. If we simply remove that
aspect altogether for now, I think we'll have more room to maneuver in
future releases.

I apologize for not having griped about this earlier, but I've really
paid no attention to the matviews work up to now; there are only so
many cycles in a day.

regards, tom lane