Thread: Inconsistency with EXPLAIN ANALYZE CREATE MATERIALIZED VIEW
EXPLAIN ANALYZE CREATE MATERIALIZED VIEW doesn't go through
ExecCreateTableAs(), but does use CreateIntoRelDestReceiver().
That bypasses the SECURITY_RESTRICTED_OPERATION in ExecCreateTableAs().
That is *not* a security problem, because the
SECURITY_RESTRICTED_OPERATION in CREATE MATERIALIZED VIEW is merely for
consistency with a later REFRESH MATERIALIZED VIEW command where the
SECURITY_RESTRICTED_OPERATION is important.
But it is inconsistent. For example:
create or replace function set() returns int
language plpgsql as $$
begin
create temp table x(i int);
return 42;
end;
$$;
create materialized view mv1 as select set(); -- fails
explain analyze
create materialized view mv1 as select set(); -- succeeds
Relatedly, if we can EXPLAIN a CREATE MATERIALIZED VIEW, perhaps we
should be able to EXPLAIN a REFRESH MATERIALIZED VIEW, too?
Comments?
Regards,
Jeff Davis
On Thu, 1 Aug 2024 at 23:27, Jeff Davis <pgsql@j-davis.com> wrote: > Relatedly, if we can EXPLAIN a CREATE MATERIALIZED VIEW, perhaps we > should be able to EXPLAIN a REFRESH MATERIALIZED VIEW, too? Sure > Comments? Seems like this is indeed inconsistent behaviour and should be fixed in all PGDG-supported versions in the upcoming August release. Do you have any suggestions on how to fix this? > Regards, > Jeff Davis
On Thu, 1 Aug 2024 at 23:27, Jeff Davis <pgsql@j-davis.com> wrote: > > EXPLAIN ANALYZE CREATE MATERIALIZED VIEW doesn't go through > ExecCreateTableAs(), but does use CreateIntoRelDestReceiver(). EXPLAIN ANALYZE and regular query goes through create_ctas_internal (WITH NO DATA case too). Maybe we can simply move SetUserIdAndSecContext call in this function?
On Fri, 2024-08-02 at 00:13 +0500, Kirill Reshke wrote:
> On Thu, 1 Aug 2024 at 23:27, Jeff Davis <pgsql@j-davis.com> wrote:
> >
> > EXPLAIN ANALYZE CREATE MATERIALIZED VIEW doesn't go through
> > ExecCreateTableAs(), but does use CreateIntoRelDestReceiver().
>
> EXPLAIN ANALYZE and regular query goes through create_ctas_internal
> (WITH NO DATA case too). Maybe we can simply move
> SetUserIdAndSecContext call in this function?
We need to set up the SECURITY_RESTRICTED_OPERATION before planning, in
case the planner executes some functions.
I believe we need to do some more refactoring to make this work. In
version 17, I already refactored so that CREATE MATERIALIZED VIEW and
REFRESH MATERIALIZED VIEW share the code. We can do something similar
to extend that to EXPLAIN ... CREATE MATERIALIZED VIEW.
As for the August release, the code freeze is on Saturday. Perhaps it
can be done by then, but is there a reason we should rush it? This
affects all supported versions, so we've lived with it for a while, and
I don't see a security problem here. I wouldn't expect it to be a
common use case, either.
Regards,
Jeff Davis
On Thu, 1 Aug 2024 23:41:18 +0500 Kirill Reshke <reshkekirill@gmail.com> wrote: > On Thu, 1 Aug 2024 at 23:27, Jeff Davis <pgsql@j-davis.com> wrote: > > Relatedly, if we can EXPLAIN a CREATE MATERIALIZED VIEW, perhaps we > > should be able to EXPLAIN a REFRESH MATERIALIZED VIEW, too? > Sure REFRESH MATERIALIZED VIEW consists of not only the view query execution in refresh_matview_datafill but also refresh_by_heap_swap or refresh_by_match_merge. The former doesn't execute any query, so it would not a problem, but the latter executes additional queries including SELECT, some DDL, DELETE, and INSERT. If we would make EXPLAIN support REFRESH MATERIALIZED VIEW CONCURRENTLY command, how should we handle these additional queries? Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
On Tue, 6 Aug 2024 at 21:06, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > On Thu, 1 Aug 2024 23:41:18 +0500 > Kirill Reshke <reshkekirill@gmail.com> wrote: > > > On Thu, 1 Aug 2024 at 23:27, Jeff Davis <pgsql@j-davis.com> wrote: > > > Relatedly, if we can EXPLAIN a CREATE MATERIALIZED VIEW, perhaps we > > > should be able to EXPLAIN a REFRESH MATERIALIZED VIEW, too? > > Sure > > REFRESH MATERIALIZED VIEW consists of not only the view query > execution in refresh_matview_datafill but also refresh_by_heap_swap > or refresh_by_match_merge. The former doesn't execute any query, so > it would not a problem, but the latter executes additional queries > including SELECT, some DDL, DELETE, and INSERT. > > If we would make EXPLAIN support REFRESH MATERIALIZED VIEW CONCURRENTLY > command, how should we handle these additional queries? > > Regards, > Yugo Nagata > > -- > Yugo Nagata <nagata@sraoss.co.jp> Hmm, is it a big issue? Maybe we can just add them in proper places of output the same way we do it with triggers? -- Best regards, Kirill Reshke
On Thu, 01 Aug 2024 13:34:51 -0700 Jeff Davis <pgsql@j-davis.com> wrote: > On Fri, 2024-08-02 at 00:13 +0500, Kirill Reshke wrote: > > On Thu, 1 Aug 2024 at 23:27, Jeff Davis <pgsql@j-davis.com> wrote: > > > > > > EXPLAIN ANALYZE CREATE MATERIALIZED VIEW doesn't go through > > > ExecCreateTableAs(), but does use CreateIntoRelDestReceiver(). > > > > EXPLAIN ANALYZE and regular query goes through create_ctas_internal > > (WITH NO DATA case too). Maybe we can simply move > > SetUserIdAndSecContext call in this function? > > We need to set up the SECURITY_RESTRICTED_OPERATION before planning, in > case the planner executes some functions. > > I believe we need to do some more refactoring to make this work. In > version 17, I already refactored so that CREATE MATERIALIZED VIEW and > REFRESH MATERIALIZED VIEW share the code. We can do something similar > to extend that to EXPLAIN ... CREATE MATERIALIZED VIEW. I think the most simple way to fix this is to set up the userid and the the SECURITY_RESTRICTED_OPERATION, and call RestrictSearchPath() before pg_plan_query in ExplainOneQuery(), as in the attached patch. However, this is similar to the old code of ExecCreateTableAs() before refactored. If we want to reuse REFRESH even in EXPLAIN as similar as the current CREATE MATERIALIZED code, I think we would have to refactor RefereshMatViewByOid to receive instrument_option and eflags as arguments and call it in ExplainOnePlan, for example. Do you think that is more preferred than setting up SECURITY_RESTRICTED_OPERATION in ExplainOneQuery? > As for the August release, the code freeze is on Saturday. Perhaps it > can be done by then, but is there a reason we should rush it? This > affects all supported versions, so we've lived with it for a while, and > I don't see a security problem here. I wouldn't expect it to be a > common use case, either. I agree that we don't have to rush it since it is not a security bug but just it could make a materialized view that cannot be refreshed. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
On Tue, 6 Aug 2024 at 22:13, Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
> On Thu, 01 Aug 2024 13:34:51 -0700
> Jeff Davis <pgsql@j-davis.com> wrote:
>
> > On Fri, 2024-08-02 at 00:13 +0500, Kirill Reshke wrote:
> > > On Thu, 1 Aug 2024 at 23:27, Jeff Davis <pgsql@j-davis.com> wrote:
> > > >
> > > > EXPLAIN ANALYZE CREATE MATERIALIZED VIEW doesn't go through
> > > > ExecCreateTableAs(), but does use CreateIntoRelDestReceiver().
> > >
> > > EXPLAIN ANALYZE and regular query goes through create_ctas_internal
> > > (WITH NO DATA case too). Maybe we can simply move
> > > SetUserIdAndSecContext call in this function?
> >
> > We need to set up the SECURITY_RESTRICTED_OPERATION before planning, in
> > case the planner executes some functions.
> >
> > I believe we need to do some more refactoring to make this work. In
> > version 17, I already refactored so that CREATE MATERIALIZED VIEW and
> > REFRESH MATERIALIZED VIEW share the code. We can do something similar
> > to extend that to EXPLAIN ... CREATE MATERIALIZED VIEW.
>
> I think the most simple way to fix this is to set up the userid and the
> the SECURITY_RESTRICTED_OPERATION, and call RestrictSearchPath()
> before pg_plan_query in ExplainOneQuery(), as in the attached patch.
>
> However, this is similar to the old code of ExecCreateTableAs() before
> refactored. If we want to reuse REFRESH even in EXPLAIN as similar as the
> current CREATE MATERIALIZED code, I think we would have to refactor
> RefereshMatViewByOid to receive instrument_option and eflags as arguments
> and call it in ExplainOnePlan, for example.
>
> Do you think that is more preferred than setting up
> SECURITY_RESTRICTED_OPERATION in ExplainOneQuery?
>
> > As for the August release, the code freeze is on Saturday. Perhaps it
> > can be done by then, but is there a reason we should rush it? This
> > affects all supported versions, so we've lived with it for a while, and
> > I don't see a security problem here. I wouldn't expect it to be a
> > common use case, either.
>
> I agree that we don't have to rush it since it is not a security bug
> but just it could make a materialized view that cannot be refreshed.
>
> Regards,
> Yugo Nagata
>
> --
> Yugo Nagata <nagata@sraoss.co.jp>
> + /*
> + * For CREATE MATERIALIZED VIEW command, switch to the owner's userid, so
> + * that any functions are run as that user. Also lock down security-restricted
> + * operations and arrange to make GUC variable changes local to this command.
> + */
> + if (into && into->viewQuery)
> + {
> + GetUserIdAndSecContext(&save_userid, &save_sec_context);
> + SetUserIdAndSecContext(save_userid,
> + save_sec_context | SECURITY_RESTRICTED_OPERATION);
> + save_nestlevel = NewGUCNestLevel();
> + RestrictSearchPath();
> + }
> +
In general, doing this ad-hoc coding for MV inside
standard_ExplainOneQuery function looks just out of place for me.
standard_ExplainOneQuery is on another level of abstraction.
--
Best regards,
Kirill Reshke
On Wed, 7 Aug 2024 02:13:04 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Thu, 01 Aug 2024 13:34:51 -0700 > Jeff Davis <pgsql@j-davis.com> wrote: > > > On Fri, 2024-08-02 at 00:13 +0500, Kirill Reshke wrote: > > > On Thu, 1 Aug 2024 at 23:27, Jeff Davis <pgsql@j-davis.com> wrote: > > > > > > > > EXPLAIN ANALYZE CREATE MATERIALIZED VIEW doesn't go through > > > > ExecCreateTableAs(), but does use CreateIntoRelDestReceiver(). > > > > > > EXPLAIN ANALYZE and regular query goes through create_ctas_internal > > > (WITH NO DATA case too). Maybe we can simply move > > > SetUserIdAndSecContext call in this function? > > > > We need to set up the SECURITY_RESTRICTED_OPERATION before planning, in > > case the planner executes some functions. > > > > I believe we need to do some more refactoring to make this work. In > > version 17, I already refactored so that CREATE MATERIALIZED VIEW and > > REFRESH MATERIALIZED VIEW share the code. We can do something similar > > to extend that to EXPLAIN ... CREATE MATERIALIZED VIEW. > > I think the most simple way to fix this is to set up the userid and the > the SECURITY_RESTRICTED_OPERATION, and call RestrictSearchPath() > before pg_plan_query in ExplainOneQuery(), as in the attached patch. I'm sorry. After sending the mail, I found the patch didn't work. If we call RestrictSearchPath() before creating a relation, it tries to create the relation in pg_catalog and it causes an error. Perhaps, we would need to create the rel before RestrictSearchPath() by calling create_ctas_nodata or something like this... > > However, this is similar to the old code of ExecCreateTableAs() before > refactored. If we want to reuse REFRESH even in EXPLAIN as similar as the > current CREATE MATERIALIZED code, I think we would have to refactor > RefereshMatViewByOid to receive instrument_option and eflags as arguments > and call it in ExplainOnePlan, for example. > > Do you think that is more preferred than setting up > SECURITY_RESTRICTED_OPERATION in ExplainOneQuery? > > > As for the August release, the code freeze is on Saturday. Perhaps it > > can be done by then, but is there a reason we should rush it? This > > affects all supported versions, so we've lived with it for a while, and > > I don't see a security problem here. I wouldn't expect it to be a > > common use case, either. > > I agree that we don't have to rush it since it is not a security bug > but just it could make a materialized view that cannot be refreshed. > > Regards, > Yugo Nagata > > -- > Yugo Nagata <nagata@sraoss.co.jp> -- Yugo Nagata <nagata@sraoss.co.jp>
On Wed, 2024-08-07 at 03:06 +0900, Yugo Nagata wrote:
> I'm sorry. After sending the mail, I found the patch didn't work.
> If we call RestrictSearchPath() before creating a relation, it tries
> to create the relation in pg_catalog and it causes an error.
Yeah, that's exactly the problem I ran into in ExecCreateTableAs(),
which was the reason I refactored it to use RefreshMatViewByOid().
> Perhaps, we would need to create the rel before RestrictSearchPath()
> by calling
> create_ctas_nodata or something like this...
I haven't looked at the details, but that makes sense: break it into
two parts so that the create (without data) happens first without doing
the work of EXPLAIN, and the second part refreshes using the explain
logic. That means RefreshMatViewByOid() would need to work with
EXPLAIN.
As you point out in the other email, it's not easy to make that all
work with REFRESH ... CONCURRENTLY, but perhaps it could work with
CREATE MATERIALIZED VIEW and REFRESH (without CONCURRENTLY).
Regards,
Jeff Davis
Jeff Davis <pgsql@j-davis.com> writes:
> As you point out in the other email, it's not easy to make that all
> work with REFRESH ... CONCURRENTLY, but perhaps it could work with
> CREATE MATERIALIZED VIEW and REFRESH (without CONCURRENTLY).
I'm not really sure I see the point of this, if it doesn't "just work"
with all variants of C.M.V. It's not like you can't easily EXPLAIN
the view's SELECT.
If REFRESH M. V. does something different than CREATE, there would
certainly be value in being able to EXPLAIN what that does --- but
that still isn't an argument for allowing EXPLAIN CREATE MATERIALIZED
VIEW.
regards, tom lane
On Tue, 06 Aug 2024 11:24:03 -0700 Jeff Davis <pgsql@j-davis.com> wrote: > On Wed, 2024-08-07 at 03:06 +0900, Yugo Nagata wrote: > > I'm sorry. After sending the mail, I found the patch didn't work. > > If we call RestrictSearchPath() before creating a relation, it tries > > to create the relation in pg_catalog and it causes an error. > > Yeah, that's exactly the problem I ran into in ExecCreateTableAs(), > which was the reason I refactored it to use RefreshMatViewByOid(). I came up with an idea that we can rewrite into->rel to add the current schemaname before calling RestrictSearchPath() as in the attached patch. It seems a simpler fix at least, but I am not sure whether this design is better than using RefreshMatViewByOid from EXPLAIN considering to support EXPLAIN REFRESH ... Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
Attachment
On Tue, 2024-08-06 at 14:36 -0400, Tom Lane wrote:
> I'm not really sure I see the point of this, if it doesn't "just
> work"
> with all variants of C.M.V. It's not like you can't easily EXPLAIN
> the view's SELECT.
>
> If REFRESH M. V. does something different than CREATE, there would
> certainly be value in being able to EXPLAIN what that does --- but
> that still isn't an argument for allowing EXPLAIN CREATE MATERIALIZED
> VIEW.
We already allow EXPLAIN ANALYZE CREATE MATERIALIZED VIEW in all
supported versions.
That seems strange and it surprised me, but the parser structure is
shared between SELECT ... INTO and CREATE MATERIALIZED VIEW, so I
suppose it was supported out of convenience.
The problem is that the implentation is split between the EXPLAIN
ANALYZE path and the non-EXPLAIN path. The long-ago commit f3ab5d4696
missed the EXPLAIN path. NB: I do not believe this is a security
concern, but it does create the inconsistency described in the email
starting this thread.
Options:
1. Do nothing on the grounds that EXPLAIN ANALYZE CREATE MATERIALIZED
VIEW is not common enough to worry about, and the consequences of the
inconsistency are not bad enough.
2. Refactor some more to make EXPLAIN ANALYZE CREATE MATERIALIZED VIEW
share the query part of the code path with REFRESH so that it benefits
from the SECURITY_RESTRICTED_OPERATION and RestrictSearchPath().
3. Do #2 but also make it work for REFRESH, but not CONCURRENTLY.
4. Do #3 but also make it work for REFRESH ... CONCURRENTLY and provide
new information that's not available by only explaining the query.
And also figure out if any of this should be back-patched.
Regards,
Jeff Davis