public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v29 6/7] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
6+ messages / 5 participants
[nested] [flat]
* [PATCH v29 6/7] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
@ 2020-04-01 01:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Justin Pryzby @ 2020-04-01 01:35 UTC (permalink / raw)
---
doc/src/sgml/ref/cluster.sgml | 12 ++++-
doc/src/sgml/ref/vacuum.sgml | 12 ++++-
src/backend/commands/cluster.c | 64 ++++++++++++++---------
src/backend/commands/matview.c | 3 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/vacuum.c | 46 +++++++---------
src/backend/postmaster/autovacuum.c | 1 +
src/include/commands/cluster.h | 6 ++-
src/include/commands/vacuum.h | 5 +-
src/test/regress/input/tablespace.source | 13 +++++
src/test/regress/output/tablespace.source | 20 +++++++
11 files changed, 123 insertions(+), 61 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index cbfc0582be..6781e3a025 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -28,6 +28,7 @@ CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable> [, ...
VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
+ INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
</synopsis>
</refsynopsisdiv>
@@ -105,6 +106,15 @@ CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable> [, ...
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INDEX_TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the table's indexes will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">table_name</replaceable></term>
<listitem>
@@ -141,7 +151,7 @@ CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable> [, ...
<term><replaceable class="parameter">new_tablespace</replaceable></term>
<listitem>
<para>
- The tablespace where the table will be rebuilt.
+ The tablespace where the table or its indexes will be rebuilt.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 5261a7c727..28cab119b6 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -36,6 +36,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ]
PARALLEL <replaceable class="parameter">integer</replaceable>
TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
+ INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -265,6 +266,15 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INDEX_TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the relation's indexes will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
@@ -314,7 +324,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
<term><replaceable class="parameter">new_tablespace</replaceable></term>
<listitem>
<para>
- The tablespace where the relation will be rebuilt.
+ The tablespace where the relation or its indexes will be rebuilt.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index b289a76d58..0f9f09a15a 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -71,7 +71,7 @@ typedef struct
static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose,
- Oid NewTableSpaceOid);
+ Oid NewTableSpaceOid, Oid NewIdxTableSpaceOid);
static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
@@ -107,9 +107,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
{
ListCell *lc;
int options = 0;
- /* Name and Oid of tablespace to use for clustered relation. */
- char *tablespaceName = NULL;
- Oid tablespaceOid = InvalidOid;
+ /* Name and Oid of tablespaces to use for clustered relations. */
+ char *tablespaceName = NULL,
+ *idxtablespaceName = NULL;
+ Oid tablespaceOid,
+ idxtablespaceOid;
/* Parse list of generic parameters not handled by the parser */
foreach(lc, stmt->params)
@@ -123,6 +125,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
options &= ~CLUOPT_VERBOSE;
else if (strcmp(opt->defname, "tablespace") == 0)
tablespaceName = defGetString(opt);
+ else if (strcmp(opt->defname, "index_tablespace") == 0)
+ idxtablespaceName = defGetString(opt);
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -131,18 +135,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
parser_errposition(pstate, opt->location)));
}
- /* Select tablespace Oid to use. */
- if (tablespaceName)
- {
- tablespaceOid = get_tablespace_oid(tablespaceName, false);
-
- /* Can't move a non-shared relation into pg_global */
- if (tablespaceOid == GLOBALTABLESPACE_OID)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move non-shared relation to tablespace \"%s\"",
- tablespaceName)));
- }
+ /* Get tablespaces to use. */
+ tablespaceOid = tablespaceName ?
+ get_tablespace_oid(tablespaceName, false) : InvalidOid;
+ idxtablespaceOid = idxtablespaceName ?
+ get_tablespace_oid(idxtablespaceName, false) : InvalidOid;
if (stmt->relation != NULL)
{
@@ -214,7 +211,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
/* Do the job. */
cluster_rel(tableOid, indexOid, options,
- tablespaceOid);
+ tablespaceOid, idxtablespaceOid);
}
else
{
@@ -264,7 +261,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
/* Do the job. */
cluster_rel(rvtc->tableOid, rvtc->indexOid,
options | CLUOPT_RECHECK,
- tablespaceOid);
+ tablespaceOid, idxtablespaceOid);
PopActiveSnapshot();
CommitTransactionCommand();
}
@@ -294,11 +291,12 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
* instead of index order. This is the new implementation of VACUUM FULL,
* and error messages should refer to the operation as VACUUM not CLUSTER.
*
- * "tablespaceOid" is the tablespace where the relation will be rebuilt, or
- * InvalidOid to use its current tablespace.
+ * "tablespaceOid" and "idxtablespaceOid" are the tablespaces where the relation
+ * and its indexes will be rebuilt, or InvalidOid to use their current
+ * tablespaces.
*/
void
-cluster_rel(Oid tableOid, Oid indexOid, int options, Oid tablespaceOid)
+cluster_rel(Oid tableOid, Oid indexOid, int options, Oid tablespaceOid, Oid idxtablespaceOid)
{
Relation OldHeap;
bool verbose = ((options & CLUOPT_VERBOSE) != 0);
@@ -466,7 +464,7 @@ cluster_rel(Oid tableOid, Oid indexOid, int options, Oid tablespaceOid)
/* rebuild_relation does all the dirty work */
rebuild_relation(OldHeap, indexOid, verbose,
- tablespaceOid);
+ tablespaceOid, idxtablespaceOid);
/* NB: rebuild_relation does table_close() on OldHeap */
@@ -615,10 +613,11 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
* NB: this routine closes OldHeap at the right time; caller should not.
*/
static void
-rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid)
+rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid, Oid NewIdxTablespaceOid)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid tableSpace = OldHeap->rd_rel->reltablespace;
+ Oid idxtableSpace;
Oid OIDNewHeap;
char relpersistence;
bool is_system_catalog;
@@ -628,7 +627,20 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace
/* Use new tablespace if passed. */
if (OidIsValid(NewTablespaceOid))
+ {
tableSpace = NewTablespaceOid;
+ /* It's not a shared catalog, so refuse to move it to shared tablespace */
+ if (tableSpace == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move non-shared relation to tablespace \"%s\"",
+ get_tablespace_name(tableSpace))));
+ }
+
+ if (OidIsValid(NewIdxTablespaceOid))
+ idxtableSpace = NewIdxTablespaceOid;
+ else
+ idxtableSpace = get_rel_tablespace(indexOid);
/* Mark the correct index as clustered */
if (OidIsValid(indexOid))
@@ -657,7 +669,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace
finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
swap_toast_by_content, false, true,
frozenXid, cutoffMulti,
- relpersistence);
+ relpersistence, idxtableSpace);
}
@@ -1405,7 +1417,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_internal,
TransactionId frozenXid,
MultiXactId cutoffMulti,
- char newrelpersistence)
+ char newrelpersistence, Oid idxtableSpace)
{
ObjectAddress object;
Oid mapped_tables[4];
@@ -1467,7 +1479,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
PROGRESS_CLUSTER_PHASE_REBUILD_INDEX);
- reindex_relation(OIDOldHeap, reindex_flags, 0, InvalidOid);
+ reindex_relation(OIDOldHeap, reindex_flags, 0, idxtableSpace);
/* Report that we are now doing clean up */
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index f80a9e96a9..68ea07cae5 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -844,7 +844,8 @@ static void
refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence)
{
finish_heap_swap(matviewOid, OIDNewHeap, false, false, true, true,
- RecentXmin, ReadNextMultiXactId(), relpersistence);
+ RecentXmin, ReadNextMultiXactId(), relpersistence,
+ InvalidOid);
}
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0461332e96..1abe4dce4f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5060,7 +5060,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
!OidIsValid(tab->newTableSpace),
RecentXmin,
ReadNextMultiXactId(),
- persistence);
+ persistence, InvalidOid);
}
else
{
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 6a8116dfa3..c95c7be3b6 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -109,9 +109,9 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
bool disable_page_skipping = false;
ListCell *lc;
- /* Name and Oid of tablespace to use for relations after VACUUM FULL. */
- char *tablespacename = NULL;
- Oid tablespaceOid = InvalidOid;
+ /* Tablespace to use for relations after VACUUM FULL. */
+ char *tablespacename = NULL,
+ *idxtablespacename = NULL;
/* Set default value */
params.index_cleanup = VACOPT_TERNARY_DEFAULT;
@@ -151,6 +151,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
params.truncate = get_vacopt_ternary_value(opt);
else if (strcmp(opt->defname, "tablespace") == 0)
tablespacename = defGetString(opt);
+ else if (strcmp(opt->defname, "index_tablespace") == 0)
+ idxtablespacename = defGetString(opt);
else if (strcmp(opt->defname, "parallel") == 0)
{
if (opt->arg == NULL)
@@ -211,26 +213,18 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("VACUUM FULL cannot be performed in parallel")));
- /* Get tablespace Oid to use. */
- if (tablespacename)
- {
- if ((params.options & VACOPT_FULL) == 0)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("incompatible TABLESPACE option"),
- errdetail("You can only use TABLESPACE with VACUUM FULL.")));
-
- tablespaceOid = get_tablespace_oid(tablespacename, false);
-
- /* Can't move a non-shared relation into pg_global */
- if (tablespaceOid == GLOBALTABLESPACE_OID)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move non-shared relation to tablespace \"%s\"",
- tablespacename)));
+ if ((params.options & VACOPT_FULL) == 0 &&
+ (tablespacename || idxtablespacename))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("incompatible TABLESPACE option"),
+ errdetail("You can only use TABLESPACE with VACUUM FULL.")));
- }
- params.tablespace_oid = tablespaceOid;
+ /* Get tablespace Oids to use. */
+ params.tablespace_oid = tablespacename ?
+ get_tablespace_oid(tablespacename, false) : InvalidOid;
+ params.idxtablespace_oid = idxtablespacename ?
+ get_tablespace_oid(idxtablespacename, false) : InvalidOid;
/*
* Make sure VACOPT_ANALYZE is specified if any column lists are present.
@@ -1733,8 +1727,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
Relation onerel;
LockRelId onerelid;
Oid toast_relid,
- save_userid,
- tablespaceOid = InvalidOid;
+ save_userid;
int save_sec_context;
int save_nestlevel;
@@ -1878,14 +1871,13 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
OidIsValid(params->tablespace_oid) &&
IsSystemRelation(onerel) && !allowSystemTableMods)
{
+ params->tablespace_oid = InvalidOid;
ereport(WARNING,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("skipping tablespace change of \"%s\"",
RelationGetRelationName(onerel)),
errdetail("Cannot move system relation, only VACUUM is performed.")));
}
- else
- tablespaceOid = params->tablespace_oid;
/*
* Get a session-level lock too. This will protect our access to the
@@ -1957,7 +1949,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
cluster_rel(relid, InvalidOid, cluster_options,
- tablespaceOid);
+ params->tablespace_oid, params->idxtablespace_oid);
}
else
table_relation_vacuum(onerel, params, vac_strategy);
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index e73e124335..aca1db0861 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2917,6 +2917,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
tab->at_params.is_wraparound = wraparound;
tab->at_params.log_min_duration = log_min_duration;
tab->at_params.tablespace_oid = InvalidOid;
+ tab->at_params.idxtablespace_oid = InvalidOid;
tab->at_vacuum_cost_limit = vac_cost_limit;
tab->at_vacuum_cost_delay = vac_cost_delay;
tab->at_relname = NULL;
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 30300a8f74..8e535d9511 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -21,7 +21,8 @@
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
extern void cluster_rel(Oid tableOid, Oid indexOid, int options,
- Oid tablespaceOid);
+ Oid tablespaceOid,
+ Oid indextablespaceOid);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal);
@@ -35,6 +36,7 @@ extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_internal,
TransactionId frozenXid,
MultiXactId minMulti,
- char newrelpersistence);
+ char newrelpersistence,
+ Oid idxtablespaceOid);
#endif /* CLUSTER_H */
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 4b5ac7145d..488d7540c7 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -229,8 +229,9 @@ typedef struct VacuumParams
* disabled.
*/
int nworkers;
- Oid tablespace_oid; /* tablespace to use for relations
- * rebuilt by VACUUM FULL */
+ /* tablespaces to use for relations rebuilt by VACUUM FULL */
+ Oid tablespace_oid;
+ Oid idxtablespace_oid;
} VacuumParams;
/* GUC parameters */
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index f4687f6bfb..3065e15e18 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -80,6 +80,19 @@ REINDEX (TABLESPACE pg_default) TABLE CONCURRENTLY regress_tblspace_test_tbl; --
SELECT relname FROM pg_class
WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+-- check CLUSTER with INDEX_TABLESPACE change to non-default location
+CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+-- check relations moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+-- check VACUUM with INDEX_TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+-- check relations moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+
+
-- create a schema we can use
CREATE SCHEMA testschema;
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index e0a5c48fe9..65c61ee373 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -114,6 +114,26 @@ WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspa
---------
(0 rows)
+-- check CLUSTER with INDEX_TABLESPACE change to non-default location
+CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+-- check relations moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+ relname
+-------------------------------
+ regress_tblspace_test_tbl_idx
+(1 row)
+
+-- check VACUUM with INDEX_TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+-- check relations moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+ relname
+---------
+(0 rows)
+
-- create a schema we can use
CREATE SCHEMA testschema;
-- try a table
--
2.17.0
--k1lZvvs/B4yU6o8G--
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: EphemeralNamedRelation and materialized view
@ 2024-11-15 08:36 Yugo NAGATA <[email protected]>
2024-11-18 04:49 ` Re: EphemeralNamedRelation and materialized view Michael Paquier <[email protected]>
2024-11-20 17:43 ` Re: EphemeralNamedRelation and materialized view Tom Lane <[email protected]>
2024-11-29 11:00 ` Re: EphemeralNamedRelation and materialized view Kirill Reshke <[email protected]>
0 siblings, 3 replies; 6+ messages in thread
From: Yugo NAGATA @ 2024-11-15 08:36 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: pgsql-hackers
On Sun, 03 Nov 2024 13:42:33 -0500
Tom Lane <[email protected]> wrote:
> Yugo Nagata <[email protected]> writes:
> > While looking into the commit b4da732fd64e936970f38c792f8b32c4bdf2bcd5,
> > I noticed that we can create a materialized view using Ephemeral Named
> > Relation in PostgreSQL 16 or earler.
>
> Yeah, we should reject that, but I feel like this patch is not
> ambitious enough, because the 17-and-up behavior isn't exactly
> polished either.
>
> I tried variants of this function in HEAD:
>
> 1. With "create table mv as select * from enr", it works and
> does what you'd expect.
>
> 2. With "create view mv as select * from enr", you get
>
> regression=# insert into tbl values (10);
> ERROR: relation "enr" does not exist
> LINE 1: create view mv as select * from enr
> ^
> QUERY: create view mv as select * from enr
> CONTEXT: PL/pgSQL function f() line 2 at SQL statement
> regression=# \errverbose
> ERROR: 42P01: relation "enr" does not exist
> LINE 1: create view mv as select * from enr
> ^
> QUERY: create view mv as select * from enr
> CONTEXT: PL/pgSQL function f() line 2 at SQL statement
> LOCATION: parserOpenTable, parse_relation.c:1452
>
> 3. With "create materialized view ..." you get
>
> regression=# insert into tbl values (10);
> ERROR: executor could not find named tuplestore "enr"
> CONTEXT: SQL statement "create materialized view mv as select * from enr"
> PL/pgSQL function f() line 2 at SQL statement
> regression=# \errverbose
> ERROR: XX000: executor could not find named tuplestore "enr"
> CONTEXT: SQL statement "create materialized view mv as select * from enr"
> PL/pgSQL function f() line 2 at SQL statement
> LOCATION: ExecInitNamedTuplestoreScan, nodeNamedtuplestorescan.c:107
>
> I don't think hitting an internal error is good enough.
> Why doesn't this case act like case 2?
I agree that raising an internal error is not enough. I attached a updated
patch that outputs a message saying that an ENR can't be used in a matview.
> You could even argue that case 2 isn't good enough either,
> and we should be delivering a specific error message saying
> that an ENR can't be used in a view/matview. To do that,
> we'd likely need to pass down the QueryEnvironment in more
> places not fewer.
We can raise a similar error for (not materialized) views by passing
QueryEnv to DefineView() (or in ealier stage) , but there are other
objects that can contain ENR in their definition, for examle, functions,
cursor, or RLS policies. Is it worth introducing this version of error
message for all these objects?
Regards,
Yugo Nagata
--
Yugo NAGATA <[email protected]>
Attachments:
[text/x-diff] 0001-Prohibit-materialized-views-to-use-ephemeral-named-r.patch (4.1K, ../../[email protected]/2-0001-Prohibit-materialized-views-to-use-ephemeral-named-r.patch)
download | inline diff:
From 6c72c884954a4223fad192ff021803bf7835e7d7 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <[email protected]>
Date: Fri, 15 Nov 2024 15:07:17 +0900
Subject: [PATCH] Prohibit materialized views to use ephemeral named relations
---
src/backend/parser/analyze.c | 11 ++++++--
src/backend/parser/parse_relation.c | 43 ++++++++++++++++++++++++++++-
src/include/parser/parse_relation.h | 1 +
3 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 506e063161..c96088c2cc 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -3125,15 +3125,20 @@ transformCreateTableAsStmt(ParseState *pstate, CreateTableAsStmt *stmt)
errmsg("materialized views must not use data-modifying statements in WITH")));
/*
- * Check whether any temporary database objects are used in the
- * creation query. It would be hard to refresh data or incrementally
- * maintain it if a source disappeared.
+ * Check whether any temporary database objects or ephemeral named
+ * relations are used in the creation query. It would be hard to refresh
+ * data or incrementally maintain it if a source disappeared.
*/
if (isQueryUsingTempRelation(query))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("materialized views must not use temporary tables or views")));
+ if (isQueryUsingENR(query))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("materialized views must not use ephemeral named relations")));
+
/*
* A materialized view would either need to save parameters for use in
* maintaining/loading the data or prohibit them entirely. The latter
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 8075b1b8a1..0e9cb7a1c6 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -102,6 +102,7 @@ static int specialAttNum(const char *attname);
static bool rte_visible_if_lateral(ParseState *pstate, RangeTblEntry *rte);
static bool rte_visible_if_qualified(ParseState *pstate, RangeTblEntry *rte);
static bool isQueryUsingTempRelation_walker(Node *node, void *context);
+static bool isQueryUsingENR_walker(Node *node, void *context);
/*
@@ -3893,7 +3894,7 @@ rte_visible_if_qualified(ParseState *pstate, RangeTblEntry *rte)
/*
* Examine a fully-parsed query, and return true iff any relation underlying
- * the query is a temporary relation (table, view, or materialized view).
+ * the query is an ephemeral named relation.
*/
bool
isQueryUsingTempRelation(Query *query)
@@ -3938,6 +3939,46 @@ isQueryUsingTempRelation_walker(Node *node, void *context)
context);
}
+/*
+ * Examine a fully-parsed query, and return true iff any relation underlying
+ * the query is an ephemeral named relation.
+ */
+bool
+isQueryUsingENR(Query *query)
+{
+ return isQueryUsingENR_walker((Node *) query, NULL);
+}
+
+static bool
+isQueryUsingENR_walker(Node *node, void *context)
+{
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, Query))
+ {
+ Query *query = (Query *) node;
+ ListCell *rtable;
+
+ foreach(rtable, query->rtable)
+ {
+ RangeTblEntry *rte = lfirst(rtable);
+
+ if (rte->rtekind == RTE_NAMEDTUPLESTORE)
+ return true;
+ }
+
+ return query_tree_walker(query,
+ isQueryUsingENR_walker,
+ context,
+ QTW_IGNORE_JOINALIASES);
+ }
+
+ return expression_tree_walker(node,
+ isQueryUsingENR_walker,
+ context);
+}
+
/*
* addRTEPermissionInfo
* Creates RTEPermissionInfo for a given RTE and adds it into the
diff --git a/src/include/parser/parse_relation.h b/src/include/parser/parse_relation.h
index 91fd8e243b..2849da7fbd 100644
--- a/src/include/parser/parse_relation.h
+++ b/src/include/parser/parse_relation.h
@@ -127,5 +127,6 @@ extern const NameData *attnumAttName(Relation rd, int attid);
extern Oid attnumTypeId(Relation rd, int attid);
extern Oid attnumCollationId(Relation rd, int attid);
extern bool isQueryUsingTempRelation(Query *query);
+extern bool isQueryUsingENR(Query *query);
#endif /* PARSE_RELATION_H */
--
2.34.1
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: EphemeralNamedRelation and materialized view
2024-11-15 08:36 Re: EphemeralNamedRelation and materialized view Yugo NAGATA <[email protected]>
@ 2024-11-18 04:49 ` Michael Paquier <[email protected]>
2 siblings, 0 replies; 6+ messages in thread
From: Michael Paquier @ 2024-11-18 04:49 UTC (permalink / raw)
To: Yugo NAGATA <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Fri, Nov 15, 2024 at 05:36:47PM +0900, Yugo NAGATA wrote:
> I agree that raising an internal error is not enough. I attached a updated
> patch that outputs a message saying that an ENR can't be used in a matview.
Hmm.. To get a better idea of the scope you are foreseeing here,
should this include some test coverage?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: EphemeralNamedRelation and materialized view
2024-11-15 08:36 Re: EphemeralNamedRelation and materialized view Yugo NAGATA <[email protected]>
@ 2024-11-20 17:43 ` Tom Lane <[email protected]>
2024-12-05 14:16 ` Re: EphemeralNamedRelation and materialized view Yugo NAGATA <[email protected]>
2 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2024-11-20 17:43 UTC (permalink / raw)
To: Yugo NAGATA <[email protected]>; +Cc: pgsql-hackers
Yugo NAGATA <[email protected]> writes:
>> You could even argue that case 2 isn't good enough either,
>> and we should be delivering a specific error message saying
>> that an ENR can't be used in a view/matview. To do that,
>> we'd likely need to pass down the QueryEnvironment in more
>> places not fewer.
> We can raise a similar error for (not materialized) views by passing
> QueryEnv to DefineView() (or in ealier stage) , but there are other
> objects that can contain ENR in their definition, for examle, functions,
> cursor, or RLS policies. Is it worth introducing this version of error
> message for all these objects?
If it's worth checking for here, why not in other cases?
I'm not sure I like using isQueryUsingTempRelation as a model,
because its existing use in transformCreateTableAsStmt seems
like mostly a hack. (And I definitely don't love introducing
yet another scan of the query.) It seems to me that we should
think about this, for MVs as well as those other object types,
as fundamentally a dependency problem. That is, the reason
we can't allow a reference to an ENR in a long-lived object
is that we have no catalog representation for the reference.
So that leads to thinking that the issue ought to be handled
in recordDependencyOnExpr and friends. If we see an ENR while
scanning a rangetable to extract dependencies, then complain.
This might be a bit messy to produce good error messages for,
though.
Speaking of error messages, I'm not sure that it's okay to
use the phrase "ephemeral named relation" in a user-facing
error message. We don't use that term in our documentation
AFAICS, except in some SPI documentation that most users
will never have read. In the context of triggers, "transition
relation" seems to be what the docs use.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: EphemeralNamedRelation and materialized view
2024-11-15 08:36 Re: EphemeralNamedRelation and materialized view Yugo NAGATA <[email protected]>
2024-11-20 17:43 ` Re: EphemeralNamedRelation and materialized view Tom Lane <[email protected]>
@ 2024-12-05 14:16 ` Yugo NAGATA <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Yugo NAGATA @ 2024-12-05 14:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: pgsql-hackers
On Wed, 20 Nov 2024 12:43:16 -0500
Tom Lane <[email protected]> wrote:
> Yugo NAGATA <[email protected]> writes:
> >> You could even argue that case 2 isn't good enough either,
> >> and we should be delivering a specific error message saying
> >> that an ENR can't be used in a view/matview. To do that,
> >> we'd likely need to pass down the QueryEnvironment in more
> >> places not fewer.
>
> > We can raise a similar error for (not materialized) views by passing
> > QueryEnv to DefineView() (or in ealier stage) , but there are other
> > objects that can contain ENR in their definition, for examle, functions,
> > cursor, or RLS policies. Is it worth introducing this version of error
> > message for all these objects?
>
> If it's worth checking for here, why not in other cases?
>
> I'm not sure I like using isQueryUsingTempRelation as a model,
> because its existing use in transformCreateTableAsStmt seems
> like mostly a hack. (And I definitely don't love introducing
> yet another scan of the query.) It seems to me that we should
> think about this, for MVs as well as those other object types,
> as fundamentally a dependency problem. That is, the reason
> we can't allow a reference to an ENR in a long-lived object
> is that we have no catalog representation for the reference.
> So that leads to thinking that the issue ought to be handled
> in recordDependencyOnExpr and friends. If we see an ENR while
> scanning a rangetable to extract dependencies, then complain.
> This might be a bit messy to produce good error messages for,
> though.
>
> Speaking of error messages, I'm not sure that it's okay to
> use the phrase "ephemeral named relation" in a user-facing
> error message. We don't use that term in our documentation
> AFAICS, except in some SPI documentation that most users
> will never have read. In the context of triggers, "transition
> relation" seems to be what the docs use.
Thank you for your suggestion.
I've attached a updated patch. Use of ENRs are now checked in
find_expr_references_walker() called from recordDependencyOnExpr().
The message is changed to "transition tables cannot be used rule"
because the view definition is stored in the pg_rewrite catalog as
a rule.
Regards,
Yugo Nagata
--
Yugo NAGATA <[email protected]>
Attachments:
[text/x-diff] v2-0001-Prohibit-materialized-views-to-use-ephemeral-name.patch (4.5K, ../../[email protected]/2-v2-0001-Prohibit-materialized-views-to-use-ephemeral-name.patch)
download | inline diff:
From 34d09775e92b0ac39cd8656d69164b13c92f6fa2 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <[email protected]>
Date: Mon, 2 Dec 2024 09:39:34 +0900
Subject: [PATCH v2] Prohibit materialized views to use ephemeral named
relations
---
src/backend/catalog/dependency.c | 24 ++++++++++++++++++++++++
src/test/regress/expected/triggers.out | 13 +++++++++++++
src/test/regress/sql/triggers.sql | 13 +++++++++++++
3 files changed, 50 insertions(+)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 2afc550540..f323e8c91e 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -138,6 +138,7 @@ typedef struct
/* for find_expr_references_walker */
typedef struct
{
+ const ObjectAddress *depender; /* depender object */
ObjectAddresses *addrs; /* addresses being accumulated */
List *rtables; /* list of rangetables to resolve Vars */
} find_expr_references_context;
@@ -1556,6 +1557,7 @@ recordDependencyOnExpr(const ObjectAddress *depender,
{
find_expr_references_context context;
+ context.depender = depender;
context.addrs = new_object_addresses();
/* Set up interpretation for Vars at varlevelsup = 0 */
@@ -1602,6 +1604,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
find_expr_references_context context;
RangeTblEntry rte = {0};
+ context.depender = depender;
context.addrs = new_object_addresses();
/* We gin up a rather bogus rangetable list to handle Vars */
@@ -1739,6 +1742,17 @@ find_expr_references_walker(Node *node,
/* (done out of line, because it's a bit bulky) */
process_function_rte_ref(rte, var->varattno, context);
}
+ else if (rte->rtekind == RTE_NAMEDTUPLESTORE)
+ {
+ /*
+ * Catalog objects cannot depend on a transtion table which has
+ * no catalog representation.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("transition table cannot be used in %s",
+ getObjectTypeDescription(context->depender, true))));
+ }
/*
* Vars referencing other RTE types require no additional work. In
@@ -2191,6 +2205,16 @@ find_expr_references_walker(Node *node,
}
context->rtables = list_delete_first(context->rtables);
break;
+ case RTE_NAMEDTUPLESTORE:
+ /*
+ * Catalog objects cannot depend on a transtion table
+ * which has no catalog representation.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("transition table cannot be used in %s",
+ getObjectTypeDescription(context->depender, true))));
+ break;
default:
break;
}
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index a044d6afe2..e3522cf3bb 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -3728,3 +3728,16 @@ Inherits: parent
drop table parent, child;
drop function f();
+-- Verify prohibition of materialized views defined using tansition table
+create table my_table (a int);
+create function make_matview() returns trigger as
+$$ begin create materialized view mv as select * from new_table; return new; end $$
+language plpgsql;
+create trigger make_matview after insert on my_table
+referencing new table as new_table
+for each statement execute function make_matview();
+insert into my_table values (42);
+ERROR: transition tables cannot be used in rule
+CONTEXT: SQL statement "create materialized view mv as select * from new_table"
+PL/pgSQL function make_matview() line 1 at SQL statement
+drop table my_table;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 51610788b2..f83bd84e6c 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -2551,6 +2551,7 @@ merge into merge_target_table t
using merge_source_table s
on t.a = s.a
when matched and s.a <= 2 then
+
update set b = t.b || ' updated by merge'
when matched and s.a > 2 then
delete
@@ -2820,3 +2821,15 @@ alter trigger parenttrig on parent rename to anothertrig;
drop table parent, child;
drop function f();
+
+-- Verify prohibition of materialized views defined using tansition table
+
+create table my_table (a int);
+create function make_matview() returns trigger as
+$$ begin create materialized view mv as select * from new_table; return new; end $$
+language plpgsql;
+create trigger make_matview after insert on my_table
+referencing new table as new_table
+for each statement execute function make_matview();
+insert into my_table values (42);
+drop table my_table;
--
2.34.1
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: EphemeralNamedRelation and materialized view
2024-11-15 08:36 Re: EphemeralNamedRelation and materialized view Yugo NAGATA <[email protected]>
@ 2024-11-29 11:00 ` Kirill Reshke <[email protected]>
2 siblings, 0 replies; 6+ messages in thread
From: Kirill Reshke @ 2024-11-29 11:00 UTC (permalink / raw)
To: Yugo NAGATA <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Fri, 15 Nov 2024 at 13:37, Yugo NAGATA <[email protected]> wrote:
>
> On Sun, 03 Nov 2024 13:42:33 -0500
> Tom Lane <[email protected]> wrote:
>
> > Yugo Nagata <[email protected]> writes:
> > > While looking into the commit b4da732fd64e936970f38c792f8b32c4bdf2bcd5,
> > > I noticed that we can create a materialized view using Ephemeral Named
> > > Relation in PostgreSQL 16 or earler.
> >
> > Yeah, we should reject that, but I feel like this patch is not
> > ambitious enough, because the 17-and-up behavior isn't exactly
> > polished either.
> >
> > I tried variants of this function in HEAD:
> >
> > 1. With "create table mv as select * from enr", it works and
> > does what you'd expect.
> >
> > 2. With "create view mv as select * from enr", you get
> >
> > regression=# insert into tbl values (10);
> > ERROR: relation "enr" does not exist
> > LINE 1: create view mv as select * from enr
> > ^
> > QUERY: create view mv as select * from enr
> > CONTEXT: PL/pgSQL function f() line 2 at SQL statement
> > regression=# \errverbose
> > ERROR: 42P01: relation "enr" does not exist
> > LINE 1: create view mv as select * from enr
> > ^
> > QUERY: create view mv as select * from enr
> > CONTEXT: PL/pgSQL function f() line 2 at SQL statement
> > LOCATION: parserOpenTable, parse_relation.c:1452
> >
> > 3. With "create materialized view ..." you get
> >
> > regression=# insert into tbl values (10);
> > ERROR: executor could not find named tuplestore "enr"
> > CONTEXT: SQL statement "create materialized view mv as select * from enr"
> > PL/pgSQL function f() line 2 at SQL statement
> > regression=# \errverbose
> > ERROR: XX000: executor could not find named tuplestore "enr"
> > CONTEXT: SQL statement "create materialized view mv as select * from enr"
> > PL/pgSQL function f() line 2 at SQL statement
> > LOCATION: ExecInitNamedTuplestoreScan, nodeNamedtuplestorescan.c:107
> >
> > I don't think hitting an internal error is good enough.
> > Why doesn't this case act like case 2?
>
> I agree that raising an internal error is not enough. I attached a updated
> patch that outputs a message saying that an ENR can't be used in a matview.
>
> > You could even argue that case 2 isn't good enough either,
> > and we should be delivering a specific error message saying
> > that an ENR can't be used in a view/matview. To do that,
> > we'd likely need to pass down the QueryEnvironment in more
> > places not fewer.
>
> We can raise a similar error for (not materialized) views by passing
> QueryEnv to DefineView() (or in ealier stage) , but there are other
> objects that can contain ENR in their definition, for examle, functions,
> cursor, or RLS policies. Is it worth introducing this version of error
> message for all these objects?
>
> Regards,
> Yugo Nagata
>
> --
> Yugo NAGATA <[email protected]>
Hi!
There are review comments that need to be addressed.
Commitfest status is now waiting on the author.
[0] https://www.postgresql.org/message-id/ZzrHUEaWB67EAZpW%40paquier.xyz
[1] https://www.postgresql.org/message-id/222722.1732124596%40sss.pgh.pa.us
--
Best regards,
Kirill Reshke
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-12-05 14:16 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 01:35 [PATCH v29 6/7] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>) Justin Pryzby <[email protected]>
2024-11-15 08:36 Re: EphemeralNamedRelation and materialized view Yugo NAGATA <[email protected]>
2024-11-18 04:49 ` Re: EphemeralNamedRelation and materialized view Michael Paquier <[email protected]>
2024-11-20 17:43 ` Re: EphemeralNamedRelation and materialized view Tom Lane <[email protected]>
2024-12-05 14:16 ` Re: EphemeralNamedRelation and materialized view Yugo NAGATA <[email protected]>
2024-11-29 11:00 ` Re: EphemeralNamedRelation and materialized view Kirill Reshke <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox