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

On Wed, Feb 14, 2024 at 10:20:28AM -0800, Jeff Davis wrote:

If those arguments are still unconvincing, then the next idea is to fix
the search_path for all maintenance commands[3]. I tried this during
the 16 cycle, but due to timing issues it was also reverted. I can
proceed with this approach again, but I'd like a clear endorsement, in
case there were other reasons to doubt the approach.

This seemed like the approach folks were most in favor of at the developer
meeting a couple weeks ago [0]https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2024_Developer_Meeting#The_Path_to_un-reverting_the_MAINTAIN_privilege. At least, that was my interpretation of
the discussion.

BTW I have been testing reverting commit 151c22d (i.e., un-reverting
MAINTAIN) every month or two, and last I checked, it still applies pretty
cleanly. The only changes I've needed to make are to the catversion and to
a hard-coded version in a test (16 -> 17).

[0]: https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2024_Developer_Meeting#The_Path_to_un-reverting_the_MAINTAIN_privilege

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Feb 14, 2024 at 01:02:26PM -0600, Nathan Bossart wrote:

BTW I have been testing reverting commit 151c22d (i.e., un-reverting
MAINTAIN) every month or two, and last I checked, it still applies pretty
cleanly. The only changes I've needed to make are to the catversion and to
a hard-coded version in a test (16 -> 17).

Posting to get some cfbot coverage.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, 2024-02-14 at 13:02 -0600, Nathan Bossart wrote:

This seemed like the approach folks were most in favor of at the
developer
meeting a couple weeks ago [0].  At least, that was my interpretation
of
the discussion.

Attached rebased version.

Note the changes in amcheck. It's creating functions and calling those
functions from the comparators, and so the comparators need to set the
search_path. I don't think that's terribly common, but does represent a
behavior change and could break something.

Regards,
Jef Davis

(Apologies in advance for anything I'm bringing up that we've already
covered somewhere else.)

On Fri, Feb 16, 2024 at 04:03:55PM -0800, Jeff Davis wrote:

Note the changes in amcheck. It's creating functions and calling those
functions from the comparators, and so the comparators need to set the
search_path. I don't think that's terribly common, but does represent a
behavior change and could break something.

Why is this change needed? Is the idea to make amcheck follow the same
rules as maintenance commands to encourage folks to set up index functions
correctly? Or is amcheck similarly affected by search_path tricks?

void
InitializeSearchPath(void)
{
+	/* Make the context we'll keep search path cache hashtable in */
+	SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext,
+												   "search_path processing cache",
+												   ALLOCSET_DEFAULT_SIZES);
+
if (IsBootstrapProcessingMode())
{
/*
@@ -4739,11 +4744,6 @@ InitializeSearchPath(void)
}
else
{
-		/* Make the context we'll keep search path cache hashtable in */
-		SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext,
-													   "search_path processing cache",
-													   ALLOCSET_DEFAULT_SIZES);
-

What is the purpose of this change?

+	SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+					PGC_S_SESSION);

I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new value
for these.

+/*
+ * Safe search path when executing code as the table owner, such as during
+ * maintenance operations.
+ */
+#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"

Is including pg_temp actually safe? I worry that a user could use their
temporary schema to inject objects that would take the place of
non-schema-qualified stuff in functions.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

New version attached.

Do we need a documentation update here? If so, where would be a good
place?

On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:

Why is this change needed?  Is the idea to make amcheck follow the
same
rules as maintenance commands to encourage folks to set up index
functions
correctly?

amcheck is calling functions it defined, so in order to find those
functions it needs to set the right search path.

What is the purpose of this [bootstrap-related] change?

DefineIndex() is called during bootstrap, and it's also a maintenance
command. I tried to handle the bootstrapping case, but I think it's
best to just guard it with a conditional. Done.

I also added Assert(!IsBootstrapProcessingMode()) in
assign_search_path().

+       SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH,
PGC_USERSET,
+                                       PGC_S_SESSION);

I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new
value
for these.

Did you have a particular concern about PGC_S_SESSION?

If it's less than PGC_S_SESSION, it won't work, because the caller's
SET command will override it, and the same manipulation is possible.

And I don't think we want it higher than PGC_S_SESSION, otherwise the
function can't set its own search_path, if needed.

+#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"

Is including pg_temp actually safe?  I worry that a user could use
their
temporary schema to inject objects that would take the place of
non-schema-qualified stuff in functions.

pg_temp cannot (currently) be excluded. If it is omitted from the
string, it will be placed *first* in the search_path, which is more
dangerous.

pg_temp does not take part in function or operator resolution, which
makes it safer than it first appears. There are potentially some risks
around tables, but it's not typical to access a table in a function
called as part of an index expression.

