Re: CREATE OR REPLACE MATERIALIZED VIEW

Warda Bibi and Khoa Nguyen and I reviewed the v8 patch. I believe they
will share some of their own thoughts as well, but I wanted to send
what I have now:

On Sun, Jun 28, 2026 at 11:14 AM Erik Wienhold <ewie(at)ewie(dot)name> wrote:
>
> On 2026-02-03 01:19 +0100, Paul A Jungwirth wrote:
> > ```
> > @@ -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.
>
> I added the static CreateTableAsRelReplaceable to leave the existing
> CreateTableAsRelExists untouched. The former now only checks whether
> the relation exists and can be replaced. The logic in ExecCreateTableAs
> is changed to first check is_matview && into->replace.

This looks better to me. But I agree with Rafia's concern about
calling checkMembershipInCurrentExtension in both
CreateTableAsRelReplaceable and create_ctas_replace, and there being a
race condition.

Also I think you skipped our question about EXPLAIN here. In the OR
REPLACE case, we get a failure:

[v20devel:15432][2542702] postgres=# CREATE OR REPLACE MATERIALIZED
VIEW mx AS SELECT 2 AS a;
SELECT 1
Time: 3.429 ms
[v20devel:15432][2542702] postgres=# EXPLAIN CREATE OR REPLACE
MATERIALIZED VIEW mx AS SELECT 2 AS a;
ERROR: relation "mx" already exists

> > ```
> > 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.)
>
> When formatting it like errmsg(is_matview ? "" : "") the PO files only
> pick up the first of the two messages. Pulling out the ternary like
> this would work:
>
> is_matview ? errmsg(...) : errmsg(...)
>
> But instead I added function checkMatviewColumns to matview.c based on
> checkViewColumns to avoid the is_matview flag altogether. Both
> functions still implement the same checks. The very last comment in
> checkViewColumns mentions leaving column defaults in place. Since
> default values can't be set on matview columns I figured that it's best
> to provide these checks as separate functions to avoid confusion.

It seems like quite a lot of duplication between checkViewColumns and
checkMatviewColumns. I think having a flag is better here. Maybe
someone more experienced can weigh in about `errmsg("cannot drop
columns from %s", is_matview ? "materialized view" : "view")`. That
seems like the best option to me, but `is_matview ? errmsg(...) :
errmsg(...)` would be okay too.

> > ```
> > 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)?
>
> I've added production opt_with_no_or_old_data as suggested, backed by
> the new enum WithDataOption in parsenodes.h. With that, I replaced
> .keepData from v7 with just .data holding that new enum.
>
> Field .skipData is still in place but I think it should be migrated to
> that new .data field. I haven't done that in v8, though, to keep the
> patch focused.

Using opt_with_no_or_old_data seems like an improvement. I agree about
consolidating .skipData and .data, which seem to overlap. And the new
.data field is not set in any production except the new one, so
elsewhere it is wrong.

Also Claude Code found several new issues, which we want to pass on:

This causes a segfault:

```
CREATE FUNCTION noop_et() RETURNS event_trigger LANGUAGE plpgsql AS
$$ BEGIN RAISE NOTICE 'et fired: %', tg_tag; END $$;
CREATE EVENT TRIGGER et_end ON ddl_command_end EXECUTE FUNCTION noop_et();

