Обсуждение: CREATE OR REPLACE MATERIALIZED VIEW

I like to add CREATE OR REPLACE MATERIALIZED VIEW with the attached
patches.

Patch 0001 adds CREATE OR REPLACE MATERIALIZED VIEW similar to CREATE OR
REPLACE VIEW.  It also includes regression tests and changes to docs.

Patch 0002 deprecates CREATE MATERIALIZED VIEW IF NOT EXISTS because it
no longer seems necessary with patch 0001.  Tom Lane commented[1] about
the general dislike of IF NOT EXISTS, to which I agree, but maybe this
was meant only in response to adding new commands.  Anyway, my idea is
to deprecate that usage in PG18 and eventually remove it in PG19, if
there's consensus for it.  We can drop that clause without violating any
standard because matviews are a Postgres extension.  I'm not married to
the idea, just want to put it on the table for discussion.

Motivation
----------

At $JOB we use materialized views for caching a couple of expensive
views.  But every now and then those views have to be changed, e.g., new
logic, new columns, etc.  The matviews have to be dropped and re-created
to include new columns.  (Just changing the underlying view logic
without adding new columns is trivial because the matviews are just thin
wrappers that just have to be refreshed.)

We also have several views that depend on those matviews.  The views
must also be dropped in order to re-create the matviews.  We've already
automated this with two procedures that stash and re-create dependent
view definitions.

Native support for replacing matviews would simplify our setup and it
would make CREATE MATERIALIZED VIEW more complete when compared to
CREATE VIEW.

I searched the lists for previous discussions on this topic but couldn't
find any.  So, I don't know if this was ever tried, but rejected for
some reason.  I've found slides[2] from 2013 (when matviews landed in
9.3) which have OR REPLACE on the roadmap:

> Materialised Views roadmap
>
> * CREATE **OR REPLACE** MATERIALIZED VIEW
>   * Just an oversight that it wasn't added
>  [...]

Replacing Matviews
------------------

With patch 0001, a matview can be replaced without having to drop it and
its dependent objects.  In our use case it is no longer necessary to
define the actual query in a separate view.  Replacing a matview works
analogous to CREATE OR REPLACE VIEW:

* the new query may change SELECT list expressions of existing columns
* new columns can be added to the end of the SELECT list
* existing columns cannot be renamed
* the data type of existing columns cannot be changed

In addition to that, CREATE OR REPLACE MATERIALIZED VIEW also replaces
access method, tablespace, and storage parameters if specified.  The
clause WITH [NO] DATA works as expected: it either populates the matview
or leaves it in an unscannable state.

It is an error to specify both OR REPLACE and IF NOT EXISTS.

Example
-------

postgres=# CREATE MATERIALIZED VIEW test AS SELECT 1 AS a;
SELECT 1
postgres=# SELECT * FROM test;
 a
---
 1
(1 row)

postgres=# CREATE OR REPLACE MATERIALIZED VIEW test AS SELECT 2 AS a, 3 AS b;
CREATE MATERIALIZED VIEW
postgres=# SELECT * FROM test;
 a | b
---+---
 2 | 3
(1 row)

Implementation Details
----------------------

Patch 0001 extends create_ctas_internal in order to adapt an existing
matview to the new tuple descriptor, access method, tablespace, and
storage parameters.  This logic is mostly based on DefineViewRelation.
This also reuses checkViewColumns, but adds argument is_matview in order
to tell if we want error messages for a matview (true) or view (false).
I'm not sure if that flag is the correct way to do that, or if I should
just create a separate function just for matviews with the same logic.
Do we even need to distinguish between view and matview in those error
messages?

The patch also adds tab completion in psql for CREATE OR REPLACE
MATERIALIZED VIEW.

[1] https://www.postgresql.org/message-id/226806.1693430777%40sss.pgh.pa.us
[2] https://wiki.postgresql.org/images/a/ad/Materialised_views_now_and_the_future-pgconfeu_2013.pdf#page=23

-- 
Erik

Hi,

> Patch 0002 deprecates CREATE MATERIALIZED VIEW IF NOT EXISTS because it
> no longer seems necessary with patch 0001.  Tom Lane commented[1] about
> the general dislike of IF NOT EXISTS, to which I agree, but maybe this
> was meant only in response to adding new commands.  Anyway, my idea is
> to deprecate that usage in PG18 and eventually remove it in PG19, if
> there's consensus for it.  We can drop that clause without violating any
> standard because matviews are a Postgres extension.  I'm not married to
> the idea, just want to put it on the table for discussion.

