Re: Materialized view assertion failure in HEAD - Mailing list pgsql-hackers

Kevin Grittner <kgrittn@ymail.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> [ why not allow matviews to have OID columns? ]

> An oid column in a materialized view will not be significantly more
> stable than its ctid.� The same "logical" row could easily have a
> different OID on a REFRESH or even an incremental update (due to
> the probable need to handle some incremental updates by deleting
> and re-inserting portions of the MV).� Allowing an "Object ID" to
> be generated when it is not as stable as the name or its usage in a
> table might suggest seems likely to lead to all kinds of trouble
> and false bug reports down the road.

Meh.  I don't find that argument all that convincing.  It'd pass as a
rationale for not supporting OIDs if we'd found it was hard to do so;
but I don't find it strong enough to justify introducing dirty kluges to
prevent people from creating matviews with OIDs.  OIDs in regular tables
aren't a sure thing as a row identifier either.  Moreover, if we are
afraid of people expecting OIDs to act in a particular way, why are
we exposing other system columns in matviews?  ctid, xmin, etc all might
have behaviors that differ from a "pure view", or that we find it
convenient to change from release to release.

>> Anyway, I think it makes more sense to go in the direction of
>> making the case work than to introduce messy kluges to prevent
>> matviews from having OIDs.

> If you can come up with a single use case where having OIDs for
> them makes sense and is useful, rather than being a loaded foot-gun
> for users, it would go a long way toward convincing me that is the
> way to go.

Arguably, OIDs in regular tables are a loaded foot-gun already: they
aren't unique unless you go out of your way to make them so, and they
aren't wide enough to be reliably unique for modern volumes of data.
There's a reason why they're deprecated.  That being the case, I'm
entirely fine with taking the whats-the-easiest-implementation approach
to whether they can be in matviews or not.

IOW, I'd not object to prohibiting them if it were possible to do so
with a small, clean patch in an unsurprising place.  But it appears
to me that it's messier to prohibit them than to allow them.

> Granted, my first go at switching away from that didn't
> touch all the bases, but at this point it's probably at least as
> much of a change to revert to supporting OIDs as to fix the
> omissions.� I will also grant that I haven't been able to see a
> really pretty way to do it, but that is mostly because the whole
> area of handling this is full of grotty special cases which need to
> be handled already.

Quite, which is why I'm objecting to adding more special cases; and
I think that disallowing this for matviews is going to end up requiring
exactly that.  I'd rather see the code just treating matviews
identically to regular relations for OID-related purposes.


>> ISTM that suppressing this check during a matview refresh is
>> rather broken, because then it won't complain if the matview
>> reads some other matview that's in a nonscannable state.� Is that
>> really the behavior we want?

> If that is what you got out of the comment, the comment probably
> needs some work.

[ looks a bit more... ]  Indeed, the comment appears to be entirely
unrelated to reality.  I still think that implementing this check in a
rangetable scan is probably broken or at least overly complicated,
though.  The fact that a rel is in the rangetable is not evidence that
it's going to be read, so you're going to need some fragile logic to
avoid throwing undesirable errors.  Moreover,
ExecCheckRelationsScannable has added an additional relcache open/close
cycle per relation per query, which is undesirable and quite unnecessary
overhead.

It'd be worth thinking about getting rid of EXEC_FLAG_WITH_NO_DATA and
instead using EXEC_FLAG_EXPLAIN_ONLY to suppress unwanted checks during
CREATE TABLE AS WITH NO DATA.  Or at least implementing both flags in
the same places.
        regards, tom lane