Hi,
On Fri, 26 Jul 2024 16:47:23 -0700
Jeff Davis <pg...@j-davis.com> wrote:
> Hello,
>
> Thank you for looking.
>
> On Fri, 2024-07-26 at 12:26 +0900, Yugo Nagata wrote:
> > Since this commit, matviews are no longer handled in
> > ExecCreateTableAs, so the
> > following error message has not to consider materialized view cases,
> > and can be made simple.
> >
> > /* 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");
>
> There's a similar error in refresh_matview_datafill(), and I suppose
> that should account for the CREATE MATERIALIZED VIEW case. We could
> pass an additional flag to RefreshMatViewByOid to indicate whether it's
> a CREATE or REFRESH, but it's an internal error, so perhaps it's not
> important.
Thank you for looking into the pach.
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).
> > Another my question is why RefreshMatViewByOid has a ParamListInfo
> > parameter.
>
> I just passed the params through, but you're right, they aren't
> referenced at all.
>
> I looked at the history, and it appears to go all the way back to the
> function's introduction in commit 3bf3ab8c56.
>
> > I don't understand why ExecRefreshMatView has one, either, because
> > currently
> > materialized views may not be defined using bound parameters, which
> > is checked
> > in transformCreateTableAsStmt, and the param argument is not used at
> > all. It might
> > be unsafe to change the interface of ExecRefreshMatView since this is
> > public for a
> > long time, but I don't think the new interface RefreshMatViewByOid
> > has to have this
> > unused argument.
>
> Extensions should be prepared for reasonable changes in these kinds of
> functions between releases. Even if the signatures remain the same, the
> parse structures may change, which creates similar incompatibilities.
> So let's just get rid of the 'params' argument from both functions.
Sure. I fixed the patch to remove 'param' from both functions. (patch 0002)
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.
Regards,
Yugo Nagata
--
Yugo NAGATA <nag...@sraoss.co.jp>
>From e6027d1e417b446d09411a98fca11a831c6b7b03 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
Date: Wed, 31 Jul 2024 17:22:41 +0900
Subject: [PATCH v2 3/3] Add a flag to RefreshMatviewByOid to indicate whether
it is CREATE or not
RefreshMatviewByOid is used for both REFRESH and CREATE MATERIALIZED VIEW.
This flag is currently just used for handling internal error messages,
but also aimed to improve code-readability.
---
src/backend/commands/matview.c | 38 +++++++++++++++++++++++++---------
src/include/commands/matview.h | 3 ++-
2 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 8644ad695c..29a80fe565 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -60,7 +60,7 @@ static bool transientrel_receive(TupleTableSlot *slot, DestReceiver *self);
static void transientrel_shutdown(DestReceiver *self);
static void transientrel_destroy(DestReceiver *self);
static uint64 refresh_matview_datafill(DestReceiver *dest, Query *query,
- const char *queryString);
+ const char *queryString, bool is_create);
static char *make_temptable_name_n(char *tempname, int n);
static void refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
int save_sec_context);
@@ -136,7 +136,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
NULL);
return RefreshMatViewByOid(matviewOid, stmt->skipData, stmt->concurrent,
- queryString, qc);
+ queryString, qc, false);
}
/*
@@ -157,10 +157,14 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
*
* The matview's "populated" state is changed based on whether the contents
* reflect the result set of the materialized view's query.
+ *
+ * This is also used to populate the materialized view created by CREATE
+ * MATERIALIZED VIEW command.
*/
ObjectAddress
RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, QueryCompletion *qc)
+ const char *queryString, QueryCompletion *qc,
+ bool is_create)
{
Relation matviewRel;
RewriteRule *rule;
@@ -169,7 +173,6 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
Oid tableSpace;
Oid relowner;
Oid OIDNewHeap;
- DestReceiver *dest;
uint64 processed = 0;
char relpersistence;
Oid save_userid;
@@ -248,6 +251,8 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
ListCell *indexoidscan;
bool hasUniqueIndex = false;
+ Assert(!is_create);
+
foreach(indexoidscan, indexoidlist)
{
Oid indexoid = lfirst_oid(indexoidscan);
@@ -284,7 +289,9 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
* NB: We count on this to protect us against problems with refreshing the
* data using TABLE_INSERT_FROZEN.
*/
- CheckTableNotInUse(matviewRel, "REFRESH MATERIALIZED VIEW");
+ CheckTableNotInUse(matviewRel,
+ is_create ? "CREATE MATERIALIZED VIEW" :
+ "REFRESH MATERIALIZED VIEW");
/*
* Tentatively mark the matview as populated or not (this will roll back
@@ -313,11 +320,16 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
matviewRel->rd_rel->relam,
relpersistence, ExclusiveLock);
LockRelationOid(OIDNewHeap, AccessExclusiveLock);
- dest = CreateTransientRelDestReceiver(OIDNewHeap);
/* Generate the data, if wanted. */
if (!skipData)
- processed = refresh_matview_datafill(dest, dataQuery, queryString);
+ {
+ DestReceiver *dest;
+
+ dest = CreateTransientRelDestReceiver(OIDNewHeap);
+ processed = refresh_matview_datafill(dest, dataQuery, queryString,
+ is_create);
+ }
/* Make the matview match the newly generated data. */
if (concurrent)
@@ -369,9 +381,14 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
* i.e., the display_rowcount flag of CMDTAG_REFRESH_MATERIALIZED_VIEW
* command tag is left false in cmdtaglist.h. Otherwise, the change of
* completion tag output might break applications using it.
+ *
+ * When called from CREATE MATERIALIZED VIEW comand, the rowcount is
+ * displayed with the command tag CMDTAG_SELECT.
*/
if (qc)
- SetQueryCompletion(qc, CMDTAG_REFRESH_MATERIALIZED_VIEW, processed);
+ SetQueryCompletion(qc,
+ is_create ? CMDTAG_SELECT : CMDTAG_REFRESH_MATERIALIZED_VIEW,
+ processed);
return address;
}
@@ -386,7 +403,7 @@ RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
*/
static uint64
refresh_matview_datafill(DestReceiver *dest, Query *query,
- const char *queryString)
+ const char *queryString, bool is_create)
{
List *rewritten;
PlannedStmt *plan;
@@ -401,7 +418,8 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
/* SELECT should never rewrite to more or less than one SELECT query */
if (list_length(rewritten) != 1)
- elog(ERROR, "unexpected rewrite result for REFRESH MATERIALIZED VIEW");
+ elog(ERROR, "unexpected rewrite result for %s",
+ is_create ? "CREATE MATERIALIZED VIEW ": "REFRESH MATERIALIZED VIEW");
query = (Query *) linitial(rewritten);
/* Check for user-requested abort. */
diff --git a/src/include/commands/matview.h b/src/include/commands/matview.h
index 7916df3039..f95e468d88 100644
--- a/src/include/commands/matview.h
+++ b/src/include/commands/matview.h
@@ -26,7 +26,8 @@ extern void SetMatViewPopulatedState(Relation relation, bool newstate);
extern ObjectAddress ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
QueryCompletion *qc);
extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, QueryCompletion *qc);
+ const char *queryString, QueryCompletion *qc,
+ bool is_create);
extern DestReceiver *CreateTransientRelDestReceiver(Oid transientoid);
--
2.34.1
>From f314d87bedfd905aafbacdcade9dabd2f6ee384b Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
Date: Fri, 26 Jul 2024 12:02:56 +0900
Subject: [PATCH v2 2/3] Remove ParamListInfo argument from RefreshMatViewByOid
and ExecRefreshMatView
This argument is not used at all, so we can remove it.
---
src/backend/commands/matview.c | 7 +++----
src/backend/tcop/utility.c | 2 +-
src/include/commands/matview.h | 5 ++---
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 6bb64be274..8644ad695c 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -119,7 +119,7 @@ SetMatViewPopulatedState(Relation relation, bool newstate)
*/
ObjectAddress
ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
- ParamListInfo params, QueryCompletion *qc)
+ QueryCompletion *qc)
{
Oid matviewOid;
LOCKMODE lockmode;
@@ -136,7 +136,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
NULL);
return RefreshMatViewByOid(matviewOid, stmt->skipData, stmt->concurrent,
- queryString, params, qc);
+ queryString, qc);
}
/*
@@ -160,8 +160,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
*/
ObjectAddress
RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, ParamListInfo params,
- QueryCompletion *qc)
+ const char *queryString, QueryCompletion *qc)
{
Relation matviewRel;
RewriteRule *rule;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index fa66b8017e..702a6c3a0b 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1688,7 +1688,7 @@ ProcessUtilitySlow(ParseState *pstate,
PG_TRY(2);
{
address = ExecRefreshMatView((RefreshMatViewStmt *) parsetree,
- queryString, params, qc);
+ queryString, qc);
}
PG_FINALLY(2);
{
diff --git a/src/include/commands/matview.h b/src/include/commands/matview.h
index a226b2e68f..7916df3039 100644
--- a/src/include/commands/matview.h
+++ b/src/include/commands/matview.h
@@ -24,10 +24,9 @@
extern void SetMatViewPopulatedState(Relation relation, bool newstate);
extern ObjectAddress ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
- ParamListInfo params, QueryCompletion *qc);
+ QueryCompletion *qc);
extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
- const char *queryString, ParamListInfo params,
- QueryCompletion *qc);
+ const char *queryString, QueryCompletion *qc);
extern DestReceiver *CreateTransientRelDestReceiver(Oid transientoid);
--
2.34.1
>From 616f674c81ae5aa07fad40f976050e56ffd1c339 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
Date: Wed, 31 Jul 2024 17:34:15 +0900
Subject: [PATCH v2 1/3] 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.
In addition, we can remove matview-related codes from
intorel_startup because materialized views are no longer handled
in this function.
Also, RefreshMatViewByOid is moved to just after create_ctas_nodata
call to improve code readability.
---
src/backend/commands/createas.c | 47 +++++++++++----------------------
1 file changed, 16 insertions(+), 31 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 2c8a93b6e5..68d5047924 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,24 @@ 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, false, false,
+ pstate->p_sourcetext, NULL 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 +304,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,20 +349,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, false, false,
- pstate->p_sourcetext, NULL, qc);
-
- if (qc)
- qc->commandTag = CMDTAG_SELECT;
- }
-
return address;
}
@@ -453,7 +449,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
{
DR_intorel *myState = (DR_intorel *) self;
IntoClause *into = myState->into;
- bool is_matview;
List *attrList;
ObjectAddress intoRelationAddr;
Relation intoRelationDesc;
@@ -462,9 +457,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
Assert(into != NULL); /* else somebody forgot to set it */
- /* This code supports both CREATE TABLE AS and CREATE MATERIALIZED VIEW */
- is_matview = (into->viewQuery != NULL);
-
/*
* Build column definitions using "pre-cooked" type and collation info. If
* a column name list was specified in CREATE TABLE AS, override the
@@ -538,13 +530,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("policies not yet implemented for this command")));
- /*
- * Tentatively mark the target as populated, if it's a matview and we're
- * going to fill it; otherwise, no change needed.
- */
- if (is_matview && !into->skipData)
- SetMatViewPopulatedState(intoRelationDesc, true);
-
/*
* Fill private fields of myState for use by later routines
*/
--
2.34.1
