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>