I can imagine how this may impact many applications and upset many
software developers worldwide. Was there even a precedent (in the
recent decade or so) when PostgreSQL broke the SQL syntax?

To clarify, I'm not opposed to this idea. If we are fine with breaking
backward compatibility on the SQL level, this would allow dropping the
support of inherited tables some day, a feature that in my humble
opinion shouldn't exist (I realize this is another and very debatable
question though). I just don't think this is something we ever do in
this project. But I admit that this information may be incorrect
and/or outdated.

-- 
Best regards,
Aleksander Alekseev

> On 2 Jul 2024, at 03:22, Erik Wienhold <ewie@ewie.name> wrote:

> Patch 0002 deprecates CREATE MATERIALIZED VIEW IF NOT EXISTS because it
> no longer seems necessary with patch 0001.  Tom Lane commented[1] about
> the general dislike of IF NOT EXISTS, to which I agree, but maybe this
> was meant only in response to adding new commands.  Anyway, my idea is
> to deprecate that usage in PG18 and eventually remove it in PG19, if
> there's consensus for it.

Considering the runway we typically give for deprecations, that seems like a
fairly short timeframe for a SQL level command which isn't unlikely to exist
in application code.

--
Daniel Gustafsson

I wrote:
> Patch 0002 deprecates CREATE MATERIALIZED VIEW IF NOT EXISTS because it
> no longer seems necessary with patch 0001.  Tom Lane commented[1] about
> the general dislike of IF NOT EXISTS, to which I agree, but maybe this
> was meant only in response to adding new commands.

One could also argue that since matviews are a hybrid of tables and
views, that CREATE MATERIALIZED VIEW should accept both OR REPLACE (as
in CREATE VIEW) and IF NOT EXISTS (as in CREATE TABLE).  But not in the
same invocation of course.

On 2024-07-02 12:46 +0200, Aleksander Alekseev wrote:
> > Anyway, my idea is to deprecate that usage in PG18 and eventually
> > remove it in PG19, if there's consensus for it.  We can drop that
> > clause without violating any standard because matviews are a
> > Postgres extension.  I'm not married to the idea, just want to put
> > it on the table for discussion.
> 
> I can imagine how this may impact many applications and upset many
> software developers worldwide. Was there even a precedent (in the
> recent decade or so) when PostgreSQL broke the SQL syntax?

A quick spelunking through the changelog with

    git log --grep deprecat -i --since '10 years ago'

turned up two commits:

578b229718 "Remove WITH OIDS support, change oid catalog column visibility."
e8d016d819 "Remove deprecated COMMENT ON RULE syntax"

Both were committed more than 10 years after deprecating the respective
feature.  My proposed one-year window seems a bit harsh in comparison.

On 2024-07-02 14:27 +0200, Daniel Gustafsson wrote:
> Considering the runway we typically give for deprecations, that seems like a
> fairly short timeframe for a SQL level command which isn't unlikely to exist
> in application code.

Is there some general agreed upon timeframe, or is decided on a
case-by-case basis?  I can imagine waiting at least until the last
release without the deprecation reaches EOL.  That would be 5 years with
the current versioning policy.

-- 
Erik

> On 2 Jul 2024, at 15:58, Erik Wienhold <ewie@ewie.name> wrote:
> On 2024-07-02 14:27 +0200, Daniel Gustafsson wrote:
>> Considering the runway we typically give for deprecations, that seems like a
>> fairly short timeframe for a SQL level command which isn't unlikely to exist
>> in application code.
>
> Is there some general agreed upon timeframe, or is decided on a
> case-by-case basis?  I can imagine waiting at least until the last
> release without the deprecation reaches EOL.  That would be 5 years with
> the current versioning policy.

AFAIK it's all decided on a case-by-case basis depending on impact.  There are
for example the removals you listed, and there are functions in libpq which
were deprecated in the postgres 6.x days which are still around to avoid
breaking ABI.

--
Daniel Gustafsson

Hi,

+1 for this feature.

Replacing Matviews
------------------

With patch 0001, a matview can be replaced without having to drop it and
its dependent objects.  In our use case it is no longer necessary to
define the actual query in a separate view.  Replacing a matview works
analogous to CREATE OR REPLACE VIEW:

* the new query may change SELECT list expressions of existing columns
* new columns can be added to the end of the SELECT list
* existing columns cannot be renamed
* the data type of existing columns cannot be changed

