Обсуждение: BUG #13907: Restore materialized view throw permission denied

Поиск
Список
Период
Сортировка

BUG #13907: Restore materialized view throw permission denied

От
marian.krucina@gmail.com
Дата:
The following bug has been logged on the website:

Bug reference:      13907
Logged by:          Marian Krucina
Email address:      marian.krucina@gmail.com
PostgreSQL version: 9.5.0
Operating system:   Centos
Description:

Hi,

restore (9.4.5, 9.5.0) or pg_upgrade (9.4.5 to 9.5.0) fail on CREATE
MATERIALIZED VIEW.
This is similar to:
http://www.postgresql.org/message-id/11166.1424357659@sss.pgh.pa.us

Problem is, when view runs as user definer.
Is possible move 'CREATE MATERIALIZED VIEW' in a dump to end?

Scenario:

CREATE ROLE role1;
CREATE ROLE role2;
CREATE TABLE table1(i INT);
CREATE VIEW view1 AS SELECT * FROM table1;
ALTER TABLE table1 OWNER TO role1;
ALTER VIEW view1 OWNER TO role2;
GRANT SELECT ON table1 TO role2;
CREATE MATERIALIZED VIEW view2 AS SELECT * FROM view1;
ALTER MATERIALIZED VIEW view2 OWNER TO role2;

# pg_dump -U postgres test -f test.sql
# psql -U postgres test2 -f test.sql -1 -e
...
CREATE MATERIALIZED VIEW view2 AS
 SELECT view1.i
   FROM view1
  WITH NO DATA;
psql:test.sql:221: ERROR:  permission denied for relation table1

Re: BUG #13907: Restore materialized view throw permission denied

От
Michael Paquier
Дата:
On Tue, Feb 2, 2016 at 7:14 PM,  <marian.krucina@gmail.com> wrote:
> restore (9.4.5, 9.5.0) or pg_upgrade (9.4.5 to 9.5.0) fail on CREATE
> MATERIALIZED VIEW.
> This is similar to:
> http://www.postgresql.org/message-id/11166.1424357659@sss.pgh.pa.us
>
> Problem is, when view runs as user definer.
> Is possible move 'CREATE MATERIALIZED VIEW' in a dump to end?

This is one of those issues where it would be cool to only plan and
execute the query creating the materialized view query with NO DATA
without planning and executing it first, and do the actual planning
and execution at the first refresh. A similar problem was discussed
here:

https://www.postgresql.org/message-id/flat/20160115175546.2968.6033%40wrigleys.postgresql.org#20160115175546.2968.6033@wrigleys.postgresql.org

Thought I don't think that it is that straight-forward, the relation
defined using a CTAS uses column information directly derived from the
query planned, so the fix would be to extract the necessary
information for those columns: collation, column name, type OID and
typemod without the need of the existing routines.
--
Michael

Re: BUG #13907: Restore materialized view throw permission denied

От
Tom Lane
Дата:
Michael Paquier <michael.paquier@gmail.com> writes:
> This is one of those issues where it would be cool to only plan and
> execute the query creating the materialized view query with NO DATA
> without planning and executing it first, and do the actual planning
> and execution at the first refresh. A similar problem was discussed
> here:
>
https://www.postgresql.org/message-id/flat/20160115175546.2968.6033%40wrigleys.postgresql.org#20160115175546.2968.6033@wrigleys.postgresql.org

> Thought I don't think that it is that straight-forward, the relation
> defined using a CTAS uses column information directly derived from the
> query planned, so the fix would be to extract the necessary
> information for those columns: collation, column name, type OID and
> typemod without the need of the existing routines.

I think you want to be looking at the way that CREATE VIEW accomplishes
the same task.  See DefineView and DefineVirtualRelation.  It might be
worth refactoring those a bit to allow code sharing.

            regards, tom lane

Re: BUG #13907: Restore materialized view throw permission denied

От
Michael Paquier
Дата:
On Tue, Jun 14, 2016 at 10:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think you want to be looking at the way that CREATE VIEW accomplishes
> the same task.  See DefineView and DefineVirtualRelation.  It might be
> worth refactoring those a bit to allow code sharing.

Ah, right! I forgot that views already solve this problem in its way,
and that's definitely something that we should do.

[code lookup]

OK I see now how to wrap that. I'll come up with something that can be
back-patched and think about the refactoring carefully.
--
Michael

Re: BUG #13907: Restore materialized view throw permission denied

От
Michael Paquier
Дата:
On Wed, Jun 15, 2016 at 4:37 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Jun 14, 2016 at 10:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think you want to be looking at the way that CREATE VIEW accomplishes
>> the same task.  See DefineView and DefineVirtualRelation.  It might be
>> worth refactoring those a bit to allow code sharing.
>
> Ah, right! I forgot that views already solve this problem in its way,
> and that's definitely something that we should do.
>
> [code lookup]
>
> OK I see now how to wrap that. I'll come up with something that can be
> back-patched and think about the refactoring carefully.

So, I have been able to build the attached WIP patch proving that this
is able to work correctly. There is no real refactoring done yet, but
this passes regression tests and tutti-quanti. By the way, there are
three points I am wondering about:
1) EXPLAIN ANALYZE is able to work with CTAS and create matview. I am
thinking that it would be better not to touch those to not impact
existing applications. By that I mean that EXPLAIN CREATE MATVIEW WITH
NO DATA would still run the planner and executor in explain.c
2) CTAS has a WITH NO DATA option, and it would be really weird to use
the planner/executor code path when this option is used for this case.
So I'd like to use the same method for both matviews and normal
relations to simplify things and make the code more consistent.
3) In this WIP patch, the command tag is CREATE MATERIALIZED VIEW if
WITH NO DATA is used. I am planning to use SELECT 0 in all cases to
keep things consistent with what is on HEAD and back-branches.

Any objections to those points? If there are none, I'll move ahead
with a more intelligent and refactored patch.
--
Michael

Вложения

Re: BUG #13907: Restore materialized view throw permission denied

От
Tom Lane
Дата:
Michael Paquier <michael.paquier@gmail.com> writes:
> So, I have been able to build the attached WIP patch proving that this
> is able to work correctly. There is no real refactoring done yet, but
> this passes regression tests and tutti-quanti. By the way, there are
> three points I am wondering about:

> 1) EXPLAIN ANALYZE is able to work with CTAS and create matview. I am
> thinking that it would be better not to touch those to not impact
> existing applications. By that I mean that EXPLAIN CREATE MATVIEW WITH
> NO DATA would still run the planner and executor in explain.c

Agreed, that needs to not break.

> 2) CTAS has a WITH NO DATA option, and it would be really weird to use
> the planner/executor code path when this option is used for this case.
> So I'd like to use the same method for both matviews and normal
> relations to simplify things and make the code more consistent.

Seems reasonable, depending on how invasive you have to be.

> 3) In this WIP patch, the command tag is CREATE MATERIALIZED VIEW if
> WITH NO DATA is used. I am planning to use SELECT 0 in all cases to
> keep things consistent with what is on HEAD and back-branches.

Meh, can't get excited about that.  If it's easy, okay, but arguably
the current behavior is just an implementation artifact itself.
I wouldn't go far out of your way to keep it.

            regards, tom lane

Re: BUG #13907: Restore materialized view throw permission denied

От
Michael Paquier
Дата:
On Fri, Jun 17, 2016 at 1:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> So, I have been able to build the attached WIP patch proving that this
>> is able to work correctly. There is no real refactoring done yet, but
>> this passes regression tests and tutti-quanti. By the way, there are
>> three points I am wondering about:
>
>> 1) EXPLAIN ANALYZE is able to work with CTAS and create matview. I am
>> thinking that it would be better not to touch those to not impact
>> existing applications. By that I mean that EXPLAIN CREATE MATVIEW WITH
>> NO DATA would still run the planner and executor in explain.c
>
> Agreed, that needs to not break.

So, left untouched.

>> 2) CTAS has a WITH NO DATA option, and it would be really weird to use
>> the planner/executor code path when this option is used for this case.
>> So I'd like to use the same method for both matviews and normal
>> relations to simplify things and make the code more consistent.
>
> Seems reasonable, depending on how invasive you have to be.

Check. It is actually not that invasive. At least I have found that
this is what this code should do naturally.

