Re: Alias collision in `refresh materialized view concurrently` - Mailing list pgsql-hackers

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;