If we determine that pg_temp is actually unsafe to include, we need to
do something like what I proposed here:

/messages/by-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.camel@j-davis.com

before this change.

Regards,
Jeff Davis

On Tue, Feb 27, 2024 at 04:22:34PM -0800, Jeff Davis wrote:

Do we need a documentation update here? If so, where would be a good
place?

I'm afraid I don't have a better idea than adding a short note in each
affected commands's page.

On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:

I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new
value
for these.

Did you have a particular concern about PGC_S_SESSION?

My only concern is that it could obscure the source of the search_path
change, which in turn might cause confusion when things fail.

If it's less than PGC_S_SESSION, it won't work, because the caller's
SET command will override it, and the same manipulation is possible.

And I don't think we want it higher than PGC_S_SESSION, otherwise the
function can't set its own search_path, if needed.

Yeah, we would have to make it equivalent in priority to PGC_S_SESSION,
which would likely require a bunch of special logic. I don't know if this
is worth it, and this seems like something that could pretty easily be
added in the future if it became necessary.

+#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"

Is including pg_temp actually safe?� I worry that a user could use
their
temporary schema to inject objects that would take the place of
non-schema-qualified stuff in functions.

pg_temp cannot (currently) be excluded. If it is omitted from the
string, it will be placed *first* in the search_path, which is more
dangerous.

pg_temp does not take part in function or operator resolution, which
makes it safer than it first appears. There are potentially some risks
around tables, but it's not typical to access a table in a function
called as part of an index expression.

If we determine that pg_temp is actually unsafe to include, we need to
do something like what I proposed here:

/messages/by-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.camel@j-davis.com

I don't doubt anything you've said, but I can't help but think that we
might as well handle the pg_temp risk, too.

Furthermore, I see that we use "" as a safe search_path for autovacuum and
fe_utils/connect.h. Is there any way to unite these? IIUC it might be
possible to combine the autovacuum and maintenance command cases (i.e.,
"!pg_temp"), but we might need to keep pg_temp for the frontend case. I
think it's worth trying to add comments about why this setting is safe for
some cases but not others, too.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, 2024-02-28 at 10:55 -0600, Nathan Bossart wrote:

I'm afraid I don't have a better idea than adding a short note in
each
affected commands's page.

OK, that works for now.

Later we should also document that the functions are run as the table
owner.

On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:

I wonder if it's worth using PGC_S_INTERACTIVE or introducing a
new
value
for these.

Did you have a particular concern about PGC_S_SESSION?

My only concern is that it could obscure the source of the
search_path
change, which in turn might cause confusion when things fail.

That's a good point. AutoVacWorkerMain uses PGC_S_OVERRIDE, but it
doesn't have to worry about SET, because there's no real session.

The function SET clause uses PGC_S_SESSION. It's arguable whether
that's really the same source as a SET command, but it's definitely
closer.

Yeah, we would have to make it equivalent in priority to
PGC_S_SESSION,
which would likely require a bunch of special logic.

I'm not clear on what problem that would solve.

I don't doubt anything you've said, but I can't help but think that
we
might as well handle the pg_temp risk, too.

That sounds good to me, but I didn't get many replies in that last
thread. And although it solves the problem, it is a bit awkward.

Can we get some closure on whether that !pg_temp patch is the right
approach? That was just my first idea, and it would be good to hear
what others think.

Furthermore, I see that we use "" as a safe search_path for
autovacuum and
fe_utils/connect.h.  Is there any way to unite these?

We could have a single function like RestrictSearchPath(), which I
believe I had in some previous iteration. That would use the safest
search path (either excluding pg_temp or putting it at the end) and
PGC_S_SESSION, and then use it everywhere.

Regards,
Jeff Davis

On Wed, 2024-02-28 at 09:29 -0800, Jeff Davis wrote:

On Wed, 2024-02-28 at 10:55 -0600, Nathan Bossart wrote:

I'm afraid I don't have a better idea than adding a short note in
each
affected commands's page.

OK, that works for now.

Committed.

The only changes are documentation and test updates.

This is a behavior change, so it still carries some risk, though we've
had a lot of discussion and generally it seems to be worth it. If it
turns out worse than expected during beta, of course we can re-revert
it.

I will restate the risks here, which come basically from two places:

