Re: Alias collision in `refresh materialized view concurrently`

Tom Lane <tgl@sss.pgh.pa.us>

From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Bernd Helmle <mailings@oopsware.de>, Mathis Rudolf <mathis.rudolf@credativ.de>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2021-08-06T14:48:40Z
Lists: pgsql-hackers
Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jun 02, 2021 at 03:44:45PM +0530, Bharath Rupireddy wrote:
>> Thanks.The changes with that approach are very minimal. PSA v5 and let
>> me know if any more changes are needed.

> Simple enough, so applied and back-patched.

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.
The originally complained-of issue was that a user-chosen column name
could collide with the query-chosen table name:

ERROR: column reference "mv" is ambiguous
LINE 1: ...alog.=) mv.mv AND newdata OPERATOR(pg_catalog.*=) mv) WHERE ...

This is true, but it's self-inflicted damage, because all you have
to do is write the query so that mv is clearly a table name:

... mv.mv AND newdata.* OPERATOR(pg_catalog.*=) mv.*) WHERE ...

AFAICT that works and generates the identical parse tree to the original
coding.  The only place touched by the patch where it's a bit difficult to
make the syntax unambiguous this way is

                    "CREATE TEMP TABLE %s AS "
                    "SELECT _$mv.ctid AS tid, _$newdata "
                    "FROM %s _$mv FULL JOIN %s _$newdata ON (",

because newdata.* would ordinarily get expanded to multiple columns
if it's at the top level of a SELECT list, and that's not what we want.
However, that's easily fixed using the same hack as in ruleutils.c's
get_variable: add a no-op cast to the table's rowtype.  So this
would look like

    appendStringInfo(&querybuf,
                     "CREATE TEMP TABLE %s AS "
                     "SELECT mv.ctid AS tid, newdata.*::%s "
                     "FROM %s mv FULL JOIN %s newdata ON (",
                     diffname, matviewname, matviewname, tempname);

Given that it took this long to notice the problem at all, maybe
this is not a fix to cram in on the weekend before the release wrap.
But I don't see why we need to settle for "mostly works" when
"always works" is barely any harder.

			regards, tom lane



Commits

  1. Really fix the ambiguity in REFRESH MATERIALIZED VIEW CONCURRENTLY.

  2. Reduce risks of conflicts in internal queries of REFRESH MATVIEW CONCURRENTLY