Thread: Materialized view assertion failure in HEAD

I'm getting an assertion failure in HEAD with materialized views, see
below for backtrace.

To reproduce, just run make installcheck, dump the regression database
and then restore it, the server crashes during restore.

(gdb) bt
#0  0x00007f283a366425 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007f283a369b8b in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x0000000000888429 in ExceptionalCondition (   conditionName=0xa0c840 "!(!matviewRel->rd_rel->relhasoids)",
errorType=0xa0c78a"FailedAssertion", fileName=0xa0c780 "matview.c",   lineNumber=135) at assert.c:54
 
#3  0x00000000005dbfc4 in ExecRefreshMatView (stmt=0x1b70a28,   queryString=0x1b6ff98 "REFRESH MATERIALIZED VIEW
tvmm;\n\n\n",params=0x0,   completionTag=0x7fff47af0a60 "") at matview.c:135
 
#4  0x00000000007758a5 in standard_ProcessUtility (parsetree=0x1b70a28,   queryString=0x1b6ff98 "REFRESH MATERIALIZED
VIEWtvmm;\n\n\n", params=0x0,   dest=0x1b70da0, completionTag=0x7fff47af0a60 "",   context=PROCESS_UTILITY_TOPLEVEL) at
utility.c:1173
#5  0x0000000000773e3f in ProcessUtility (parsetree=0x1b70a28,   queryString=0x1b6ff98 "REFRESH MATERIALIZED VIEW
tvmm;\n\n\n",params=0x0,   dest=0x1b70da0, completionTag=0x7fff47af0a60 "",   context=PROCESS_UTILITY_TOPLEVEL) at
utility.c:341
#6  0x0000000000772d7e in PortalRunUtility (portal=0x1aeb6d8,   utilityStmt=0x1b70a28, isTopLevel=1 '\001',
dest=0x1b70da0,  completionTag=0x7fff47af0a60 "") at pquery.c:1185
 
#7  0x0000000000772f56 in PortalRunMulti (portal=0x1aeb6d8,   isTopLevel=1 '\001', dest=0x1b70da0, altdest=0x1b70da0,
completionTag=0x7fff47af0a60"") at pquery.c:1317
 
#8  0x0000000000772481 in PortalRun (portal=0x1aeb6d8,   count=9223372036854775807, isTopLevel=1 '\001',
dest=0x1b70da0,  altdest=0x1b70da0, completionTag=0x7fff47af0a60 "") at pquery.c:814
 
#9  0x000000000076c155 in exec_simple_query (   query_string=0x1b6ff98 "REFRESH MATERIALIZED VIEW tvmm;\n\n\n")   at
postgres.c:1048
#10 0x0000000000770517 in PostgresMain (argc=2, argv=0x1ab4bb0,   username=0x1ab4a88 "joe") at postgres.c:3969
#11 0x000000000070ccec in BackendRun (port=0x1ae5ac0) at postmaster.c:3989
#12 0x000000000070c401 in BackendStartup (port=0x1ae5ac0) at postmaster.c:3673
#13 0x0000000000708ce6 in ServerLoop () at postmaster.c:1575
#14 0x0000000000708420 in PostmasterMain (argc=3, argv=0x1ab2420)   at postmaster.c:1244
#15 0x00000000006704f7 in main (argc=3, argv=0x1ab2420) at main.c:197

Joachim Wieland <joe@mcknight.de> wrote:

> I'm getting an assertion failure in HEAD with materialized views, see
> below for backtrace.
>
> To reproduce, just run make installcheck, dump the regression database
> and then restore it, the server crashes during restore.

> #2  0x0000000000888429 in ExceptionalCondition (
>   conditionName=0xa0c840 "!(!matviewRel->rd_rel->relhasoids)",
>   errorType=0xa0c78a "FailedAssertion", fileName=0xa0c780 "matview.c",
>   lineNumber=135) at assert.c:54

Will investigate.

