I wrote:
> I just came across this issue while preparing the release notes.
> ISTM that people have expended a great deal of effort on a fundamentally
> unreliable solution, when a reliable one is easily available.
Concretely, I propose the attached.
regards, tom lane
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 9493b227b4..512b00bc65 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -537,9 +537,12 @@ transientrel_destroy(DestReceiver *self)
/*
* Given a qualified temporary table name, append an underscore followed by
* the given integer, to make a new table name based on the old one.
+ * The result is a palloc'd string.
*
- * This leaks memory through palloc(), which won't be cleaned up until the
- * current memory context is freed.
+ * As coded, this would fail to make a valid SQL name if the given name were,
+ * say, "FOO"."BAR". Currently, the table name portion of the input will
+ * never be double-quoted because it's of the form "pg_temp_NNN", cf
+ * make_new_heap(). But we might have to work harder someday.
*/
static char *
make_temptable_name_n(char *tempname, int n)
@@ -627,16 +630,20 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
* that in a way that allows showing the first duplicated row found. Even
* after we pass this test, a unique index on the materialized view may
* find a duplicate key problem.
+ *
+ * Note: here and below, we use "tablename.*::tablerowtype" as a hack to
+ * keep ".*" from being expanded into multiple columns in a SELECT list.
+ * Compare ruleutils.c's get_variable().
*/
resetStringInfo(&querybuf);
appendStringInfo(&querybuf,
- "SELECT _$newdata FROM %s _$newdata "
- "WHERE _$newdata IS NOT NULL AND EXISTS "
- "(SELECT 1 FROM %s _$newdata2 WHERE _$newdata2 IS NOT NULL "
- "AND _$newdata2 OPERATOR(pg_catalog.*=) _$newdata "
- "AND _$newdata2.ctid OPERATOR(pg_catalog.<>) "
- "_$newdata.ctid)",
- tempname, tempname);
+ "SELECT newdata.*::%s FROM %s newdata "
+ "WHERE newdata.* IS NOT NULL AND EXISTS "
+ "(SELECT 1 FROM %s newdata2 WHERE newdata2.* IS NOT NULL "
+ "AND newdata2.* OPERATOR(pg_catalog.*=) newdata.* "
+ "AND newdata2.ctid OPERATOR(pg_catalog.<>) "
+ "newdata.ctid)",
+ tempname, tempname, tempname);
if (SPI_execute(querybuf.data, false, 1) != SPI_OK_SELECT)
elog(ERROR, "SPI_exec failed: %s", querybuf.data);
if (SPI_processed > 0)
@@ -663,9 +670,9 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
resetStringInfo(&querybuf);
appendStringInfo(&querybuf,
"CREATE TEMP TABLE %s AS "
- "SELECT _$mv.ctid AS tid, _$newdata "
- "FROM %s _$mv FULL JOIN %s _$newdata ON (",
- diffname, matviewname, tempname);
+ "SELECT mv.ctid AS tid, newdata.*::%s AS newdata "
+ "FROM %s mv FULL JOIN %s newdata ON (",
+ diffname, tempname, matviewname, tempname);
/*
* Get the list of index OIDs for the table from the relcache, and look up
@@ -757,9 +764,9 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
if (foundUniqueIndex)
appendStringInfoString(&querybuf, " AND ");
- leftop = quote_qualified_identifier("_$newdata",
+ leftop = quote_qualified_identifier("newdata",
NameStr(attr->attname));
- rightop = quote_qualified_identifier("_$mv",
+ rightop = quote_qualified_identifier("mv",
NameStr(attr->attname));
generate_operator_clause(&querybuf,
@@ -787,8 +794,8 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
Assert(foundUniqueIndex);
appendStringInfoString(&querybuf,
- " AND _$newdata OPERATOR(pg_catalog.*=) _$mv) "
- "WHERE _$newdata IS NULL OR _$mv IS NULL "
+ " AND newdata.* OPERATOR(pg_catalog.*=) mv.*) "
+ "WHERE newdata.* IS NULL OR mv.* IS NULL "
"ORDER BY tid");
/* Create the temporary "diff" table. */
@@ -814,10 +821,10 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
/* Deletes must come before inserts; do them first. */
resetStringInfo(&querybuf);
appendStringInfo(&querybuf,
- "DELETE FROM %s _$mv WHERE ctid OPERATOR(pg_catalog.=) ANY "
- "(SELECT _$diff.tid FROM %s _$diff "
- "WHERE _$diff.tid IS NOT NULL "
- "AND _$diff._$newdata IS NULL)",
+ "DELETE FROM %s mv WHERE ctid OPERATOR(pg_catalog.=) ANY "
+ "(SELECT diff.tid FROM %s diff "
+ "WHERE diff.tid IS NOT NULL "
+ "AND diff.newdata IS NULL)",
matviewname, diffname);
if (SPI_exec(querybuf.data, 0) != SPI_OK_DELETE)
elog(ERROR, "SPI_exec failed: %s", querybuf.data);
@@ -825,8 +832,8 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
/* Inserts go last. */
resetStringInfo(&querybuf);
appendStringInfo(&querybuf,
- "INSERT INTO %s SELECT (_$diff._$newdata).* "
- "FROM %s _$diff WHERE tid IS NULL",
+ "INSERT INTO %s SELECT (diff.newdata).* "
+ "FROM %s diff WHERE tid IS NULL",
matviewname, diffname);
if (SPI_exec(querybuf.data, 0) != SPI_OK_INSERT)
elog(ERROR, "SPI_exec failed: %s", querybuf.data);
diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
index 4b3a2e0cb7..313c72a268 100644
--- a/src/test/regress/expected/matview.out
+++ b/src/test/regress/expected/matview.out
@@ -551,7 +551,15 @@ NOTICE: drop cascades to materialized view mvtest_mv_v
-- make sure running as superuser works when MV owned by another role (bug #11208)
CREATE ROLE regress_user_mvtest;
SET ROLE regress_user_mvtest;
-CREATE TABLE mvtest_foo_data AS SELECT i, md5(random()::text)
+-- this test case also checks for ambiguity in the queries issued by
+-- refresh_by_match_merge(), by choosing column names that intentionally
+-- duplicate all the aliases used in those queries
+CREATE TABLE mvtest_foo_data AS SELECT i,
+ i+1 AS tid,
+ md5(random()::text) AS mv,
+ md5(random()::text) AS newdata,
+ md5(random()::text) AS newdata2,
+ md5(random()::text) AS diff
FROM generate_series(1, 10) i;
CREATE MATERIALIZED VIEW mvtest_mv_foo AS SELECT * FROM mvtest_foo_data;
CREATE MATERIALIZED VIEW mvtest_mv_foo AS SELECT * FROM mvtest_foo_data;
diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql
index 4a4bd0d6b6..68b9ccfd45 100644
--- a/src/test/regress/sql/matview.sql
+++ b/src/test/regress/sql/matview.sql
@@ -211,7 +211,15 @@ DROP TABLE mvtest_v CASCADE;
-- make sure running as superuser works when MV owned by another role (bug #11208)
CREATE ROLE regress_user_mvtest;
SET ROLE regress_user_mvtest;
-CREATE TABLE mvtest_foo_data AS SELECT i, md5(random()::text)
+-- this test case also checks for ambiguity in the queries issued by
+-- refresh_by_match_merge(), by choosing column names that intentionally
+-- duplicate all the aliases used in those queries
+CREATE TABLE mvtest_foo_data AS SELECT i,
+ i+1 AS tid,
+ md5(random()::text) AS mv,
+ md5(random()::text) AS newdata,
+ md5(random()::text) AS newdata2,
+ md5(random()::text) AS diff
FROM generate_series(1, 10) i;
CREATE MATERIALIZED VIEW mvtest_mv_foo AS SELECT * FROM mvtest_foo_data;
CREATE MATERIALIZED VIEW mvtest_mv_foo AS SELECT * FROM mvtest_foo_data;