In addition to that, CREATE OR REPLACE MATERIALIZED VIEW also replaces
access method, tablespace, and storage parameters if specified.  The
clause WITH [NO] DATA works as expected: it either populates the matview
or leaves it in an unscannable state.

It is an error to specify both OR REPLACE and IF NOT EXISTS.

I noticed replacing the materialized view is blocking all reads. Is that expected ? Even if there is a unique index ?

Best,
Saïd

On 2024-07-04 22:18 +0200, Said Assemlal wrote:
> +1 for this feature.

Thanks!

> I noticed replacing the materialized view is blocking all reads. Is that
> expected ? Even if there is a unique index ?

That is expected because AccessExclusiveLock is acquired on the existing
matview.  This is also the case for CREATE OR REPLACE VIEW.

My initial idea, while writing the patch, was that one could replace the
matview without populating it and then run the concurrent refresh, like
this:

    CREATE OR REPLACE MATERIALIZED VIEW foo AS ... WITH NO DATA;
    REFRESH MATERIALIZED VIEW CONCURRENTLY foo;

But that won't work because concurrent refresh requires an already
populated matview.

Right now the patch either populates the replaced matview or leaves it
in an unscannable state.  Technically, it's also possible to skip the
refresh and leave the old data in place, perhaps by specifying
WITH *OLD* DATA.  New columns would just be null.  Of course you can't
tell if you got stale data without knowing how the matview was replaced.
Thoughts?

-- 
Erik

> That is expected because AccessExclusiveLock is acquired on the existing
> matview.  This is also the case for CREATE OR REPLACE VIEW.

Right, had this case many times.

>
> My initial idea, while writing the patch, was that one could replace the
> matview without populating it and then run the concurrent refresh, like
> this:
>
>      CREATE OR REPLACE MATERIALIZED VIEW foo AS ... WITH NO DATA;
>      REFRESH MATERIALIZED VIEW CONCURRENTLY foo;
>
> But that won't work because concurrent refresh requires an already
> populated matview.
>
> Right now the patch either populates the replaced matview or leaves it
> in an unscannable state.  Technically, it's also possible to skip the
> refresh and leave the old data in place, perhaps by specifying
> WITH *OLD* DATA.  New columns would just be null.  Of course you can't
> tell if you got stale data without knowing how the matview was replaced.
> Thoughts?


I believe the expectation is to get materialized views updated whenever 
it gets replaced so likely to confuse users ?

On 2024-07-12 16:49 +0200, Said Assemlal wrote:
> > My initial idea, while writing the patch, was that one could replace the
> > matview without populating it and then run the concurrent refresh, like
> > this:
> > 
> >      CREATE OR REPLACE MATERIALIZED VIEW foo AS ... WITH NO DATA;
> >      REFRESH MATERIALIZED VIEW CONCURRENTLY foo;
> > 
> > But that won't work because concurrent refresh requires an already
> > populated matview.
> > 
> > Right now the patch either populates the replaced matview or leaves it
> > in an unscannable state.  Technically, it's also possible to skip the
> > refresh and leave the old data in place, perhaps by specifying
> > WITH *OLD* DATA.  New columns would just be null.  Of course you can't
> > tell if you got stale data without knowing how the matview was replaced.
> > Thoughts?
> 
> I believe the expectation is to get materialized views updated whenever it
> gets replaced so likely to confuse users ?

I agree, that could be confusing -- unless it's well documented.  The
attached 0003 implements WITH OLD DATA and states in the docs that this
is intended to be used before a concurrent refresh.

Patch 0001 now covers all matview cases in psql's tab completion.  I
missed some of them with v1.

-- 
Erik

The attached v6 fixes the build.  Somehow I missed testing with
--with-cassert the whole time and it turned that out I forgot to pass
queryString to ExecRefreshMatView.

-- 
Erik Wienhold

Erik Wienhold <ewie@ewie.name> writes:
> [ v6 patches ]

A couple of drive-by comments:

* I think the proposal to deprecate IF NOT EXISTS is a nonstarter.
Yeah, I don't like it much, but the standard of proof to remove
features is amazingly high and I don't think it's been reached here.
We're unlikely to remove IF NOT EXISTS for tables, and to the extent
that matviews are like tables it's reasonable for them to have it too.

* On the other hand, the semantics you've implemented for CREATE OR
REPLACE are not right.  The contract for any form of C.O.R. is that
it will either fail, or produce exactly the same object definition
that you would have gotten from plain CREATE with no conflicting
object.  The v6 code is visibly not doing that for properties such
as tablespace --- if the command doesn't mention that, you don't
get the default tablespace, you get whatever the old object had.

* BTW, I'm inclined to think that WITH OLD DATA ought to fail
if the command isn't replacing an existing matview.  It seems
inconsistent to silently reinterpret it as WITH DATA, just as
silently reinterpreting "no tablespace mentioned" as "use the
old tablespace" is inconsistent.  I'm not dead set on that
but it feels wrong.

            regards, tom lane

Sorry for the late reply but I haven't had the time to dig into this.
Here's v7 fixing the points below.

On 2025-04-05 22:37 +0200, Tom Lane wrote:
> * I think the proposal to deprecate IF NOT EXISTS is a nonstarter.
> Yeah, I don't like it much, but the standard of proof to remove
> features is amazingly high and I don't think it's been reached here.
> We're unlikely to remove IF NOT EXISTS for tables, and to the extent
> that matviews are like tables it's reasonable for them to have it too.

Yeah, I got that gist from the replies upthread and dropped that patch.

> * On the other hand, the semantics you've implemented for CREATE OR
> REPLACE are not right.  The contract for any form of C.O.R. is that
> it will either fail, or produce exactly the same object definition
> that you would have gotten from plain CREATE with no conflicting
> object.  The v6 code is visibly not doing that for properties such
> as tablespace --- if the command doesn't mention that, you don't
> get the default tablespace, you get whatever the old object had.

Thanks a lot.  I added a test case for that and v7-0001 now restores the
default options if none are specified.  Handling the default tablespace
is a bit cumbersome IMO because its name must be passed to
AlterTableInternal.  With v7-0002 I moved that to ATPrepSetTableSpace as
an alternative using the empty string as stand-in for the default
tablespace.  What do you think?

> * BTW, I'm inclined to think that WITH OLD DATA ought to fail
> if the command isn't replacing an existing matview.  It seems
> inconsistent to silently reinterpret it as WITH DATA, just as
> silently reinterpreting "no tablespace mentioned" as "use the
> old tablespace" is inconsistent.  I'm not dead set on that
> but it feels wrong.

Yes that also felt iffy to me.  It just didn't occur to me to simply
raise an error in ExecCreateTableAs.  Done so in v7-0003.

-- 
Erik Wienhold

Hello,

Yan Haibo and I reviewed this patch for the Patch Review Workshop.

The patch applies, compiles, and passes tests.

From v7-0001-Add-OR-REPLACE-option-to-CREATE-MATERIALIZED-VIEW.patch:

```
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml
b/doc/src/sgml/ref/create_materialized_view.sgml
index 62d897931c3..5e03320eb73 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -67,7 +78,7 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ]
<replaceable>table_name</replaceable>
       Do not throw an error if a materialized view with the same name already
       exists. A notice is issued in this case.  Note that there is no guarantee
       that the existing materialized view is anything like the one that would
-      have been created.
+      have been created, unless you use <literal>OR REPLACE</literal> instead.
      </para>
     </listitem>
    </varlistentry>
```

"Unless" flows strangely here, at first connoting that it will work
with IF NOT EXISTS, but then "instead" retracts that suggestion.
Perhaps "unlike when using OR REPLACE".

```
 static ObjectAddress
 create_ctas_internal(List *attrList, IntoClause *into)
 {
-   CreateStmt *create = makeNode(CreateStmt);
-   bool        is_matview;
+   bool        is_matview,
+               replace = false;
    char        relkind;
-   Datum       toast_options;
-   const char *const validnsps[] = HEAP_RELOPT_NAMESPACES;
+   Oid         matviewOid = InvalidOid;
    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->accessMethod = into->accessMethod;
```

It seems like there is very little shared in this function between the
old code and new. Should replacing a matview just have a different
function altogether?

```
+   /* Check if an existing materialized view needs to be replaced. */
+   if (is_matview)
+   {
+       LOCKMODE    lockmode;

-   /*
-    * 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, NULL);
+       lockmode = into->replace ? AccessExclusiveLock : NoLock;
+       (void) RangeVarGetAndCheckCreationNamespace(into->rel, lockmode,
+                                                   &matviewOid);
+       replace = OidIsValid(matviewOid) && into->replace;
+   }
```

