Really fix the ambiguity in REFRESH MATERIALIZED VIEW CONCURRENTLY.

Rather than trying to pick table aliases that won't conflict with
any possible user-defined matview column name, adjust the queries'
syntax so that the aliases are only used in places where they can't be
mistaken for column names.  Mostly this consists of writing "alias.*"
not just "alias", which adds clarity for humans as well as machines.
We do have the issue that "SELECT alias.*" acts differently from
"SELECT alias", but we can use the same hack ruleutils.c uses for
whole-row variables in SELECT lists: write "alias.*::compositetype".

We might as well revert to the original aliases after doing this;
they're a bit easier to read.

Like 75d66d10e, back-patch to all supported branches.

Discussion: https://postgr.es/m/2488325.1628261320@sss.pgh.pa.us

diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 9493b227b4b17b55d87a2d5ee4bc67262904268f..512b00bc656961a82255f112f144bcb9b4d324a8 100644 (file)
--- 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 4b3a2e0cb730637fe767b2730e60052cbba7201b..313c72a268551260114fe88684b7364ed769a7bc 100644 (file)
--- 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 4a4bd0d6b6199c94c361403cf4fadd32194a67f9..68b9ccfd4529964c2131b1d172dda803471daebf 100644 (file)
--- 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;