You don't have default_with_oids = on, do you?

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

On Tue, Mar 5, 2013 at 9:11 AM, Kevin Grittner <kgrittn@ymail.com> wrote:
> Will investigate.
> You don't have default_with_oids = on, do you?

No, this was a default install with a default postgresql.conf

Joachim Wieland <joe@mcknight.de> wrote:

> I'm getting an assertion failure in HEAD with materialized views

> To reproduce, just run make installcheck, dump the regression
> database and then restore it, the server crashes during restore.

I failed to touch everything necessary to prevent MVs from having
OIDs.  This patch fixes the reported problem, and doesn't leave any
gaps as far as I know; but I will do additional review to try to
catch any other omissions.  I figured I should address the reported
problem now, though.

Will push later today if there are no objections.

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

Kevin Grittner <kgrittn@ymail.com> writes:
> I failed to touch everything necessary to prevent MVs from having
> OIDs.� This patch fixes the reported problem, and doesn't leave any
> gaps as far as I know; but I will do additional review to try to
> catch any other omissions.� I figured I should address the reported
> problem now, though.

> Will push later today if there are no objections.

I object --- that's not a fix, that's a crude hack.  It should not be
necessary to introduce relkind tests there.  Determination of whether
OIDs exist in the target table should happen well upstream, ie in
whatever is constructing the intoClause.  Otherwise we'll be fixing
code that examines the intoClause until doomsday.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@ymail.com> writes:
>> I failed to touch everything necessary to prevent MVs from
>> having OIDs.  This patch fixes the reported problem, and doesn't
>> leave any gaps as far as I know; but I will do additional review
>> to try to catch any other omissions.  I figured I should address
>> the reported problem now, though.
>
>> Will push later today if there are no objections.
>
> I object --- that's not a fix, that's a crude hack.  It should
> not be necessary to introduce relkind tests there.  Determination
> of whether OIDs exist in the target table should happen well
> upstream, ie in whatever is constructing the intoClause.
> Otherwise we'll be fixing code that examines the intoClause until
> doomsday.

OK.  I started doing it that way, but saw how much more code was
changed than this way and gave in to an impulse to do a minimal
change.  I really need to resist that impulse more....

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

Kevin Grittner <kgrittn@ymail.com> wrote:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Kevin Grittner <kgrittn@ymail.com> writes:
>>> I failed to touch everything necessary to prevent MVs from
>>> having OIDs.  This patch fixes the reported problem, and
>>> doesn't leave any gaps as far as I know; but I will do
>>> additional review to try to catch any other omissions.  I
>>> figured I should address the reported problem now, though.
>>
>>> Will push later today if there are no objections.
>>
>> I object --- that's not a fix, that's a crude hack.  It should
>> not be necessary to introduce relkind tests there.
>> Determination of whether OIDs exist in the target table should
>> happen well upstream, ie in whatever is constructing the
>> intoClause.  Otherwise we'll be fixing code that examines the
>> intoClause until doomsday.
>
> OK.  I started doing it that way, but saw how much more code was
> changed than this way and gave in to an impulse to do a minimal
> change.  I really need to resist that impulse more....

The presence of default_with_oids and the special-handling of the
oids option via interpretOidsOption() makes it hard to come up with
a solution which would qualify as "elegant".  Here's a rough cut at
an approach which seems best to me.  If this sits well with others
I'll add comments and think about that error message some more.
I'm not entirely sure I like accepting WITH (oids = false) but
throwing an error on WITH (oids = true), but it seems marginally
better than rejecting both.

Comments?

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

Kevin Grittner <kgrittn@ymail.com> writes:
> Kevin Grittner <kgrittn@ymail.com> wrote:
> The presence of default_with_oids and the special-handling of the
> oids option via interpretOidsOption() makes it hard to come up with
> a solution which would qualify as "elegant".� Here's a rough cut at
> an approach which seems best to me.� If this sits well with others
> I'll add comments and think about that error message some more. 

