Re: MAINTAIN privilege -- what do we need to un-revert it?

Hi,

On Fri, 26 Jul 2024 16:47:23 -0700
Jeff Davis <pgsql@j-davis.com> wrote:

> 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.
> > 
> >         /* SELECT should never rewrite to more or less than one
> > SELECT query */
> >         if (list_length(rewritten) != 1)
> >             elog(ERROR, "unexpected rewrite result for %s",
> >                  is_matview ? "CREATE MATERIALIZED VIEW" :
> >                  "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.

Thank you for looking into the pach.

I agree that it might not be important, but I think adding the flag would be
also helpful for improving code-readability because it clarify the function
is used in the two cases. I attached patch for this fix (patch 0003).

> > 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.

Sure. I fixed the patch to remove 'param' from both functions. (patch 0002)

I also add the small refactoring around ExecCreateTableAs(). (patch 0001)

- Remove matview-related codes from intorel_startup.
  Materialized views are no longer handled in this function.

- RefreshMatViewByOid is moved to just after create_ctas_nodata
  call to improve code readability.


Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>