On Thu, 01 Aug 2024 11:31:53 -0700
Jeff Davis <pg...@j-davis.com> wrote:
> On Wed, 2024-07-31 at 18:20 +0900, Yugo NAGATA wrote:
> > I agree that it might not be important, but I think adding the flag
> > would be
> > also helpful for improving code-readability because it clarify the
> > function
> > is used in the two cases. I attached patch for this fix (patch 0003).
>
> Committed with one minor modification: I moved the boolean flag to be
> near the other booleans rather than at the end. Thank you.
>
> > Sure. I fixed the patch to remove 'param' from both functions. (patch
> > 0002)
>
> Committed, thank you.
Thank you for committing them.
Should not they be backported to REL_17_STABLE?
>
> > I also add the small refactoring around ExecCreateTableAs(). (patch
> > 0001)
> >
> > - Remove matview-related codes from intorel_startup.
> > Materialized views are no longer handled in this function.
> >
> > - RefreshMatViewByOid is moved to just after create_ctas_nodata
> > call to improve code readability.
> >
>
> I'm not sure the changes in intorel_startup() are correct. I tried
> adding an Assert(into->viewQuery == NULL), and it fails because there's
> another path I did not consider: "EXPLAIN ANALYZE CREATE MATERIALIZED
> VIEW ...", which does not go through ExecCreateTableAs() but does go
> through CreateIntoRelDestReceiver().
>
> See:
>
> https://postgr.es/m/20444c382e6cb5e21e93c94d679d0198b0dba4dd.ca...@j-davis.com
>
> Should we refactor a bit and try to make EXPLAIN use the same code
> paths?
I overlooked that CreateIntoRelDestReceiver() is used from EXPLAIN. I saw the
thread above and I agree that we should refactor it to make EXPLAIN consistent
CREATE MATERIALIZED VIEW, but I suppose this should be discussed the other
thread.
I attached a updated patch removed the intorel_startup() part from.
Regards,
Yugo Nagata
--
Yugo NAGATA <nag...@sraoss.co.jp>
>From 3609e85c288726e16dc851996a3b99e6179f43db Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
Date: Fri, 2 Aug 2024 16:02:07 +0900
Subject: [PATCH v3] Small refactoring around ExecCreateTableAs
Since commit b4da732f, the refresh logic is used to populate
materialized views, so we can simplify the error message
in ExecCreateTableAs.
Also, RefreshMatViewByOid is moved to just after create_ctas_nodata
call to improve code readability.
---
src/backend/commands/createas.c | 34 ++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 36e192b79b..c71ff80188 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -228,9 +228,6 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
bool do_refresh = false;
DestReceiver *dest;
ObjectAddress address;
- List *rewritten;
- PlannedStmt *plan;
- QueryDesc *queryDesc;
/* Check if the relation exists or not */
if (CreateTableAsRelExists(stmt))
@@ -279,9 +276,25 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
* from running the planner before all dependencies are set up.
*/
address = create_ctas_nodata(query->targetList, into);
+
+ /*
+ * For materialized views, reuse the REFRESH logic, which locks down
+ * security-restricted operations and restricts the search_path. This
+ * reduces the chance that a subsequent refresh will fail.
+ */
+ if (do_refresh)
+ RefreshMatViewByOid(address.objectId, true, false, false,
+ pstate->p_sourcetext, qc);
+
}
else
{
+ List *rewritten;
+ PlannedStmt *plan;
+ QueryDesc *queryDesc;
+
+ Assert(!is_matview);
+
/*
* Parse analysis was done already, but we still have to run the rule
* rewriter. We do not do AcquireRewriteLocks: we assume the query
@@ -292,9 +305,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
/* 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");
+ elog(ERROR, "unexpected rewrite result for CREATE TABLE AS SELECT");
query = linitial_node(Query, rewritten);
Assert(query->commandType == CMD_SELECT);
@@ -339,17 +350,6 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
PopActiveSnapshot();
}
- /*
- * For materialized views, reuse the REFRESH logic, which locks down
- * security-restricted operations and restricts the search_path. This
- * reduces the chance that a subsequent refresh will fail.
- */
- if (do_refresh)
- {
- RefreshMatViewByOid(address.objectId, true, false, false,
- pstate->p_sourcetext, qc);
- }
-
return address;
}
--
2.34.1