We didn't call RangeVarGetAndCheckCreationNamespace before, but now we
call it whether you say `OR REPLACE` or not. That seems suspicious.
Why do we need to call it even when you don't ask to replace an
existing matview? We only use matviewOid if we're replacing, and we
only lock the relation if we're replacing. So probably this should be
`if (is_matview && into->replace)` (which makes this function seem
even more strongly that it should be split into two).

```
+       rel = relation_open(matviewOid, NoLock);
```

The stanzas in this branch need comments. The code strongly resembles
DefineVirtualRelation, but without the comments. For instance in this
line, I wasn't sure why were opening the relation with no lock. This
comment from DefineVirtualRelation is very helpful:

```
/* Relation is already locked, but we must build a relcache entry. */`
```

The other lost comments would be helpful too.

```
+       /* add new attributes */
+       if (list_length(attrList) > rel->rd_att->natts)
+       {
+           ListCell   *c;
+           int         skip = rel->rd_att->natts;
+
+           foreach(c, attrList)
+           {
+               if (skip > 0)
+               {
+                   skip--;
+                   continue;
+               }
+               atcmd = makeNode(AlterTableCmd);
+               atcmd->subtype = AT_AddColumnToView;
+               atcmd->def = (Node *) lfirst(c);
+               atcmds = lappend(atcmds, atcmd);
+           }
+       }
```

Can we use list_nth_cell to start at the right position and avoid the
manual skipping? (So the loop would be for instead of foreach.) Or
even better, for_each_from looks perfect for this.

```
+       /* access method */
+       atcmd = makeNode(AlterTableCmd);
+       atcmd->subtype = AT_SetAccessMethod;
+       atcmd->name = into->accessMethod ? into->accessMethod :
default_table_access_method;
+       atcmds = lappend(atcmds, atcmd);
```

Just to confirm, this is a noop if the access method is already the
same? And likewise with the other AT subcommands?

```
@@ -234,7 +342,26 @@ ExecCreateTableAs(ParseState *pstate,
CreateTableAsStmt *stmt,

    /* Check if the relation exists or not */
    if (CreateTableAsRelExists(stmt))
+   {
+       /* An existing materialized view can be replaced. */
+       if (is_matview && into->replace)
+       {
+           RefreshMatViewStmt *refresh;
+
+           /* Change the relation to match the new query and other options. */
+           (void) create_ctas_nodata(query->targetList, into);
+
+           /* Refresh the materialized view with a fake statement. */
+           refresh = makeNode(RefreshMatViewStmt);
+           refresh->relation = into->rel;
+           refresh->skipData = into->skipData;
+           refresh->concurrent = false;
+
+           return ExecRefreshMatView(refresh, pstate->p_sourcetext, qc);
+       }
+
        return InvalidObjectAddress;
+   }
```

Maybe an `else` for the other case? Having parallel indentation
communicates the parallel execution.

```
@@ -402,14 +529,15 @@ CreateTableAsRelExists(CreateTableAsStmt *ctas)
    oldrelid = get_relname_relid(into->rel->relname, nspid);
    if (OidIsValid(oldrelid))
    {
-       if (!ctas->if_not_exists)
+       if (!ctas->if_not_exists && !into->replace)
```

Changing the contract without changing the function comment seems wrong.

But is this really factored correctly? Maybe the check should happen
in the caller instead.

Does this require a change to ExplainOneUtility, which also calls this
function? At least we probably need to handle OR REPLACE there.

```
    if (newdesc->natts < olddesc->natts)
-       ereport(ERROR,
-               (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
-                errmsg("cannot drop columns from view")));
+   {
+       if (is_matview)
+           ereport(ERROR,
+                   errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                   errmsg("cannot drop columns from materialized view"));
+       else
+           ereport(ERROR,
+                   (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                    errmsg("cannot drop columns from view")));
+   }
```

Instead of repeating so much, perhaps `errmsg("cannot drop columns
from %s", is_matview ? "materialized view" : "view")`? Or since that
is probably bad for translators, at least this:

```
errmsg(is_matview
       ? "cannot drop columns from materialized view"
       : "cannot drop columns from view")
```

Likewise with more error messages in this function. (There are a lot of them.)

The parser changes seem fine (see below re the third patch though).

Tab completion seems fine.

From v7-0002-Handle-default-tablespace-in-AlterTableInternal.patch:

These changes should get rolled into the previous patch. I think you
broke them out to allow considering them separately, although your
patches would be simpler if they were the first patch in the series
then.

```
+       /* use empty string to specify default tablespace */
+       atcmd->name = into->tableSpaceName ? into->tableSpaceName : "";
        atcmds = lappend(atcmds, atcmd);
```

We didn't love using "" as a magic string to signal the
AT_SetTableSpace subcommand. At least it should be a named constant,
but maybe a separate struct member is better.

```
-   /* Check that the tablespace exists */
-   tablespaceId = get_tablespace_oid(tablespacename, false);
+   if (tablespacename != NULL && tablespacename[0] == '\0')
+   {
+       /* Use default tablespace if name is empty string */
+       tablespaceId =
GetDefaultTablespace(rel->rd_rel->relpersistence,
rel->rd_rel->relispartition);
+       if (!OidIsValid(tablespaceId))
+           tablespaceId = MyDatabaseTableSpace;
+   }
+   else
+   {
+       /* Check that the tablespace exists */
+       tablespaceId = get_tablespace_oid(tablespacename, false);
+   }

    /* Check permissions except when moving to database's default */
    if (OidIsValid(tablespaceId) && tablespaceId != MyDatabaseTableSpace)
```

This seems like something that should happen during analysis, but I
know we are less strict for utility statements.

Are there any locking issues with calling GetDefaultTablespace here
instead of earlier? Why does ATPrepSetTableSpace take a lockmode but
not use it? Can the table space get dropped in the middle of the
command? But this is probably handled already. Do we need the lock the
*old* tablespace?

What about partitions?

From v7-0003-Add-WITH-OLD-DATA-to-CREATE-OR-REPLACE-MATERIALIZ.patch:

```
@@ -383,6 +391,9 @@ ExecCreateTableAs(ParseState *pstate,
CreateTableAsStmt *stmt,
     */
    if (is_matview)
    {
+       if (into->keepData)
+           elog(ERROR, "must not specify WITH OLD DATA when creating
a new materialized view");
+
        do_refresh = !into->skipData;
        into->skipData = true;
    }
```

Should this be ereport? I guess the current parser productions should
prevent us from reaching this code though. So perhaps a comment?

```
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7aaf0e37ad8..e51ce2701be 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4944,6 +4944,22 @@ CreateMatViewStmt:
                    $7->replace = true;
                    $$ = (Node *) ctas;
                }
+       | CREATE OR REPLACE OptNoLog MATERIALIZED VIEW
create_mv_target AS SelectStmt WITH OLD DATA_P
+               {
+                   CreateTableAsStmt *ctas = makeNode(CreateTableAsStmt);
+
+                   ctas->query = $9;
+                   ctas->into = $7;
+                   ctas->objtype = OBJECT_MATVIEW;
+                   ctas->is_select_into = false;
+                   ctas->if_not_exists = false;
+                   /* cram additional flags into the IntoClause */
+                   $7->rel->relpersistence = $4;
+                   $7->skipData = false;
+                   $7->keepData = true;
+                   $7->replace = true;
+                   $$ = (Node *) ctas;
+               }
        ;
```

This is the fourth production for minor variations of
CreateMatViewStmt, which seems like a lot. Perhaps instead of
opt_with_data we add a new production opt_with_no_or_old_data that can
return 3 alteratives (WITH [{NO|OLD}] DATA)?

```
+-- replace query but keep old data
+CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS
+  SELECT 5 AS a
+  WITH OLD DATA;
+SELECT * FROM mvtest_replace;
+REFRESH MATERIALIZED VIEW mvtest_replace;
+SELECT * FROM mvtest_replace;
+
```

We would like to see a test adding a new column while using WITH OLD
DATA. It is safe because every heap tuple knows how many attributes it
holds (and I tested it), but I did wonder if anyone had been tempted
to call fastgetattr directly for matviews. (Before your feature, it
would have been safe, I think.) Here is the test I did:

```
create materialized view mvnulls as
  select 1 c1, 2 c2, 3 c3, 4 c4, 5 c5, 6 c6, 7 c7, null c8;
-- add a ninth column (exceeding the old t_bits):
create or replace materialized view mvnulls as
  select 1 c1, 2 c2, 3 c3, 4 c4, 5 c5, 6 c6, 7 c7, null c8, null c9
with old data;
select c9 from mvnulls;
```

Another thing to think about and test: what if while using WITH OLD
DATA the new matview definition violates a unique index? Or a domain?
Are there any other constraints that can be applied to matviews to
consider?

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com