>> 3) In this WIP patch, the command tag is CREATE MATERIALIZED VIEW if
>> WITH NO DATA is used. I am planning to use SELECT 0 in all cases to
>> keep things consistent with what is on HEAD and back-branches.
>
> Meh, can't get excited about that.  If it's easy, okay, but arguably
> the current behavior is just an implementation artifact itself.
> I wouldn't go far out of your way to keep it.

Okay, I just suggested that because I thought people would care about
it. A couple of years back when rewriting CTAS on a fork of Postgres I
got complains from users regarding such a change because that was not
consistent :) Not doing it makes the code more simple and readable, so
let's go with the normal command tags then.

Attached is a new patch, with the promised refactoring, more
regression tests, etc. After some thoughts, I have arrived to the
conclusion that it is better to limit the footprint of this patch in
views.c. So I have created a routine makeColumnDef that is used for
views, ctas and matviews, but I am letting the creation of the column
definition list separated as each code path has slight differences
when building it. Things could be made more shared on HEAD but that
would be really intrusive for back branches, and I have kept that in
mind for this patch.

Comments?
--
Michael

Вложения

Re: BUG #13907: Restore materialized view throw permission denied

От
Michael Paquier
Дата:
On Fri, Jun 17, 2016 at 2:59 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Attached is a new patch, with the promised refactoring, more
> regression tests, etc. After some thoughts, I have arrived to the
> conclusion that it is better to limit the footprint of this patch in
> views.c. So I have created a routine makeColumnDef that is used for
> views, ctas and matviews, but I am letting the creation of the column
> definition list separated as each code path has slight differences
> when building it. Things could be made more shared on HEAD but that
> would be really intrusive for back branches, and I have kept that in
> mind for this patch.
>
> Comments?

Patch is added to the next commit fest. I don't want this thing to be
lost in translation:
https://commitfest.postgresql.org/10/652/
--
Michael

Re: BUG #13907: Restore materialized view throw permission denied

От
Tom Lane
Дата:
Michael Paquier <michael.paquier@gmail.com> writes:
> Attached is a new patch, with the promised refactoring, more
> regression tests, etc. After some thoughts, I have arrived to the
> conclusion that it is better to limit the footprint of this patch in
> views.c. So I have created a routine makeColumnDef that is used for
> views, ctas and matviews, but I am letting the creation of the column
> definition list separated as each code path has slight differences
> when building it. Things could be made more shared on HEAD but that
> would be really intrusive for back branches, and I have kept that in
> mind for this patch.

I'm planning to apply the attached revision as soon as I get done
back-porting it.  Main differences from your version:

* We can, and I think should, skip the rewriting phase too in the WITH NO
DATA case.  Rewriting should never change a query's exposed result
rowtype, and any other side-effects it might have are likely to be bad
for our purposes.

* Rather than add a goto, I put the existing code sequence into the if's
else block.  This will probably cause me some back-patching pain, but
I don't like uglifying code just to make back-patch simpler.

* The regression test cases you added seem not entirely on point, because
they pass just fine against HEAD.  I don't object to them, but I added
this to exercise the desired behavior change:

-- make sure that create WITH NO DATA does not plan the query (bug #13907)
create materialized view mvtest_error as select 1/0 as x;  -- fail
create materialized view mvtest_error as select 1/0 as x with no data;
refresh materialized view mvtest_error;  -- fail here
drop materialized view mvtest_error;

* I also got rid of the static variable CreateAsReladdr, which while
not related to the immediate problem is ugly and dangerous.  (It'd
cause a failure in a nested-CREATE-TABLE-AS situation, which would
be unusual but surely isn't forbidden.)


I spent a little bit of time wondering whether we couldn't get rid of
having intorel_startup create the relation at all, instead always doing
it in ExecCreateTableAs().  But that doesn't work conveniently for
CREATE TABLE AS EXECUTE, since there's no query tree handy in that case.
You could imagine moving the build-ColumnDefs-from-a-TupleDesc logic
to someplace else that's only used for CREATE TABLE AS EXECUTE, but
it's not clear to me that it's worth further refactoring just to make
the WITH DATA and WITH NO DATA cases a bit more alike.

            regards, tom lane

diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 5a853c4..5b4f6af 100644
*** a/src/backend/commands/createas.c
--- b/src/backend/commands/createas.c
***************
*** 10,16 ****
   *
   * Formerly, CTAS was implemented as a variant of SELECT, which led
   * to assorted legacy behaviors that we still try to preserve, notably that
!  * we must return a tuples-processed count in the completionTag.
   *
   * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
   * Portions Copyright (c) 1994, Regents of the University of California
--- 10,17 ----
   *
   * Formerly, CTAS was implemented as a variant of SELECT, which led
   * to assorted legacy behaviors that we still try to preserve, notably that
!  * we must return a tuples-processed count in the completionTag.  (We no
!  * longer do that for CTAS ... WITH NO DATA, however.)
   *
   * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
   * Portions Copyright (c) 1994, Regents of the University of California
***************
*** 36,41 ****
--- 37,44 ----
  #include "commands/tablecmds.h"
  #include "commands/view.h"
  #include "miscadmin.h"
+ #include "nodes/makefuncs.h"
+ #include "nodes/nodeFuncs.h"
  #include "parser/parse_clause.h"
  #include "rewrite/rewriteHandler.h"
  #include "storage/smgr.h"
*************** typedef struct
*** 53,66 ****
      IntoClause *into;            /* target relation specification */
      /* These fields are filled by intorel_startup: */
      Relation    rel;            /* relation to write to */
      CommandId    output_cid;        /* cmin to insert in output tuples */
      int            hi_options;        /* heap_insert performance options */
      BulkInsertState bistate;    /* bulk insert state */
  } DR_intorel;

! /* the address of the created table, for ExecCreateTableAs consumption */
! static ObjectAddress CreateAsReladdr = {InvalidOid, InvalidOid, 0};

  static void intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo);
  static bool intorel_receive(TupleTableSlot *slot, DestReceiver *self);
  static void intorel_shutdown(DestReceiver *self);
--- 56,72 ----
      IntoClause *into;            /* target relation specification */
      /* These fields are filled by intorel_startup: */
      Relation    rel;            /* relation to write to */
+     ObjectAddress reladdr;        /* address of rel, for ExecCreateTableAs */
      CommandId    output_cid;        /* cmin to insert in output tuples */
      int            hi_options;        /* heap_insert performance options */
      BulkInsertState bistate;    /* bulk insert state */
  } DR_intorel;

! /* utility functions for CTAS definition creation */
! static ObjectAddress create_ctas_internal(List *attrList, IntoClause *into);
! static ObjectAddress create_ctas_nodata(List *tlist, IntoClause *into);

+ /* DestReceiver routines for collecting data */
  static void intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo);
  static bool intorel_receive(TupleTableSlot *slot, DestReceiver *self);
  static void intorel_shutdown(DestReceiver *self);
