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

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

Well, we agree on that, anyway.  :-/

>> 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.

I would really like to hear the opinions of others here. Basically,
I think Tom and I are agreeing that OIDs already require some ugly
hacks and they are not of any practical use in a materialized view,
but that there's no way to prohibit their use in MVs without
additional ugly hacks.  (Please correct me if I'm
mis-characterizing your POV.)  We're coming down on opposite sides
of the fence on whether it is worth compounding the source code
hackery to keep this away from MV users who will probably
occasionally (mis)use it and then tell us we have a bug.  The most
common form of such a "bug" report would probably be that they used
the oid to identify a row, and due to a REFRESH or due to updates
to an underlying table the same "logical" row in the MV suddenly
has a new oid and their link is broken.  To dance around the
existing hacks requires that the new hacks violate modularity to
some degree.  I've come around to the POV that the least invasive
and least fragile form of hack for this is to essentially mock up a
"WITH (oids = false)" at CREATE MATERIALIZED VIEW parse time and
let that ripple through the rest of the layers.

>>> 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'll wait until the other issues are sorted to fix that.

> 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 sounds like I missed something there, but I'm not immediately
clear on how best to deal with that.  Pointers or suggestions
welcome.  Actually, it might be enough if you can point me at a
good description of the rangetable abstraction and how it's meant
to be used.  I haven't seen anything like that.  I've done my best
to reverse-engineer the intent from source code, but I apparently
haven't entirely hit the mark.

> 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.

Will look that over.  Is this intended to address the issues you
mention in the previous paragraph?

I want to give everyone else a chance to weigh in before I start
the pendulum swinging back in the other direction on OIDs in MVs.
Opinions?

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company