This seems even grottier than the other way.  I was expecting that it
should be taken care of during parse analysis; the grammar doesn't
have much more business than the executor dealing with this issue.

Let me think about it and see if I can propose a better fix.

> I'm not entirely sure I like accepting WITH (oids = false) but
> throwing an error on WITH (oids = true), but it seems marginally
> better than rejecting both.

Hm --- we'd need to deal with that issue regardless of just where in the
code it's going to happen.  I think we definitely need to reject
WITH (oids = true), if that's not to be supported, but have less of
an opinion about the other.

BTW, is there a really solid reason why a matview couldn't be allowed to
have OIDs on demand, and thereby dodge this whole problem?  I'm thinking
that the analogy to regular views not having OIDs is not a very good
argument, because certainly matview rows are going to need all the other
system columns.

[ wanders away wondering why IntoClause has grown a relkind field... ]
        regards, tom lane

Tom Lane wrote:

> [ wanders away wondering why IntoClause has grown a relkind field... ]

See transformCreateTableAsStmt, SetupForCreateTableAs, intorel_startup.
gram.y uses the convention of assigning '\0' to it initially and parse
analysis fills in the right value.  I wonder if a boolean field would
do.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

I wrote:
> BTW, is there a really solid reason why a matview couldn't be allowed to
> have OIDs on demand, and thereby dodge this whole problem?  I'm thinking
> that the analogy to regular views not having OIDs is not a very good
> argument, because certainly matview rows are going to need all the other
> system columns.

Some experimentation says that actually it works just fine to create
matviews with OIDs; the only part of the backend that has a problem with
that is REFRESH MATERIALIZED VIEW, which is easily fixed as per
attached.

If we go in this direction it would probably also be a good idea to
revert the hack in psql/describe.c to prevent it from printing Has OIDs
for matviews.

Also, the fact that Joachim saw this problem in a dump/reload implies
that pg_dump is failing to preserve the state of relhasoids for
matviews, because tvmm hasn't got OIDs in the regression database, but
his stack trace says it did after dump and reload.  I haven't looked
into how come, but whichever way we jump as far as the backend behavior,
pg_dump is clearly confused.

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.

Also ... while I was nosing around I noticed this bit in InitPlan():

      /*
+      * Unless we are creating a view or are creating a materialized view WITH
+      * NO DATA, ensure that all referenced relations are scannable.
+      */
+     if ((eflags & EXEC_FLAG_WITH_NO_DATA) == 0)
+         ExecCheckRelationsScannable(rangeTable);

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?  (Scanning the rangetable is probably wrong for this anyway
--- it'd be better to put the responsibility into the table-scan-node
initialization functions.)

            regards, tom lane

diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index e20fedaeaf77772a47b34f423ddeb062b429bf1f..dbcbf332539b00516afbb28be56857a5f9c2402f 100644
*** a/src/backend/commands/matview.c
--- b/src/backend/commands/matview.c
*************** typedef struct
*** 44,55 ****
      BulkInsertState bistate;    /* bulk insert state */
  } DR_transientrel;

  static void transientrel_startup(DestReceiver *self, int operation, TupleDesc typeinfo);
  static void transientrel_receive(TupleTableSlot *slot, DestReceiver *self);
  static void transientrel_shutdown(DestReceiver *self);
  static void transientrel_destroy(DestReceiver *self);
- static void refresh_matview_datafill(DestReceiver *dest, Query *query,
-                                      const char *queryString);

  /*
   * SetRelationIsScannable
--- 44,55 ----
      BulkInsertState bistate;    /* bulk insert state */
  } DR_transientrel;