*************** static void intorel_destroy(DestReceiver
*** 68,73 ****
--- 74,223 ----


  /*
+  * create_ctas_internal
+  *
+  * Internal utility used for the creation of the definition of a relation
+  * created via CREATE TABLE AS or a materialized view.  Caller needs to
+  * provide a list of attributes (ColumnDef nodes).
+  */
+ static ObjectAddress
+ create_ctas_internal(List *attrList, IntoClause *into)
+ {
+     CreateStmt *create = makeNode(CreateStmt);
+     bool        is_matview;
+     char        relkind;
+     Datum        toast_options;
+     static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
+     ObjectAddress intoRelationAddr;
+
+     /* This code supports both CREATE TABLE AS and CREATE MATERIALIZED VIEW */
+     is_matview = (into->viewQuery != NULL);
+     relkind = is_matview ? RELKIND_MATVIEW : RELKIND_RELATION;
+
+     /*
+      * Create the target relation by faking up a CREATE TABLE parsetree and
+      * passing it to DefineRelation.
+      */
+     create->relation = into->rel;
+     create->tableElts = attrList;
+     create->inhRelations = NIL;
+     create->ofTypename = NULL;
+     create->constraints = NIL;
+     create->options = into->options;
+     create->oncommit = into->onCommit;
+     create->tablespacename = into->tableSpaceName;
+     create->if_not_exists = false;
+
+     /*
+      * Create the relation.  (This will error out if there's an existing view,
+      * so we don't need more code to complain if "replace" is false.)
+      */
+     intoRelationAddr = DefineRelation(create, relkind, InvalidOid, NULL);
+
+     /*
+      * If necessary, create a TOAST table for the target table.  Note that
+      * NewRelationCreateToastTable ends with CommandCounterIncrement(), so
+      * that the TOAST table will be visible for insertion.
+      */
+     CommandCounterIncrement();
+
+     /* parse and validate reloptions for the toast table */
+     toast_options = transformRelOptions((Datum) 0,
+                                         create->options,
+                                         "toast",
+                                         validnsps,
+                                         true, false);
+
+     (void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true);
+
+     NewRelationCreateToastTable(intoRelationAddr.objectId, toast_options);
+
+     /* Create the "view" part of a materialized view. */
+     if (is_matview)
+     {
+         /* StoreViewQuery scribbles on tree, so make a copy */
+         Query       *query = (Query *) copyObject(into->viewQuery);
+
+         StoreViewQuery(intoRelationAddr.objectId, query, false);
+         CommandCounterIncrement();
+     }
+
+     return intoRelationAddr;
+ }
+
+
+ /*
+  * create_ctas_nodata
+  *
+  * Create CTAS or materialized view when WITH NO DATA is used, starting from
+  * the targetlist of the SELECT or view definition.
+  */
+ static ObjectAddress
+ create_ctas_nodata(List *tlist, IntoClause *into)
+ {
+     List       *attrList;
+     ListCell   *t,
+                *lc;
+
+     /*
+      * Build list of ColumnDefs from non-junk elements of the tlist.  If a
+      * column name list was specified in CREATE TABLE AS, override the column
+      * names in the query.  (Too few column names are OK, too many are not.)
+      */
+     attrList = NIL;
+     lc = list_head(into->colNames);
+     foreach(t, tlist)
+     {
+         TargetEntry *tle = (TargetEntry *) lfirst(t);
+
+         if (!tle->resjunk)
+         {
+             ColumnDef  *col;
+             char       *colname;
+
+             if (lc)
+             {
+                 colname = strVal(lfirst(lc));
+                 lc = lnext(lc);
+             }
+             else
+                 colname = tle->resname;
+
+             col = makeColumnDef(colname,
+                                 exprType((Node *) tle->expr),
+                                 exprTypmod((Node *) tle->expr),
+                                 exprCollation((Node *) tle->expr));
+
+             /*
+              * It's possible that the column is of a collatable type but the
+              * collation could not be resolved, so double-check.  (We must
+              * check this here because DefineRelation would adopt the type's
+              * default collation rather than complaining.)
+              */
+             if (!OidIsValid(col->collOid) &&
+                 type_is_collatable(col->typeName->typeOid))
+                 ereport(ERROR,
+                         (errcode(ERRCODE_INDETERMINATE_COLLATION),
+                          errmsg("no collation was derived for column \"%s\" with collatable type %s",
+                                 col->colname,
+                                 format_type_be(col->typeName->typeOid)),
+                          errhint("Use the COLLATE clause to set the collation explicitly.")));
+
+             attrList = lappend(attrList, col);
+         }
+     }
+
+     if (lc != NULL)
+         ereport(ERROR,
+                 (errcode(ERRCODE_SYNTAX_ERROR),
+                  errmsg("too many column names were specified")));
+
+     /* Create the relation definition using the ColumnDef list */
+     return create_ctas_internal(attrList, into);
+ }
+
+
+ /*
   * ExecCreateTableAs -- execute a CREATE TABLE AS command
   */
  ObjectAddress
*************** ExecCreateTableAs(CreateTableAsStmt *stm
*** 85,91 ****
      List       *rewritten;
      PlannedStmt *plan;
      QueryDesc  *queryDesc;
-     ScanDirection dir;

      if (stmt->if_not_exists)
      {
--- 235,240 ----
*************** ExecCreateTableAs(CreateTableAsStmt *stm
*** 121,128 ****
          Assert(!is_matview);    /* excluded by syntax */
          ExecuteQuery(estmt, into, queryString, params, dest, completionTag);

!         address = CreateAsReladdr;
!         CreateAsReladdr = InvalidObjectAddress;
          return address;
      }
      Assert(query->commandType == CMD_SELECT);
--- 270,278 ----
          Assert(!is_matview);    /* excluded by syntax */
          ExecuteQuery(estmt, into, queryString, params, dest, completionTag);

!         /* get object address that intorel_startup saved for us */
!         address = ((DR_intorel *) dest)->reladdr;
!
          return address;
      }
      Assert(query->commandType == CMD_SELECT);
*************** ExecCreateTableAs(CreateTableAsStmt *stm
*** 142,211 ****
          save_nestlevel = NewGUCNestLevel();
      }

!     /*
!      * Parse analysis was done already, but we still have to run the rule
!      * rewriter.  We do not do AcquireRewriteLocks: we assume the query either
!      * came straight from the parser, or suitable locks were acquired by
!      * plancache.c.
!      *
!      * Because the rewriter and planner tend to scribble on the input, we make
!      * a preliminary copy of the source querytree.  This prevents problems in
!      * the case that CTAS is in a portal or plpgsql function and is executed
!      * repeatedly.  (See also the same hack in EXPLAIN and PREPARE.)
!      */
!     rewritten = QueryRewrite((Query *) copyObject(query));

!     /* SELECT should never rewrite to more or less than one SELECT query */
!     if (list_length(rewritten) != 1)
!         elog(ERROR, "unexpected rewrite result for CREATE TABLE AS SELECT");
!     query = (Query *) linitial(rewritten);
!     Assert(query->commandType == CMD_SELECT);

!     /* plan the query */
!     plan = pg_plan_query(query, 0, params);

!     /*
!      * Use a snapshot with an updated command ID to ensure this query sees
!      * results of any previously executed queries.  (This could only matter if
!      * the planner executed an allegedly-stable function that changed the
!      * database contents, but let's do it anyway to be parallel to the EXPLAIN
!      * code path.)
!      */
!     PushCopiedSnapshot(GetActiveSnapshot());
!     UpdateActiveSnapshotCommandId();

!     /* Create a QueryDesc, redirecting output to our tuple receiver */
!     queryDesc = CreateQueryDesc(plan, queryString,
!                                 GetActiveSnapshot(), InvalidSnapshot,
!                                 dest, params, 0);

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

!     /*
!      * Normally, we run the plan to completion; but if skipData is specified,
!      * just do tuple receiver startup and shutdown.
!      */
!     if (into->skipData)
!         dir = NoMovementScanDirection;
!     else
!         dir = ForwardScanDirection;

!     /* run the plan */
!     ExecutorRun(queryDesc, dir, 0L);

!     /* save the rowcount if we're given a completionTag to fill */
!     if (completionTag)
!         snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
!                  "SELECT " UINT64_FORMAT, queryDesc->estate->es_processed);

!     /* and clean up */
!     ExecutorFinish(queryDesc);
!     ExecutorEnd(queryDesc);

!     FreeQueryDesc(queryDesc);

!     PopActiveSnapshot();

      if (is_matview)
      {
--- 292,372 ----
          save_nestlevel = NewGUCNestLevel();
      }

!     if (into->skipData)
!     {
!         /*
!          * If WITH NO DATA was specified, do not go through the rewriter,
!          * planner and executor.  Just define the relation using a code path
!          * similar to CREATE VIEW.  This avoids dump/restore problems stemming
!          * from running the planner before all dependencies are set up.
!          */
!         address = create_ctas_nodata(query->targetList, into);
!     }
!     else
!     {
!         /*
!          * Parse analysis was done already, but we still have to run the rule
!          * rewriter.  We do not do AcquireRewriteLocks: we assume the query
!          * either came straight from the parser, or suitable locks were
!          * acquired by plancache.c.
!          *
!          * Because the rewriter and planner tend to scribble on the input, we
!          * make a preliminary copy of the source querytree.  This prevents
!          * problems in the case that CTAS is in a portal or plpgsql function
!          * and is executed repeatedly.  (See also the same hack in EXPLAIN and
!          * PREPARE.)
!          */
!         rewritten = QueryRewrite((Query *) copyObject(query));

!         /* SELECT should never rewrite to more or less than one SELECT query */
!         if (list_length(rewritten) != 1)
!             elog(ERROR, "unexpected rewrite result for %s",
!                  is_matview ? "CREATE MATERIALIZED VIEW" :
!                  "CREATE TABLE AS SELECT");
!         query = (Query *) linitial(rewritten);
!         Assert(query->commandType == CMD_SELECT);

!         /* plan the query */
!         plan = pg_plan_query(query, 0, params);

!         /*
!          * Use a snapshot with an updated command ID to ensure this query sees
!          * results of any previously executed queries.  (This could only
!          * matter if the planner executed an allegedly-stable function that
!          * changed the database contents, but let's do it anyway to be
!          * parallel to the EXPLAIN code path.)
!          */
!         PushCopiedSnapshot(GetActiveSnapshot());
!         UpdateActiveSnapshotCommandId();

!         /* Create a QueryDesc, redirecting output to our tuple receiver */
!         queryDesc = CreateQueryDesc(plan, queryString,
!                                     GetActiveSnapshot(), InvalidSnapshot,
!                                     dest, params, 0);

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

!         /* run the plan to completion */
!         ExecutorRun(queryDesc, ForwardScanDirection, 0L);

!         /* save the rowcount if we're given a completionTag to fill */
!         if (completionTag)
!             snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
!                      "SELECT " UINT64_FORMAT,
!                      queryDesc->estate->es_processed);

!         /* get object address that intorel_startup saved for us */
!         address = ((DR_intorel *) dest)->reladdr;

!         /* and clean up */
!         ExecutorFinish(queryDesc);
!         ExecutorEnd(queryDesc);

!         FreeQueryDesc(queryDesc);

!         PopActiveSnapshot();
!     }

      if (is_matview)
      {
*************** ExecCreateTableAs(CreateTableAsStmt *stm
*** 216,224 ****
          SetUserIdAndSecContext(save_userid, save_sec_context);
      }

-     address = CreateAsReladdr;
-     CreateAsReladdr = InvalidObjectAddress;
-
      return address;
  }

