Message-ID: <a7ce2ffa94cb26e40acf0ed8de857425f4d74727.camel@j-davis.com>
Subject: Re: MAINTAIN privilege -- what do we need to un-revert it?
From: Jeff Davis <pgsql@j-davis.com>
To: Yugo Nagata <nagata@sraoss.co.jp>, Noah Misch <noah@leadboat.com>
Cc: Michael Paquier <michael@paquier.xyz>, Nathan Bossart
	 <nathandbossart@gmail.com>, pgsql-hackers@postgresql.org, Joe Conway
	 <mail@joeconway.com>, Robert Haas <robertmhaas@gmail.com>
Date: Fri, 26 Jul 2024 16:47:23 -0700
In-Reply-To: <20240726122630.70e889f63a4d7e26f8549de8@sraoss.co.jp>
References: <20240228165523.GA2435539@nathanxps13>
	 <31ae1bf73e8a1dae3ee7ef2dd43a9bbda1e1ab86.camel@j-davis.com>
	 <fe5d572b02d07d8e9940f9b518d900708e0b0166.camel@j-davis.com>
	 <20240630222344.db.nmisch@google.com> <ZozWqg0XCkiPCnop@paquier.xyz>
	 <23fac3b2a722373a52b92a12ee0f500821450f97.camel@j-davis.com>
	 <20240711125207.98.nmisch@google.com>
	 <d97b888293e7588164b602850969a4b7d9550cc5.camel@j-davis.com>
	 <20240712231149.0d.nmisch@google.com>
	 <8e0438821bddd4de198989e6ac0f9a2aa27cabeb.camel@j-davis.com>
	 <20240713214748.cb@rfd.leadboat.com.sranhm>
	 <20240726122630.70e889f63a4d7e26f8549de8@sraoss.co.jp>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/a7ce2ffa94cb26e40acf0ed8de857425f4d74727.camel%40j-davis.com>
Precedence: bulk

Hello,

Thank you for looking.

On Fri, 2024-07-26 at 12:26 +0900, Yugo Nagata wrote:
> Since this commit, matviews are no longer handled in
> ExecCreateTableAs, so the
> following error message has not to consider materialized view cases,
> and can be made simple.
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* SELECT should never rewrite=
 to more or less than one
> SELECT query */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (list_length(rewritten) !=
=3D 1)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 elog(E=
RROR, "unexpected rewrite result for %s",
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 is_matview ? "CREATE MATERIALIZED VIEW" :
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 "CREATE TABLE AS SELECT");

There's a similar error in refresh_matview_datafill(), and I suppose
that should account for the CREATE MATERIALIZED VIEW case. We could
pass an additional flag to RefreshMatViewByOid to indicate whether it's
a CREATE or REFRESH, but it's an internal error, so perhaps it's not
important.

> Another my question is why RefreshMatViewByOid has a ParamListInfo
> parameter.

I just passed the params through, but you're right, they aren't
referenced at all.

I looked at the history, and it appears to go all the way back to the
function's introduction in commit 3bf3ab8c56.

> I don't understand why ExecRefreshMatView has one, either, because
> currently
> materialized views may not be defined using bound parameters, which
> is checked
> in transformCreateTableAsStmt, and the param argument is not used at
> all. It might
> be unsafe to change the interface of ExecRefreshMatView since this is
> public for a
> long time, but I don't think the new interface RefreshMatViewByOid
> has to have this
> unused argument.

Extensions should be prepared for reasonable changes in these kinds of
functions between releases. Even if the signatures remain the same, the
parse structures may change, which creates similar incompatibilities.
So let's just get rid of the 'params' argument from both functions.

Regards,
	Jeff Davis