CREATE MATERIALIZED VIEW m1 AS SELECT 1 AS a;
CREATE OR REPLACE MATERIALIZED VIEW m1 AS SELECT 2 AS a; -- crash
```

We should add it to the event_trigger regress suite.

Here is a backtrace:

```
#0 EventTriggerAlterTableRelid (objectId=16386) at event_trigger.c:1803
currentEventTriggerState->currentCommand->d.alterTable.objectId
= objectId;
#1 AlterTableInternal (relid=16386, ...) at tablecmds.c:4662
#2 create_ctas_replace (..., matviewOid=16386) at createas.c:341
#3 create_ctas_nodata (...) at createas.c:228
#4 ExecCreateTableAs (...) at createas.c:388
#5 ProcessUtilitySlow (...) at utility.c:1682
```

The problem is that in create_ctas_replace you have:

/* EventTriggerAlterTableStart called by ProcessUtilitySlow */
AlterTableInternal(matviewOid, atcmds, true);

But that comment isn't true. I think it's just copied from
DefineVirtualRelation? If you look in ProcessUtilitySlow, you can see
that T_ViewStmt does call EventTriggerAlterTable{Start,End}, but
T_CreateTableAsStmt doesn't.

But I don't think adding it is the right answer. For one thing, adding
it to ProcessUtilitySlow would call it for *all* CREATE TABLE AS
commands, and that would be a behavior change. More important, we
don't fire event triggers for purely internal ALTER TABLE commands.
You can see that T_RefreshMatViewStmt uses
EventTriggerInhibitCommandCollection to avoid them for the same
reason. I think that's the pattern we should follow.

It seems odd that we have these different command tags depending on
how you word the command:

```
CREATE MATERIALIZED VIEW m2 AS SELECT 1 AS a;
-> SELECT 1
CREATE OR REPLACE MATERIALIZED VIEW m2 AS SELECT 2 AS a;
-> REFRESH MATERIALIZED VIEW
CREATE OR REPLACE MATERIALIZED VIEW m2 AS SELECT 3 AS a WITH NO DATA;
-> REFRESH MATERIALIZED VIEW
CREATE OR REPLACE MATERIALIZED VIEW m2 AS SELECT 4 AS a WITH OLD DATA;
-> CREATE MATERIALIZED VIEW
```

The existing behavior is:

```
CREATE MATERIALIZED VIEW mt AS SELECT ... -> SELECT 3
CREATE MATERIALIZED VIEW mt AS SELECT ... WITH NO DATA -> CREATE
MATERIALIZED VIEW
```

I think replacing should give the same command tag as creating. So perhaps this:

```
CREATE MATERIALIZED VIEW m2 AS SELECT 1 AS a;
-> SELECT 1
CREATE OR REPLACE MATERIALIZED VIEW m2 AS SELECT 2 AS a;
-> SELECT 1
CREATE OR REPLACE MATERIALIZED VIEW m2 AS SELECT 3 AS a WITH NO DATA;
-> CREATE MATERIALIZED VIEW
CREATE OR REPLACE MATERIALIZED VIEW m2 AS SELECT 4 AS a WITH OLD DATA;
-> CREATE MATERIALIZED VIEW
```

The last one is hard to say, but WITH OLD DATA avoids writing new heap
files, just like WITH NO DATA, so it seems correct.

In the regress tests you can test this by adding `\set QUIET false`
(as in triggers.sql and for_portion_of.sql).

There are some issues with Tom's rule that OR REPLACE should give the
same result as if you said CREATE. Potentially moving the tablespace
(because the default differs from the old matview's value) and causing
a table rewrite is a big effect, so let's at least document that it
might happen. More seriously, this can leave your indexes stranded in
the old tablespace, different from the matview's. That's not a state
you can normally achieve, so it seems risky for us and confusing for
users. I'm sure we don't want to sign up for handling that case going
forward. So if the tablespace changes, should we rewrite the indexes
too?

CreateTableAsRelReplaceable is almost identical to
CreateTableAsRelExists. And we can't actually reach the ereport, can
we? And I don't think we want to raise that notice anyway. With CREATE
OR REPLACE FUNCTION we don't say "already exists"; we just replace it.
Maybe that could be an Assert or at least an elog?

The new tests don't drop mvtest_replace and mvtest_dom. Sometimes we
do that to test pg_upgrade, but I don't think they actually add
anything there, so they should get cleaned up.

The prototype for create_ctas_replace names its first parameter tlist,
but the definition has attrList.

```
@@ -232,8 +376,37 @@ ExecCreateTableAs(ParseState *pstate,
CreateTableAsStmt *stmt,
DestReceiver *dest;
ObjectAddress address;

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

/*
```

This return fails to call the post_parse_analyze_hook.

Should we add a CREATE OR REPLACE example to test_ddl_deparse?

We've tried to summarize and interpret Claude's findings as best we
can, but we've attached the raw Claude report as well if you're
interested. You can see that we didn't always agree 100%. Also it has
some suggested fixes for some of the problems it found, so that might
save you some time.

Yours,

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com