(1) Functions called from index expressions which rely on search_path
(and don't have a SET clause).

Such a function would have already been fairly broken before my commit,
because anyone accessing the table without the right search_path would
have seen an error or wrong results. And there is no means to set the
"right" search_path for autoanalyze or logical replication, so those
would not have worked with such a broken function before my commit, no
matter what.

That being said, surely some users did have such broken functions, and
with this commit, they will have to remedy them with a SET clause.
Fortunately, the performance impact of doing so has been greatly
reduced.

(2) Matierialized views which call functions that rely on search_path
(and don't have a SET clause).

This is arguably a worse kind of breakage because materialized views
are often refreshed only by the table owner, and it's easier to control
search_path when running REFRESH. Additionally, functions called from
materialized views are more likely to be "interesting" than functions
called from an index expression. However, the remedy is
straightforward: use a SET clause.

Regards,
Jeff Davis

On Mon, Mar 04, 2024 at 07:52:05PM -0800, Jeff Davis wrote:

Committed.

Commit 2af07e2 wrote:

--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1412,6 +1412,8 @@ brin_summarize_range(PG_FUNCTION_ARGS)
SetUserIdAndSecContext(heapRel->rd_rel->relowner,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
+		SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+						PGC_S_SESSION);

I've audited NewGUCNestLevel() calls that didn't get this addition. Among
those, these need the addition:

- Each in ComputeIndexAttrs() -- they arise when the caller is DefineIndex()
- In DefineIndex(), after comment "changed a behavior-affecting GUC"

While "not necessary for security", ExecCreateTableAs() should do it for the
same reason it calls NewGUCNestLevel().

On Sun, Jun 30, 2024 at 03:23:44PM -0700, Noah Misch wrote:

I've audited NewGUCNestLevel() calls that didn't get this addition. Among
those, these need the addition:

- Each in ComputeIndexAttrs() -- they arise when the caller is DefineIndex()
- In DefineIndex(), after comment "changed a behavior-affecting GUC"

Hmm. Is RestrictSearchPath() something that we should advertise more
strongly, thinking here about extensions that call NewGUCNestLevel()?
That would be really easy to miss, and it could have bad consequences.
I know that this is not something that's published in the release
notes, but it looks like something sensible to have, though.

While "not necessary for security", ExecCreateTableAs() should do it for the
same reason it calls NewGUCNestLevel().

+1.
--
Michael

On Tue, 2024-07-09 at 15:20 +0900, Michael Paquier wrote:

On Sun, Jun 30, 2024 at 03:23:44PM -0700, Noah Misch wrote:

I've audited NewGUCNestLevel() calls that didn't get this
addition.  Among
those, these need the addition:

- Each in ComputeIndexAttrs() -- they arise when the caller is
DefineIndex()
- In DefineIndex(), after comment "changed a behavior-affecting
GUC"

Thank you for the report. Patch attached to address these missing call
sites.

Hmm.  Is RestrictSearchPath() something that we should advertise more
strongly, thinking here about extensions that call NewGUCNestLevel()?
That would be really easy to miss, and it could have bad
consequences.
I know that this is not something that's published in the release
notes, but it looks like something sensible to have, though.

The pattern also involves SetUserIdAndSecContext(). Perhaps we could
come up with a wrapper function to better encapsulate the general
pattern?

While "not necessary for security", ExecCreateTableAs() should do
it for the
same reason it calls NewGUCNestLevel().

+1.

Do you have a suggestion about how that should be done?

It's not trivial, because the both creates the table and populates it
in ExecutorRun. For table creation, we need to use the original
search_path, but we need to use the restricted search_path when
populating it.

I could try to refactor it into two statements and execute them
separately, or I could try to rewrite the statement to use a fully-
qualified destination table before execution. Thoughts?

Regards,
Jeff Davis

On Tue, Jul 09, 2024 at 05:47:36PM -0700, Jeff Davis wrote:

On Tue, 2024-07-09 at 15:20 +0900, Michael Paquier wrote:

On Sun, Jun 30, 2024 at 03:23:44PM -0700, Noah Misch wrote:

Hmm.� Is RestrictSearchPath() something that we should advertise more
strongly, thinking here about extensions that call NewGUCNestLevel()?
That would be really easy to miss, and it could have bad
consequences.
I know that this is not something that's published in the release
notes, but it looks like something sensible to have, though.

The pattern also involves SetUserIdAndSecContext(). Perhaps we could
come up with a wrapper function to better encapsulate the general
pattern?

Worth a look. usercontext.c has an existing wrapper for a superuser process
switching to an untrusted user. It could become the home for another wrapper
targeting MAINTAIN-relevant callers.

While "not necessary for security", ExecCreateTableAs() should do
it for the
same reason it calls NewGUCNestLevel().

+1.

Do you have a suggestion about how that should be done?

It's not trivial, because the both creates the table and populates it
in ExecutorRun. For table creation, we need to use the original
search_path, but we need to use the restricted search_path when
populating it.

I could try to refactor it into two statements and execute them
separately, or I could try to rewrite the statement to use a fully-
qualified destination table before execution. Thoughts?

Those sound fine. Also fine: just adding a comment on why creation namespace
considerations led to not doing it there.

On Thu, 2024-07-11 at 05:52 -0700, Noah Misch wrote:

I could try to refactor it into two statements and execute them
separately, or I could try to rewrite the statement to use a fully-
qualified destination table before execution. Thoughts?

Those sound fine.  Also fine: just adding a comment on why creation
namespace
considerations led to not doing it there.

Attached. 0002 separates the CREATE MATERIALIZED VIEW ... WITH DATA
into (effectively):

CREATE MATERIALIZED VIEW ... WITH NO DATA;
REFRESH MATERIALIZED VIEW ...;

Using refresh also achieves the stated goal more directly: to (mostly)
ensure that a subsequent REFRESH will succeed.

Note: the creation itself no longer executes in a security-restricted
context, but I don't think that's a problem. The only reason it's using
the security restricted context is so the following REFRESH will
succeed, right?

Regards,
Jeff Davis

On Fri, Jul 12, 2024 at 02:50:52PM -0700, Jeff Davis wrote:

On Thu, 2024-07-11 at 05:52 -0700, Noah Misch wrote:

I could try to refactor it into two statements and execute them
separately, or I could try to rewrite the statement to use a fully-
qualified destination table before execution. Thoughts?

Those sound fine.� Also fine: just adding a comment on why creation
namespace
considerations led to not doing it there.

Attached. 0002 separates the CREATE MATERIALIZED VIEW ... WITH DATA
into (effectively):

CREATE MATERIALIZED VIEW ... WITH NO DATA;
REFRESH MATERIALIZED VIEW ...;

Using refresh also achieves the stated goal more directly: to (mostly)
ensure that a subsequent REFRESH will succeed.

Note: the creation itself no longer executes in a security-restricted
context, but I don't think that's a problem. The only reason it's using
the security restricted context is so the following REFRESH will
succeed, right?

Right, that's the only reason.

@@ -346,13 +339,21 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
PopActiveSnapshot();
}

-	if (is_matview)
+	/*
+	 * For materialized views, use REFRESH, which locks down
+	 * security-restricted operations and restricts the search_path.
+	 * Otherwise, one could create a materialized view not possible to
+	 * refresh.
+	 */
+	if (do_refresh)
{
-		/* Roll back any GUC changes */
-		AtEOXact_GUC(false, save_nestlevel);
+		RefreshMatViewStmt *refresh = makeNode(RefreshMatViewStmt);

-		/* Restore userid and security context */
-		SetUserIdAndSecContext(save_userid, save_sec_context);
+		refresh->relation = into->rel;
+		ExecRefreshMatView(refresh, pstate->p_sourcetext, NULL, qc);
+
+		if (qc)
+			qc->commandTag = CMDTAG_SELECT;
}

Since refresh->relation is a RangeVar, this departs from the standard against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).