+ static void refresh_matview_datafill(DestReceiver *dest, bool hasoids,
+                                      Query *query, const char *queryString);
  static void transientrel_startup(DestReceiver *self, int operation, TupleDesc typeinfo);
  static void transientrel_receive(TupleTableSlot *slot, DestReceiver *self);
  static void transientrel_shutdown(DestReceiver *self);
  static void transientrel_destroy(DestReceiver *self);

  /*
   * SetRelationIsScannable
*************** ExecRefreshMatView(RefreshMatViewStmt *s
*** 138,145 ****
       */
      Assert(!IsSystemRelation(matviewRel));

-     Assert(!matviewRel->rd_rel->relhasoids);
-
      /*
       * Check that everything is correct for a refresh. Problems at this point
       * are internal errors, so elog is sufficient.
--- 138,143 ----
*************** ExecRefreshMatView(RefreshMatViewStmt *s
*** 185,198 ****

      tableSpace = matviewRel->rd_rel->reltablespace;

-     heap_close(matviewRel, NoLock);
-
      /* Create the transient table that will receive the regenerated data. */
      OIDNewHeap = make_new_heap(matviewOid, tableSpace);
      dest = CreateTransientRelDestReceiver(OIDNewHeap);

      if (!stmt->skipData)
!         refresh_matview_datafill(dest, dataQuery, queryString);

      /*
       * Swap the physical files of the target and transient tables, then
--- 183,197 ----

      tableSpace = matviewRel->rd_rel->reltablespace;

      /* Create the transient table that will receive the regenerated data. */
      OIDNewHeap = make_new_heap(matviewOid, tableSpace);
      dest = CreateTransientRelDestReceiver(OIDNewHeap);

      if (!stmt->skipData)
!         refresh_matview_datafill(dest, matviewRel->rd_rel->relhasoids,
!                                  dataQuery, queryString);
!
!     heap_close(matviewRel, NoLock);

      /*
       * Swap the physical files of the target and transient tables, then
*************** ExecRefreshMatView(RefreshMatViewStmt *s
*** 208,215 ****
   * refresh_matview_datafill
   */
  static void
! refresh_matview_datafill(DestReceiver *dest, Query *query,
!                          const char *queryString)
  {
      List       *rewritten;
      PlannedStmt *plan;
--- 207,214 ----
   * refresh_matview_datafill
   */
  static void
! refresh_matview_datafill(DestReceiver *dest, bool hasoids,
!                          Query *query, const char *queryString)
  {
      List       *rewritten;
      PlannedStmt *plan;
*************** refresh_matview_datafill(DestReceiver *d
*** 217,222 ****
--- 216,222 ----
      List       *rtable;
      RangeTblEntry    *initial_rte;
      RangeTblEntry    *second_rte;
+     int            eflags;

      rewritten = QueryRewrite((Query *) copyObject(query));

*************** refresh_matview_datafill(DestReceiver *d
*** 265,272 ****
                                  GetActiveSnapshot(), InvalidSnapshot,
                                  dest, NULL, 0);

      /* call ExecutorStart to prepare the plan for execution */
!     ExecutorStart(queryDesc, EXEC_FLAG_WITHOUT_OIDS);

      /* run the plan */
      ExecutorRun(queryDesc, ForwardScanDirection, 0L);
--- 265,281 ----
                                  GetActiveSnapshot(), InvalidSnapshot,
                                  dest, NULL, 0);

+     /*
+      * Tell the executor whether to produce OIDs or not (we need this hack
+      * because the executor doesn't see the matview as a target relation).
+      */
+     if (hasoids)
+         eflags = EXEC_FLAG_WITH_OIDS;
+     else
+         eflags = EXEC_FLAG_WITHOUT_OIDS;
+
      /* call ExecutorStart to prepare the plan for execution */
!     ExecutorStart(queryDesc, eflags);

      /* run the plan */
      ExecutorRun(queryDesc, ForwardScanDirection, 0L);
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 4ce831a433523711a81f95588c839e3f4018e6ec..57235ef792bcadec111e2193a29d8a244def5c33 100644
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*************** describeOneTableDetails(const char *sche
*** 2266,2273 ****
              printTableAddFooter(&cont, buf.data);
          }

!         /* OIDs, if verbose and not a materialized view */
!         if (verbose && tableinfo.relkind != 'm')
          {
              const char *s = _("Has OIDs");

--- 2266,2273 ----
              printTableAddFooter(&cont, buf.data);
          }

!         /* OIDs, if verbose */
!         if (verbose)
          {
              const char *s = _("Has OIDs");

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:

>> BTW, is there a really solid reason why a matview couldn't be
>> allowed to have OIDs on demand, and thereby dodge this whole
>> problem? I'm thinking that the analogy to regular views not
>> having OIDs is not a very good argument, because certainly
>> matview rows are going to need all the other system columns.
>
> Some experimentation says that actually it works just fine to
> create matviews with OIDs; the only part of the backend that has
> a problem with that is REFRESH MATERIALIZED VIEW, which is easily
> fixed as per attached.

Sure that works.  Initial versions of the patch did it that way,
and the paint is not as dry in the "no OIDS" area because it is a
recent change to the patch which I haven't (yet) tested as
thoroughly.  What happened was this: in his review Noah asked why
WITH OIDS and WITHOUT OIDS were supported, meaning (as I later
found out) that he questioned support for the *old syntax* without
the parentheses.  I misunderstood and thought he was questioning
whether OIDs were actually useful in materialized views, and came
to the conclusion that they were not.

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.

That said, there is no doubt that it would be the easiest thing to
do; but once that genie is out of the bottle, do we take it away
later, or deal with the headaches forever?  Neither seems appealing
to me.

> If we go in this direction it would probably also be a good idea
> to revert the hack in psql/describe.c to prevent it from printing
> Has OIDs for matviews.

Sure, although I fail to see what that "if" any more of a hack than
100 others in that area of the code.

> Also, the fact that Joachim saw this problem in a dump/reload
> implies that pg_dump is failing to preserve the state of
> relhasoids for matviews, because tvmm hasn't got OIDs in the
> regression database, but his stack trace says it did after dump
> and reload.  I haven't looked into how come, but whichever way we
> jump as far as the backend behavior, pg_dump is clearly confused.

Yeah, that was the bug I was fixing here.  The MV was created
between two SETs of default_with_oids based on table names with
different settings of this.  If we change back to allowing OIDs for
MVs we need to reinstate the test for whether to omit SETs for
default_with_oids for them instead of having an assert that they
don't have OIDs.

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

> Also ... while I was nosing around I noticed this bit in
> InitPlan():
>  
>       /*
> +     * Unless we are creating a view or are creating a materialized view WITH
> +     * NO DATA, ensure that all referenced relations are scannable.
> +     */
> +     if ((eflags & EXEC_FLAG_WITH_NO_DATA) == 0)
> +         ExecCheckRelationsScannable(rangeTable);
>
> 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?

It isn't, nor is it the behavior we get:

test=# create materialized view x as select 1;
SELECT 1
test=# create materialized view y as select * from x;
SELECT 1
test=# refresh materialized view x with no data;
REFRESH MATERIALIZED VIEW
test=# refresh materialized view y;
ERROR:  materialized view "x" has not been populated
HINT:  Use the REFRESH MATERIALIZED VIEW command.

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

> (Scanning the rangetable is probably wrong for this anyway
> --- it'd be better to put the responsibility into the
> table-scan-node initialization functions.)

Don't we want to lock the appropriate relations at the point where
I have this?  Would it be right to wait until the point you suggest
from the locking POV?

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

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

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

On Mon, Mar 18, 2013 at 4:42 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
> 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?

I agree that it's probably better to just disallow this, but I have to
admit I don't like your proposed patch much.  It seems to me that the
right place to fix this is in interpretOidsOption(), by returning
false rather than default_with_oids whenever the relation is a
materialized view.  That would require passing the relkind as an
additional argument to interpretOidsOption(), but that doesn't seem
problematic.

Then, the parser could just error out if OIDS=anything appears in the
options list, but it wouldn't need to actually kludge the options list
as you've done here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> wrote:
> Kevin Grittner <kgrittn@ymail.com> wrote:
>> 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?
>
> I agree that it's probably better to just disallow this, but I have to
> admit I don't like your proposed patch much.  It seems to me that the
> right place to fix this is in interpretOidsOption(), by returning
> false rather than default_with_oids whenever the relation is a
> materialized view.  That would require passing the relkind as an
> additional argument to interpretOidsOption(), but that doesn't seem
> problematic.

I like it.  It seems to be less of a modularity violation than my
suggestion, and if we're going to have kluges for oids, we might as
well keep them together rather than scattering them around.

> Then, the parser could just error out if OIDS=anything appears in the
> options list, but it wouldn't need to actually kludge the options list
> as you've done here.

Right.

Any other comments?

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

Kevin Grittner <kgrittn@ymail.com> wrote:
> Robert Haas <robertmhaas@gmail.com> wrote:

>> It seems to me that the right place to fix this is in
>> interpretOidsOption(), by returning false rather than
>> default_with_oids whenever the relation is a materialized view.

> I like it.

In working up a patch for this approach, I see that if CREATE
FOREIGN TABLE is executed with default_with_oids set to true, it
adds an oid column which appears to be always zero in my tests so
far (although maybe other FDWs support it?).  Do we want to leave
that alone?  If we're going to add code to ignore that setting for
matviews do we also want to ignore it for FDWs?

[ thinks... ]

I suppose I should post a patch which preserves the status quo for
FDWs and treat that as a separate issue.  So, rough cut attached.
Obviously some docs should be added around this, and I still need
to do another pass to make sure I didn't miss anything; but it
passes make world-check, make installworld-check, and the
regression database can be dumped and loaded without problem.

Comments?

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

Kevin Grittner <kgrittn@ymail.com> wrote:
> Kevin Grittner <kgrittn@ymail.com> wrote:
>> Robert Haas <robertmhaas@gmail.com> wrote:
>
>>> It seems to me that the right place to fix this is in
>>> interpretOidsOption(), by returning false rather than
>>> default_with_oids whenever the relation is a materialized view.
>
>> I like it.
>
> In working up a patch for this approach, I see that if CREATE
> FOREIGN TABLE is executed with default_with_oids set to true, it
> adds an oid column which appears to be always zero in my tests so
> far (although maybe other FDWs support it?).  Do we want to leave
> that alone?  If we're going to add code to ignore that setting for
> matviews do we also want to ignore it for FDWs?
>
> [ thinks... ]
>
> I suppose I should post a patch which preserves the status quo for
> FDWs and treat that as a separate issue.  So, rough cut attached.
> Obviously some docs should be added around this, and I still need
> to do another pass to make sure I didn't miss anything; but it
> passes make world-check, make installworld-check, and the
> regression database can be dumped and loaded without problem.
>
> Comments?

Tidied up, further tested, and pushed.

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

Kevin Grittner <kgrittn@ymail.com> writes:
> In working up a patch for this approach, I see that if CREATE
> FOREIGN TABLE is executed with default_with_oids set to true, it
> adds an oid column which appears to be always zero in my tests so
> far (although maybe other FDWs support it?).� Do we want to leave
> that alone?� If we're going to add code to ignore that setting for
> matviews do we also want to ignore it for FDWs?

I don't see any very good reason for the core code to be assuming
anything about whether an FDW will support OIDs or not.  It would have
been noticeably more painful to get postgres_fdw's writable-table
support working if the core code had tried to enforce an opinion that
foreign tables couldn't have ctid, to take something somewhat
comparable.

Where I'd like to see things going is to add a hook for FDWs to control
what system columns get created for their tables.  When that happens,
an FDW could throw error about OIDs if it wants to.  In the meantime
I don't feel a need to change the behavior.
        regards, tom lane