--- 377,382 ----
*************** intorel_startup(DestReceiver *self, int
*** 287,300 ****
      IntoClause *into = myState->into;
      bool        is_matview;
      char        relkind;
!     CreateStmt *create;
      ObjectAddress intoRelationAddr;
      Relation    intoRelationDesc;
      RangeTblEntry *rte;
-     Datum        toast_options;
      ListCell   *lc;
      int            attnum;
-     static char *validnsps[] = HEAP_RELOPT_NAMESPACES;

      Assert(into != NULL);        /* else somebody forgot to set it */

--- 445,456 ----
      IntoClause *into = myState->into;
      bool        is_matview;
      char        relkind;
!     List       *attrList;
      ObjectAddress intoRelationAddr;
      Relation    intoRelationDesc;
      RangeTblEntry *rte;
      ListCell   *lc;
      int            attnum;

      Assert(into != NULL);        /* else somebody forgot to set it */

*************** intorel_startup(DestReceiver *self, int
*** 303,364 ****
      relkind = is_matview ? RELKIND_MATVIEW : RELKIND_RELATION;

      /*
-      * Create the target relation by faking up a CREATE TABLE parsetree and
-      * passing it to DefineRelation.
-      */
-     create = makeNode(CreateStmt);
-     create->relation = into->rel;
-     create->tableElts = NIL;    /* will fill below */
-     create->inhRelations = NIL;
-     create->ofTypename = NULL;
-     create->constraints = NIL;
-     create->options = into->options;
-     create->oncommit = into->onCommit;
-     create->tablespacename = into->tableSpaceName;
-     create->if_not_exists = false;
-
-     /*
       * Build column definitions using "pre-cooked" type and collation info. If
       * a column name list was specified in CREATE TABLE AS, override the
       * column names derived from the query.  (Too few column names are OK, too
       * many are not.)
       */
      lc = list_head(into->colNames);
      for (attnum = 0; attnum < typeinfo->natts; attnum++)
      {
          Form_pg_attribute attribute = typeinfo->attrs[attnum];
!         ColumnDef  *col = makeNode(ColumnDef);
!         TypeName   *coltype = makeNode(TypeName);

          if (lc)
          {
!             col->colname = strVal(lfirst(lc));
              lc = lnext(lc);
          }
          else
!             col->colname = NameStr(attribute->attname);
!         col->typeName = coltype;
!         col->inhcount = 0;
!         col->is_local = true;
!         col->is_not_null = false;
!         col->is_from_type = false;
!         col->storage = 0;
!         col->raw_default = NULL;
!         col->cooked_default = NULL;
!         col->collClause = NULL;
!         col->collOid = attribute->attcollation;
!         col->constraints = NIL;
!         col->fdwoptions = NIL;
!         col->location = -1;

!         coltype->names = NIL;
!         coltype->typeOid = attribute->atttypid;
!         coltype->setof = false;
!         coltype->pct_type = false;
!         coltype->typmods = NIL;
!         coltype->typemod = attribute->atttypmod;
!         coltype->arrayBounds = NIL;
!         coltype->location = -1;

          /*
           * It's possible that the column is of a collatable type but the
--- 459,489 ----
      relkind = is_matview ? RELKIND_MATVIEW : RELKIND_RELATION;

      /*
       * Build column definitions using "pre-cooked" type and collation info. If
       * a column name list was specified in CREATE TABLE AS, override the
       * column names derived from the query.  (Too few column names are OK, too
       * many are not.)
       */
+     attrList = NIL;
      lc = list_head(into->colNames);
      for (attnum = 0; attnum < typeinfo->natts; attnum++)
      {
          Form_pg_attribute attribute = typeinfo->attrs[attnum];
!         ColumnDef  *col;
!         char       *colname;

          if (lc)
          {
!             colname = strVal(lfirst(lc));
              lc = lnext(lc);
          }
          else
!             colname = NameStr(attribute->attname);

!         col = makeColumnDef(colname,
!                             attribute->atttypid,
!                             attribute->atttypmod,
!                             attribute->attcollation);

          /*
           * It's possible that the column is of a collatable type but the
*************** intorel_startup(DestReceiver *self, int
*** 367,380 ****
           * collation rather than complaining.)
           */
          if (!OidIsValid(col->collOid) &&
!             type_is_collatable(coltype->typeOid))
              ereport(ERROR,
                      (errcode(ERRCODE_INDETERMINATE_COLLATION),
                       errmsg("no collation was derived for column \"%s\" with collatable type %s",
!                             col->colname, format_type_be(coltype->typeOid)),
                       errhint("Use the COLLATE clause to set the collation explicitly.")));

!         create->tableElts = lappend(create->tableElts, col);
      }

      if (lc != NULL)
--- 492,506 ----
           * collation rather than complaining.)
           */
          if (!OidIsValid(col->collOid) &&
!             type_is_collatable(col->typeName->typeOid))
              ereport(ERROR,
                      (errcode(ERRCODE_INDETERMINATE_COLLATION),
                       errmsg("no collation was derived for column \"%s\" with collatable type %s",
!                             col->colname,
!                             format_type_be(col->typeName->typeOid)),
                       errhint("Use the COLLATE clause to set the collation explicitly.")));

!         attrList = lappend(attrList, col);
      }

      if (lc != NULL)
*************** intorel_startup(DestReceiver *self, int
*** 385,419 ****
      /*
       * Actually create the target table
       */
!     intoRelationAddr = DefineRelation(create, relkind, InvalidOid, NULL);
!
!     /*
!      * If necessary, create a TOAST table for the target table.  Note that
!      * NewRelationCreateToastTable ends with CommandCounterIncrement(), so
!      * that the TOAST table will be visible for insertion.
!      */
!     CommandCounterIncrement();
!
!     /* parse and validate reloptions for the toast table */
!     toast_options = transformRelOptions((Datum) 0,
!                                         create->options,
!                                         "toast",
!                                         validnsps,
!                                         true, false);
!
!     (void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true);
!
!     NewRelationCreateToastTable(intoRelationAddr.objectId, toast_options);
!
!     /* Create the "view" part of a materialized view. */
!     if (is_matview)
!     {
!         /* StoreViewQuery scribbles on tree, so make a copy */
!         Query       *query = (Query *) copyObject(into->viewQuery);
!
!         StoreViewQuery(intoRelationAddr.objectId, query, false);
!         CommandCounterIncrement();
!     }

      /*
       * Finally we can open the target table
--- 511,517 ----
      /*
       * Actually create the target table
       */
!     intoRelationAddr = create_ctas_internal(attrList, into);

      /*
       * Finally we can open the target table
*************** intorel_startup(DestReceiver *self, int
*** 462,472 ****
       * Fill private fields of myState for use by later routines
       */
      myState->rel = intoRelationDesc;
      myState->output_cid = GetCurrentCommandId(true);

-     /* and remember the new relation's address for ExecCreateTableAs */
-     CreateAsReladdr = intoRelationAddr;
-
      /*
       * We can skip WAL-logging the insertions, unless PITR or streaming
       * replication is in use. We can skip the FSM in any case.
--- 560,568 ----
       * Fill private fields of myState for use by later routines
       */
      myState->rel = intoRelationDesc;
+     myState->reladdr = intoRelationAddr;
      myState->output_cid = GetCurrentCommandId(true);

      /*
       * We can skip WAL-logging the insertions, unless PITR or streaming
       * replication is in use. We can skip the FSM in any case.
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index e9d9ba2..085bf32 100644
*** a/src/backend/commands/view.c
--- b/src/backend/commands/view.c
*************** DefineVirtualRelation(RangeVar *relation
*** 82,106 ****
      attrList = NIL;
      foreach(t, tlist)
      {
!         TargetEntry *tle = lfirst(t);

          if (!tle->resjunk)
          {
!             ColumnDef  *def = makeNode(ColumnDef);
!
!             def->colname = pstrdup(tle->resname);
!             def->typeName = makeTypeNameFromOid(exprType((Node *) tle->expr),
!                                              exprTypmod((Node *) tle->expr));
!             def->inhcount = 0;
!             def->is_local = true;
!             def->is_not_null = false;
!             def->is_from_type = false;
!             def->storage = 0;
!             def->raw_default = NULL;
!             def->cooked_default = NULL;
!             def->collClause = NULL;
!             def->collOid = exprCollation((Node *) tle->expr);
!             def->location = -1;

              /*
               * It's possible that the column is of a collatable type but the
--- 82,95 ----
      attrList = NIL;
      foreach(t, tlist)
      {
!         TargetEntry *tle = (TargetEntry *) lfirst(t);

          if (!tle->resjunk)
          {
!             ColumnDef  *def = makeColumnDef(tle->resname,
!                                             exprType((Node *) tle->expr),
!                                             exprTypmod((Node *) tle->expr),
!                                           exprCollation((Node *) tle->expr));

              /*
               * It's possible that the column is of a collatable type but the
*************** DefineVirtualRelation(RangeVar *relation
*** 117,123 ****
              }
              else
                  Assert(!OidIsValid(def->collOid));
-             def->constraints = NIL;

              attrList = lappend(attrList, def);
          }
--- 106,111 ----
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index f625c32..d72a85e 100644
*** a/src/backend/nodes/makefuncs.c
--- b/src/backend/nodes/makefuncs.c
*************** makeTypeNameFromOid(Oid typeOid, int32 t
*** 477,482 ****
--- 477,512 ----
  }

  /*
+  * makeColumnDef -
+  *    build a ColumnDef node to represent a simple column definition.
+  *
+  * Type and collation are specified by OID.
+  * Other properties are all basic to start with.
+  */
+ ColumnDef *
+ makeColumnDef(const char *colname, Oid typeOid, int32 typmod, Oid collOid)
+ {
+     ColumnDef  *n = makeNode(ColumnDef);
+
+     n->colname = pstrdup(colname);
+     n->typeName = makeTypeNameFromOid(typeOid, typmod);
+     n->inhcount = 0;
+     n->is_local = true;
+     n->is_not_null = false;
+     n->is_from_type = false;
+     n->storage = 0;
+     n->raw_default = NULL;
+     n->cooked_default = NULL;
+     n->collClause = NULL;
+     n->collOid = collOid;
+     n->constraints = NIL;
+     n->fdwoptions = NIL;
+     n->location = -1;
+
+     return n;
+ }
+
+ /*
   * makeFuncExpr -
   *    build an expression tree representing a function call.
   *
diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h
index ef34a6e..01c5cb4 100644
*** a/src/include/nodes/makefuncs.h
--- b/src/include/nodes/makefuncs.h
*************** extern TypeName *makeTypeName(char *typn
*** 72,77 ****
--- 72,80 ----
  extern TypeName *makeTypeNameFromNameList(List *names);
  extern TypeName *makeTypeNameFromOid(Oid typeOid, int32 typmod);

+ extern ColumnDef *makeColumnDef(const char *colname,
+               Oid typeOid, int32 typmod, Oid collOid);
+
  extern FuncExpr *makeFuncExpr(Oid funcid, Oid rettype, List *args,
               Oid funccollid, Oid inputcollid, CoercionForm fformat);

diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
index 7f9741e..102bf1f 100644
*** a/src/test/regress/expected/matview.out
--- b/src/test/regress/expected/matview.out
*************** DROP TABLE mvtest_boxes CASCADE;
*** 455,467 ****
  NOTICE:  drop cascades to materialized view mvtest_boxmv
  -- make sure that column names are handled correctly
  CREATE TABLE mvtest_v (i int, j int);
! CREATE MATERIALIZED VIEW mvtest_mv_v (ii) AS SELECT i, j AS jj FROM mvtest_v;
  ALTER TABLE mvtest_v RENAME COLUMN i TO x;
  INSERT INTO mvtest_v values (1, 2);
  CREATE UNIQUE INDEX mvtest_mv_v_ii ON mvtest_mv_v (ii);
  REFRESH MATERIALIZED VIEW mvtest_mv_v;
  UPDATE mvtest_v SET j = 3 WHERE x = 1;
  REFRESH MATERIALIZED VIEW CONCURRENTLY mvtest_mv_v;
  SELECT * FROM mvtest_v;
   x | j
  ---+---
--- 455,477 ----
  NOTICE:  drop cascades to materialized view mvtest_boxmv
  -- make sure that column names are handled correctly
  CREATE TABLE mvtest_v (i int, j int);
! CREATE MATERIALIZED VIEW mvtest_mv_v (ii, jj, kk) AS SELECT i, j FROM mvtest_v; -- error
! ERROR:  too many column names were specified
! CREATE MATERIALIZED VIEW mvtest_mv_v (ii, jj) AS SELECT i, j FROM mvtest_v; -- ok
! CREATE MATERIALIZED VIEW mvtest_mv_v_2 (ii) AS SELECT i, j FROM mvtest_v; -- ok
! CREATE MATERIALIZED VIEW mvtest_mv_v_3 (ii, jj, kk) AS SELECT i, j FROM mvtest_v WITH NO DATA; -- error
! ERROR:  too many column names were specified
! CREATE MATERIALIZED VIEW mvtest_mv_v_3 (ii, jj) AS SELECT i, j FROM mvtest_v WITH NO DATA; -- ok
! CREATE MATERIALIZED VIEW mvtest_mv_v_4 (ii) AS SELECT i, j FROM mvtest_v WITH NO DATA; -- ok
  ALTER TABLE mvtest_v RENAME COLUMN i TO x;
  INSERT INTO mvtest_v values (1, 2);
  CREATE UNIQUE INDEX mvtest_mv_v_ii ON mvtest_mv_v (ii);
  REFRESH MATERIALIZED VIEW mvtest_mv_v;
  UPDATE mvtest_v SET j = 3 WHERE x = 1;
  REFRESH MATERIALIZED VIEW CONCURRENTLY mvtest_mv_v;
+ REFRESH MATERIALIZED VIEW mvtest_mv_v_2;
+ REFRESH MATERIALIZED VIEW mvtest_mv_v_3;
+ REFRESH MATERIALIZED VIEW mvtest_mv_v_4;
  SELECT * FROM mvtest_v;
   x | j
  ---+---
*************** SELECT * FROM mvtest_mv_v;
*** 474,481 ****
    1 |  3
  (1 row)

  DROP TABLE mvtest_v CASCADE;
! NOTICE:  drop cascades to materialized view mvtest_mv_v
  -- make sure that matview rows can be referenced as source rows (bug #9398)
  CREATE TABLE mvtest_v AS SELECT generate_series(1,10) AS a;
  CREATE MATERIALIZED VIEW mvtest_mv_v AS SELECT a FROM mvtest_v WHERE a <= 5;
--- 484,520 ----
    1 |  3
  (1 row)

+ SELECT * FROM mvtest_mv_v_2;
+  ii | j
+ ----+---
+   1 | 3
+ (1 row)
+
+ SELECT * FROM mvtest_mv_v_3;
+  ii | jj
+ ----+----
+   1 |  3
+ (1 row)
+
+ SELECT * FROM mvtest_mv_v_4;
+  ii | j
+ ----+---
+   1 | 3
+ (1 row)
+
  DROP TABLE mvtest_v CASCADE;
! NOTICE:  drop cascades to 4 other objects
! DETAIL:  drop cascades to materialized view mvtest_mv_v
! drop cascades to materialized view mvtest_mv_v_2
! drop cascades to materialized view mvtest_mv_v_3
! drop cascades to materialized view mvtest_mv_v_4
! -- make sure that create WITH NO DATA does not plan the query (bug #13907)
! create materialized view mvtest_error as select 1/0 as x;  -- fail
! ERROR:  division by zero
! create materialized view mvtest_error as select 1/0 as x with no data;
! refresh materialized view mvtest_error;  -- fail here
! ERROR:  division by zero
! drop materialized view mvtest_error;
  -- make sure that matview rows can be referenced as source rows (bug #9398)
  CREATE TABLE mvtest_v AS SELECT generate_series(1,10) AS a;
  CREATE MATERIALIZED VIEW mvtest_mv_v AS SELECT a FROM mvtest_v WHERE a <= 5;
diff --git a/src/test/regress/expected/select_into.out b/src/test/regress/expected/select_into.out
index b577d1b..cee77e7 100644
*** a/src/test/regress/expected/select_into.out
--- b/src/test/regress/expected/select_into.out
*************** DETAIL:  drop cascades to table selinto_
*** 50,55 ****
--- 50,93 ----
  drop cascades to table selinto_schema.tmp2
  drop cascades to table selinto_schema.tmp3
  DROP USER selinto_user;
+ -- Tests for WITH NO DATA and column name consistency
+ CREATE TABLE ctas_base (i int, j int);
+ INSERT INTO ctas_base VALUES (1, 2);
+ CREATE TABLE ctas_nodata (ii, jj, kk) AS SELECT i, j FROM ctas_base; -- Error
+ ERROR:  too many column names were specified
+ CREATE TABLE ctas_nodata (ii, jj, kk) AS SELECT i, j FROM ctas_base WITH NO DATA; -- Error
+ ERROR:  too many column names were specified
+ CREATE TABLE ctas_nodata (ii, jj) AS SELECT i, j FROM ctas_base; -- OK
+ CREATE TABLE ctas_nodata_2 (ii, jj) AS SELECT i, j FROM ctas_base WITH NO DATA; -- OK
+ CREATE TABLE ctas_nodata_3 (ii) AS SELECT i, j FROM ctas_base; -- OK
+ CREATE TABLE ctas_nodata_4 (ii) AS SELECT i, j FROM ctas_base WITH NO DATA; -- OK
+ SELECT * FROM ctas_nodata;
+  ii | jj
+ ----+----
+   1 |  2
+ (1 row)
+
+ SELECT * FROM ctas_nodata_2;
+  ii | jj
+ ----+----
+ (0 rows)
+
+ SELECT * FROM ctas_nodata_3;
+  ii | j
+ ----+---
+   1 | 2
+ (1 row)
+
+ SELECT * FROM ctas_nodata_4;
+  ii | j
+ ----+---
+ (0 rows)
+
+ DROP TABLE ctas_base;
+ DROP TABLE ctas_nodata;
+ DROP TABLE ctas_nodata_2;
+ DROP TABLE ctas_nodata_3;
+ DROP TABLE ctas_nodata_4;
  --
  -- CREATE TABLE AS/SELECT INTO as last command in a SQL function
  -- have been known to cause problems
diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql
index 002698a..a108b69 100644
*** a/src/test/regress/sql/matview.sql
--- b/src/test/regress/sql/matview.sql
*************** DROP TABLE mvtest_boxes CASCADE;
*** 176,192 ****

  -- make sure that column names are handled correctly
  CREATE TABLE mvtest_v (i int, j int);
! CREATE MATERIALIZED VIEW mvtest_mv_v (ii) AS SELECT i, j AS jj FROM mvtest_v;
  ALTER TABLE mvtest_v RENAME COLUMN i TO x;
  INSERT INTO mvtest_v values (1, 2);
  CREATE UNIQUE INDEX mvtest_mv_v_ii ON mvtest_mv_v (ii);
  REFRESH MATERIALIZED VIEW mvtest_mv_v;
  UPDATE mvtest_v SET j = 3 WHERE x = 1;
  REFRESH MATERIALIZED VIEW CONCURRENTLY mvtest_mv_v;
  SELECT * FROM mvtest_v;
  SELECT * FROM mvtest_mv_v;
  DROP TABLE mvtest_v CASCADE;

  -- make sure that matview rows can be referenced as source rows (bug #9398)
  CREATE TABLE mvtest_v AS SELECT generate_series(1,10) AS a;
  CREATE MATERIALIZED VIEW mvtest_mv_v AS SELECT a FROM mvtest_v WHERE a <= 5;
--- 176,209 ----

  -- make sure that column names are handled correctly
  CREATE TABLE mvtest_v (i int, j int);
! CREATE MATERIALIZED VIEW mvtest_mv_v (ii, jj, kk) AS SELECT i, j FROM mvtest_v; -- error
! CREATE MATERIALIZED VIEW mvtest_mv_v (ii, jj) AS SELECT i, j FROM mvtest_v; -- ok
! CREATE MATERIALIZED VIEW mvtest_mv_v_2 (ii) AS SELECT i, j FROM mvtest_v; -- ok
! CREATE MATERIALIZED VIEW mvtest_mv_v_3 (ii, jj, kk) AS SELECT i, j FROM mvtest_v WITH NO DATA; -- error
! CREATE MATERIALIZED VIEW mvtest_mv_v_3 (ii, jj) AS SELECT i, j FROM mvtest_v WITH NO DATA; -- ok
! CREATE MATERIALIZED VIEW mvtest_mv_v_4 (ii) AS SELECT i, j FROM mvtest_v WITH NO DATA; -- ok
  ALTER TABLE mvtest_v RENAME COLUMN i TO x;
  INSERT INTO mvtest_v values (1, 2);
  CREATE UNIQUE INDEX mvtest_mv_v_ii ON mvtest_mv_v (ii);
  REFRESH MATERIALIZED VIEW mvtest_mv_v;
  UPDATE mvtest_v SET j = 3 WHERE x = 1;
  REFRESH MATERIALIZED VIEW CONCURRENTLY mvtest_mv_v;
+ REFRESH MATERIALIZED VIEW mvtest_mv_v_2;
+ REFRESH MATERIALIZED VIEW mvtest_mv_v_3;
+ REFRESH MATERIALIZED VIEW mvtest_mv_v_4;
  SELECT * FROM mvtest_v;
  SELECT * FROM mvtest_mv_v;
+ SELECT * FROM mvtest_mv_v_2;
+ SELECT * FROM mvtest_mv_v_3;
+ SELECT * FROM mvtest_mv_v_4;
  DROP TABLE mvtest_v CASCADE;

+ -- make sure that create WITH NO DATA does not plan the query (bug #13907)
+ create materialized view mvtest_error as select 1/0 as x;  -- fail
+ create materialized view mvtest_error as select 1/0 as x with no data;
+ refresh materialized view mvtest_error;  -- fail here
+ drop materialized view mvtest_error;
+
  -- make sure that matview rows can be referenced as source rows (bug #9398)
  CREATE TABLE mvtest_v AS SELECT generate_series(1,10) AS a;
  CREATE MATERIALIZED VIEW mvtest_mv_v AS SELECT a FROM mvtest_v WHERE a <= 5;
diff --git a/src/test/regress/sql/select_into.sql b/src/test/regress/sql/select_into.sql
index e4460ae..632077c 100644
*** a/src/test/regress/sql/select_into.sql
--- b/src/test/regress/sql/select_into.sql
*************** RESET SESSION AUTHORIZATION;
*** 53,58 ****
--- 53,77 ----
  DROP SCHEMA selinto_schema CASCADE;
  DROP USER selinto_user;

+ -- Tests for WITH NO DATA and column name consistency
+ CREATE TABLE ctas_base (i int, j int);
+ INSERT INTO ctas_base VALUES (1, 2);
+ CREATE TABLE ctas_nodata (ii, jj, kk) AS SELECT i, j FROM ctas_base; -- Error
+ CREATE TABLE ctas_nodata (ii, jj, kk) AS SELECT i, j FROM ctas_base WITH NO DATA; -- Error
+ CREATE TABLE ctas_nodata (ii, jj) AS SELECT i, j FROM ctas_base; -- OK
+ CREATE TABLE ctas_nodata_2 (ii, jj) AS SELECT i, j FROM ctas_base WITH NO DATA; -- OK
+ CREATE TABLE ctas_nodata_3 (ii) AS SELECT i, j FROM ctas_base; -- OK
+ CREATE TABLE ctas_nodata_4 (ii) AS SELECT i, j FROM ctas_base WITH NO DATA; -- OK
+ SELECT * FROM ctas_nodata;
+ SELECT * FROM ctas_nodata_2;
+ SELECT * FROM ctas_nodata_3;
+ SELECT * FROM ctas_nodata_4;
+ DROP TABLE ctas_base;
+ DROP TABLE ctas_nodata;
+ DROP TABLE ctas_nodata_2;
+ DROP TABLE ctas_nodata_3;
+ DROP TABLE ctas_nodata_4;
+
  --
  -- CREATE TABLE AS/SELECT INTO as last command in a SQL function
  -- have been known to cause problems

Re: BUG #13907: Restore materialized view throw permission denied

От
Michael Paquier
Дата:
On Tue, Jun 28, 2016 at 3:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm planning to apply the attached revision as soon as I get done
> back-porting it.  Main differences from your version:

Thanks for looking at that!

> * We can, and I think should, skip the rewriting phase too in the WITH NO
> DATA case.  Rewriting should never change a query's exposed result
> rowtype, and any other side-effects it might have are likely to be bad
> for our purposes.

OK. I have not skipped the rewrite phase in the case of my patch
because my thoughts of the moment were potential side damages with
rules. But after playing more with it I am not seeing any problems in
skipping it.

> * Rather than add a goto, I put the existing code sequence into the if's
> else block.  This will probably cause me some back-patching pain, but
> I don't like uglifying code just to make back-patch simpler.

OK. No problems with that, my point was indeed to ease backpatching.
That's an exercise difficult enough, there is no need to make it more
difficult.

> * The regression test cases you added seem not entirely on point, because
> they pass just fine against HEAD.  I don't object to them, but I added
> this to exercise the desired behavior change:
> -- make sure that create WITH NO DATA does not plan the query (bug #13907)
> create materialized view mvtest_error as select 1/0 as x;  -- fail
> create materialized view mvtest_error as select 1/0 as x with no data;
> refresh materialized view mvtest_error;  -- fail here
> drop materialized view mvtest_error;

Good idea. I haven't thought about triggering an error to be honest. I
got in mind first to create a plpgsql function that raises a NOTICE
message to show that the thing got planned or not, but gave up as that
was proving to be ugly for the purpose.

> * I also got rid of the static variable CreateAsReladdr, which while
> not related to the immediate problem is ugly and dangerous.  (It'd
> cause a failure in a nested-CREATE-TABLE-AS situation, which would
> be unusual but surely isn't forbidden.)

OK. That's really neater this way.

> I spent a little bit of time wondering whether we couldn't get rid of
> having intorel_startup create the relation at all, instead always doing
> it in ExecCreateTableAs().  But that doesn't work conveniently for
> CREATE TABLE AS EXECUTE, since there's no query tree handy in that case.

Yes, agreed.

> You could imagine moving the build-ColumnDefs-from-a-TupleDesc logic
> to someplace else that's only used for CREATE TABLE AS EXECUTE, but
> it's not clear to me that it's worth further refactoring just to make
> the WITH DATA and WITH NO DATA cases a bit more alike.

Nah, that does not seem worth it to me..
--
Michael

Re: BUG #13907: Restore materialized view throw permission denied

От
Kevin Grittner
Дата:
On Mon, Jun 27, 2016 at 1:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I'm planning to apply the attached revision as soon as I get done
> back-porting it.

I'm thinking of applying something like the attached to fix the
rest of this bug.

Thoughts?

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

Вложения

Re: BUG #13907: Restore materialized view throw permission denied

От
Peter Eisentraut
Дата:
On 7/25/16 4:09 PM, Kevin Grittner wrote:
> On Mon, Jun 27, 2016 at 1:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> I'm planning to apply the attached revision as soon as I get done
>> back-porting it.
>
> I'm thinking of applying something like the attached to fix the
> rest of this bug.

The reason that ACLs are restored last is that they could contain owner
self-revokes.  So anything that you run after the ACLs could fail
because of that.  I think a more complete fix would be to split up the
ACL restores into the general part, which you would run right after the
object is restored, and the owner revokes, which you would run last.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: BUG #13907: Restore materialized view throw permission denied

От
Kevin Grittner
Дата:
On Mon, Jul 25, 2016 at 8:37 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 7/25/16 4:09 PM, Kevin Grittner wrote:
>> On Mon, Jun 27, 2016 at 1:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>>> I'm planning to apply the attached revision as soon as I get done
>>> back-porting it.
>>
>> I'm thinking of applying something like the attached to fix the
>> rest of this bug.
>
> The reason that ACLs are restored last is that they could contain owner
> self-revokes.  So anything that you run after the ACLs could fail
> because of that.  I think a more complete fix would be to split up the
> ACL restores into the general part, which you would run right after the
> object is restored, and the owner revokes, which you would run last.

So you are suggesting that restoring from pg_dump output should
generate materialized view data under a different security context
than would be used by a REFRESH statement on the source database?

Anything you can add to clarify what you mean by "owner
self-revokes" and why the security should be different when the
matview data is refreshed at the end of the restore should be
different from what would happen on the database which was backed
up might be helpful.

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

Re: BUG #13907: Restore materialized view throw permission denied

От
Tom Lane
Дата:
Kevin Grittner <kgrittn@gmail.com> writes:
> So you are suggesting that restoring from pg_dump output should
> generate materialized view data under a different security context
> than would be used by a REFRESH statement on the source database?

Yes.  Consider the following simple example (done by a non-superuser
named joe):

create table joes_table(f1 int);
insert into joes_table values(1);
revoke insert on joes_table from joe;

pg_dump is required to be able to restore the state of this table
correctly.  It will fail to do so if it issues the revoke before
loading data.  The same issue applies to all data loading,
including refreshing matviews.

            regards, tom lane

Re: BUG #13907: Restore materialized view throw permission denied

От
Kevin Grittner
Дата:
On Tue, Jul 26, 2016 at 8:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@gmail.com> writes:
>> So you are suggesting that restoring from pg_dump output should
>> generate materialized view data under a different security context
>> than would be used by a REFRESH statement on the source database?
>
> Yes.  Consider the following simple example (done by a non-superuser
> named joe):
>
> create table joes_table(f1 int);
> insert into joes_table values(1);
> revoke insert on joes_table from joe;
>
> pg_dump is required to be able to restore the state of this table
> correctly.  It will fail to do so if it issues the revoke before
> loading data.  The same issue applies to all data loading,
> including refreshing matviews.

test=# create role joe;
CREATE ROLE
test=# set role joe;
SET
test=> create table joes_table(f1 int);
CREATE TABLE
test=> insert into joes_table values(1);
INSERT 0 1
test=> revoke insert on joes_table from joe;
REVOKE
test=> create materialized view joes_mv as select * from joes_table;
SELECT 1
test=> revoke insert on joes_mv from joe;
REVOKE
test=> refresh materialized view joes_mv;
REFRESH MATERIALIZED VIEW

The problem we're having restoring the matview state is that not
all ACLs needed for *SELECT* permissions are in place in time.  I
am not seeing the problem with self-revoke on REFRESH.  What am I
missing?

Of course, since the REVOKE on the matview has no affect, it should
probably not be allowed.  I could add an error for the attempt.

One other question about the patch was what I did for testing.  It
seemed like a good idea to have dump/restore tests, but I don't see
how to do that without leaving a role or two lingering in the
cluster.  Is that allowed?  (I see that I need to DIE first, to
prevent errors on multiple runs, but otherwise?)

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

Re: BUG #13907: Restore materialized view throw permission denied

От
Tom Lane
Дата:
Kevin Grittner <kgrittn@gmail.com> writes:
> The problem we're having restoring the matview state is that not
> all ACLs needed for *SELECT* permissions are in place in time.  I
> am not seeing the problem with self-revoke on REFRESH.  What am I
> missing?

Uh ... that the owner might revoke his own SELECT privilege?

> One other question about the patch was what I did for testing.  It
> seemed like a good idea to have dump/restore tests, but I don't see
> how to do that without leaving a role or two lingering in the
> cluster.  Is that allowed?

In make installcheck, no, it is absolutely not.  I'd suggest thinking
about testing this in the pg_dump TAP tests, instead, where we're just
creating a private database instance.

            regards, tom lane

Re: BUG #13907: Restore materialized view throw permission denied

От
Kevin Grittner
Дата:
On Tue, Jul 26, 2016 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@gmail.com> writes:
>> The problem we're having restoring the matview state is that not
>> all ACLs needed for *SELECT* permissions are in place in time.  I
>> am not seeing the problem with self-revoke on REFRESH.  What am I
>> missing?
>
> Uh ... that the owner might revoke his own SELECT privilege?

What about policies that might have changed since the latest
REFRESH before dump?  How about views or functions that might have
changed?  If I'm understanding what you want, the only way to get
that would be to COPY matview data the same as a table, and have a
way to load it in before locking down the matview from direct
changes.  That seems to me like it would completely compromise
future ability to incrementally maintain matviews, since you could
not have any assurance that the state of a matview matched any
particular derivation from the sources, matched to a point where
changes could be played forward.

>> One other question about the patch was what I did for testing.  It
>> seemed like a good idea to have dump/restore tests, but I don't see
>> how to do that without leaving a role or two lingering in the
>> cluster.  Is that allowed?
>
> In make installcheck, no, it is absolutely not.  I'd suggest thinking
> about testing this in the pg_dump TAP tests, instead, where we're just
> creating a private database instance.

ok

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

Re: BUG #13907: Restore materialized view throw permission denied

От
Tom Lane
Дата:
Kevin Grittner <kgrittn@gmail.com> writes:
> On Tue, Jul 26, 2016 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Uh ... that the owner might revoke his own SELECT privilege?

> What about policies that might have changed since the latest
> REFRESH before dump?  How about views or functions that might have
> changed?

I think you're setting up straw men.  No one has argued that we should
dump and restore the verbatim current contents of a matview.  But there
is a longstanding expectation that pg_dump be able to dump and restore
the contents of read-only tables, and what you proposed breaks that.
That won't do.

It's possible that the most reasonable fix is to move matview refreshes to
after the ACL restoration step.  That would wreak a lot of havoc with
the current view of a dump as being tripartite (pre-data/data/post-data),
so it might be more work than we want to do.  But it seems like the
logically soundest thing.

            regards, tom lane

Re: BUG #13907: Restore materialized view throw permission denied

От
Kevin Grittner
Дата:
On Tue, Jul 26, 2016 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> It's possible that the most reasonable fix is to move matview refreshes to
> after the ACL restoration step.  That would wreak a lot of havoc with
> the current view of a dump as being tripartite (pre-data/data/post-data),
> so it might be more work than we want to do.  But it seems like the
> logically soundest thing.

That is exactly what the patch I posted yesterday does.  Peter was
suggesting that wasn't good enough and that we should "split up the
ACL restores into the general part, which you would run right after
the object is restored, and the owner revokes, which you would run
last."  I guess the idea is that we would refresh the matviews in
between those two phases.

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

Re: BUG #13907: Restore materialized view throw permission denied

От
Tom Lane
Дата:
Kevin Grittner <kgrittn@gmail.com> writes:
> On Tue, Jul 26, 2016 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It's possible that the most reasonable fix is to move matview refreshes to
>> after the ACL restoration step.  That would wreak a lot of havoc with
>> the current view of a dump as being tripartite (pre-data/data/post-data),
>> so it might be more work than we want to do.  But it seems like the
>> logically soundest thing.

> That is exactly what the patch I posted yesterday does.  Peter was
> suggesting that wasn't good enough

Well, he's right: just minimally rearranging the dump order is not enough.
For one thing, I'm pretty sure this patch will fail to fix the bug in a
parallel restore, because restore_toc_entries_parallel has hard-wired
knowledge that ACLs can be done last and serially.  Even if the fix works
in parallel restore, it would result in doing matview refreshes serially
not in parallel, which is a pretty tough nut to swallow.  And I'm not
really convinced that the patch preserves dependency ordering requirements
at all, even in plain serial restore.

The core of the problem here, I think, is that pg_dump has never had any
real notion of dependency ordering for ACLs, since it's always been
sufficient to dump them at the very end in arbitrary order.  If we're now
going to make objects-with-dependencies also dependent on ACLs, I'm afraid
that raises the bar quite a lot in terms of needing to treat ACLs as
real, dependency-sorted objects.

I believe the thrust of Peter's suggestion is to try to avoid biting that
bullet, by instead instituting a rule of "dump an object's ACL immediately
after the object".  But to make this work with the read-only-table
scenario, it would have to be something like "dump an object's GRANTs
immediately after the object, but if there are any self-revokes, dump
those last as we always have".

With either approach, I'm afraid we're talking about a patch much larger
and more invasive than what's here :-(

            regards, tom lane

Re: BUG #13907: Restore materialized view throw permission denied

От
Kevin Grittner
Дата:
On Tue, Jul 26, 2016 at 1:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@gmail.com> writes:
>> On Tue, Jul 26, 2016 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> It's possible that the most reasonable fix is to move matview refreshes to
>>> after the ACL restoration step.  That would wreak a lot of havoc with
>>> the current view of a dump as being tripartite (pre-data/data/post-data),
>>> so it might be more work than we want to do.  But it seems like the
>>> logically soundest thing.
>
>> That is exactly what the patch I posted yesterday does.  Peter was
>> suggesting that wasn't good enough
>
> Well, he's right: just minimally rearranging the dump order is not enough.
> For one thing, I'm pretty sure this patch will fail to fix the bug in a
> parallel restore, because restore_toc_entries_parallel has hard-wired
> knowledge that ACLs can be done last and serially.

I think I covered that.  I tested all dump formats, including
parallel; but it is possible there was something wrong with my
testing.

> Even if the fix works
> in parallel restore, it would result in doing matview refreshes serially
> not in parallel, which is a pretty tough nut to swallow.

ok

> And I'm not
> really convinced that the patch preserves dependency ordering requirements
> at all, even in plain serial restore.

Pre-patch, it passes the sorted objects twice -- once for
everything except ACLs and again for ACLs.  I just made sure that
refreshes sorted past ACLs and moved them to the ACL phase.  I
don't see why that would disturb the order in which the objects are
passed either time; just what the filtering is on each pass.

> The core of the problem here, I think, is that pg_dump has never had any
> real notion of dependency ordering for ACLs, since it's always been
> sufficient to dump them at the very end in arbitrary order.  If we're now
> going to make objects-with-dependencies also dependent on ACLs, I'm afraid
> that raises the bar quite a lot in terms of needing to treat ACLs as
> real, dependency-sorted objects.

I was wondering why we went outside the normal ordering logic for
ACLs -- seems like a pretty ugly hack, but I didn't figure it was
the job of this patch to fix that.

> I believe the thrust of Peter's suggestion is to try to avoid biting that
> bullet, by instead instituting a rule of "dump an object's ACL immediately
> after the object".  But to make this work with the read-only-table
> scenario, it would have to be something like "dump an object's GRANTs
> immediately after the object, but if there are any self-revokes, dump
> those last as we always have".

I see what you're saying, but read-only-tables aren't the problem.
The only problem this patch leaves, I think, is that a matview for
which objects it depends on have changed since the last REFRESH (or
CREATE WITH DATA) might not be able to refresh at the end; but I
don't see how you avoid that with policies, functions, or views --
so I'm not clear why we care more about ACLs in that regard.

> With either approach, I'm afraid we're talking about a patch much larger
> and more invasive than what's here :-(

Well, this allows the example from the bug report to run
successfully.  It seems to me to generate dumps that are clearly
better that what are generated without the patch; so perhaps this
is worthwhile for now (with more appropriate tests, of course)
while we sort out the perfect solution for somewhere down the line.

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