On Fri, 2024-07-12 at 16:11 -0700, Noah Misch wrote:

Since refresh->relation is a RangeVar, this departs from the standard
against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).

Interesting, thank you.

I did a rough refactor and attached v3. Aside from cleanup issues, is
this what you had in mind?

Regards,
Jeff Davis

On Fri, Jul 12, 2024 at 04:50:17PM -0700, Jeff Davis wrote:

On Fri, 2024-07-12 at 16:11 -0700, Noah Misch wrote:

Since refresh->relation is a RangeVar, this departs from the standard
against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).

Interesting, thank you.

I did a rough refactor and attached v3. Aside from cleanup issues, is
this what you had in mind?

+extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
+										 const char *queryString, ParamListInfo params,
+										 QueryCompletion *qc);

Yes, that's an API design that avoids repeated name lookups.

Hi,

On Sat, 13 Jul 2024 14:47:48 -0700
Noah Misch <noah@leadboat.com> wrote:

On Fri, Jul 12, 2024 at 04:50:17PM -0700, Jeff Davis wrote:

On Fri, 2024-07-12 at 16:11 -0700, Noah Misch wrote:

Since refresh->relation is a RangeVar, this departs from the standard
against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).

Interesting, thank you.

I did a rough refactor and attached v3. Aside from cleanup issues, is
this what you had in mind?

+extern ObjectAddress RefreshMatViewByOid(Oid matviewOid, bool skipData, bool concurrent,
+										 const char *queryString, ParamListInfo params,
+										 QueryCompletion *qc);

Yes, that's an API design that avoids repeated name lookups.

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");

RefreshMatViewByOid has REFRESH specific error messages in spite of its use
in CREATE MATERIALIZED VIEW, but these errors seem not to occur in CREATE MATERIALIZED
VIEW case, so I don't think it would be a problem.

Another my question is why RefreshMatViewByOid has a ParamListInfo parameter.
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.

I attaehd patches for fixing them respectedly.

What do you think about it?

Regards,
Yugo Nagata

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

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.

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

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>