Skip ExecCheckRTPerms in CTAS with no data

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Skip ExecCheckRTPerms in CTAS with no data

Bharath Rupireddy
Hi,

In case of CTAS with no data, we actually do not insert the tuples
into the created table, so we can skip checking for the insert
permissions. Anyways, the insert permissions will be checked when the
tuples are inserted into the table.

Attaching a small patch doing $subject.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

v1-Skip-ExecCheckRTPerms-in-CTAS-with-no-data.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Tom Lane-2
Bharath Rupireddy <[hidden email]> writes:
> In case of CTAS with no data, we actually do not insert the tuples
> into the created table, so we can skip checking for the insert
> permissions. Anyways, the insert permissions will be checked when the
> tuples are inserted into the table.

I'd argue this is wrong.  You don't get to skip permissions checks
in ordinary queries just because, say, there's a LIMIT 0 on the
query.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Bharath Rupireddy
On Mon, Sep 28, 2020 at 7:48 PM Tom Lane <[hidden email]> wrote:

>
> Bharath Rupireddy <[hidden email]> writes:
> > In case of CTAS with no data, we actually do not insert the tuples
> > into the created table, so we can skip checking for the insert
> > permissions. Anyways, the insert permissions will be checked when the
> > tuples are inserted into the table.
>
> I'd argue this is wrong.  You don't get to skip permissions checks
> in ordinary queries just because, say, there's a LIMIT 0 on the
> query.
>

Right, when there's a select with limit 0 clause, we do check for the
select permissions. But my point is, we don't check insert
permissions(or select or update etc.) when we create a plain table
using CREATE TABLE test_tbl(a1 INT). Of course, we do check create
permissions. Going by the similar point, shouldn't we also check only
create permission(which is already being done as part of
DefineRelation) and skip the insert permission(the change this patch
does) for the new table being created as part of CREATE TABLE test_tbl
AS SELECT * FROM test_tbl2? However select permission will be checked
for test_tbl2. The insert permissions will be checked anyways before
inserting rows into the table created in CTAS.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Bharath Rupireddy
On Tue, Sep 29, 2020 at 5:09 PM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Mon, Sep 28, 2020 at 7:48 PM Tom Lane <[hidden email]> wrote:
> >
> > Bharath Rupireddy <[hidden email]> writes:
> > > In case of CTAS with no data, we actually do not insert the tuples
> > > into the created table, so we can skip checking for the insert
> > > permissions. Anyways, the insert permissions will be checked when the
> > > tuples are inserted into the table.
> >
> > I'd argue this is wrong.  You don't get to skip permissions checks
> > in ordinary queries just because, say, there's a LIMIT 0 on the
> > query.
> >
>
> Right, when there's a select with limit 0 clause, we do check for the
> select permissions. But my point is, we don't check insert
> permissions(or select or update etc.) when we create a plain table
> using CREATE TABLE test_tbl(a1 INT). Of course, we do check create
> permissions. Going by the similar point, shouldn't we also check only
> create permission(which is already being done as part of
> DefineRelation) and skip the insert permission(the change this patch
> does) for the new table being created as part of CREATE TABLE test_tbl
> AS SELECT * FROM test_tbl2? However select permission will be checked
> for test_tbl2. The insert permissions will be checked anyways before
> inserting rows into the table created in CTAS.
>

Added this to the commitfest for further review.

https://commitfest.postgresql.org/30/2755/



With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Anastasia Lubennikova
In reply to this post by Bharath Rupireddy
On 29.09.2020 14:39, Bharath Rupireddy wrote:

> On Mon, Sep 28, 2020 at 7:48 PM Tom Lane <[hidden email]> wrote:
>> Bharath Rupireddy <[hidden email]> writes:
>>> In case of CTAS with no data, we actually do not insert the tuples
>>> into the created table, so we can skip checking for the insert
>>> permissions. Anyways, the insert permissions will be checked when the
>>> tuples are inserted into the table.
>> I'd argue this is wrong.  You don't get to skip permissions checks
>> in ordinary queries just because, say, there's a LIMIT 0 on the
>> query.
>>
> Right, when there's a select with limit 0 clause, we do check for the
> select permissions. But my point is, we don't check insert
> permissions(or select or update etc.) when we create a plain table
> using CREATE TABLE test_tbl(a1 INT). Of course, we do check create
> permissions. Going by the similar point, shouldn't we also check only
> create permission(which is already being done as part of
> DefineRelation) and skip the insert permission(the change this patch
> does) for the new table being created as part of CREATE TABLE test_tbl
> AS SELECT * FROM test_tbl2? However select permission will be checked
> for test_tbl2. The insert permissions will be checked anyways before
> inserting rows into the table created in CTAS.
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com
>
I see Tom's objection above. Still, I tend to agree that if 'WITH NO
DATA' was specified explicitly, CREATE AS should behave more like a
utility statement rather than a regular query. So I think that this
patch can be useful in some use-cases and I definitely don't see any
harm it could cause. Even the comment in the current code suggests that
it is an option.

I took a look at the patch. It is quite simple, so no comments about the
code. It would be good to add a test to select_into.sql to show that it
only applies to 'WITH NO DATA' and that subsequent insertions will fail
if permissions are not set.

Maybe we should also mention it a documentation, but I haven't found any
specific paragraph about permissions on CTAS.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Michael Paquier-2
On Mon, Nov 09, 2020 at 10:48:09PM +0300, Anastasia Lubennikova wrote:
> I see Tom's objection above. Still, I tend to agree that if 'WITH NO DATA'
> was specified explicitly, CREATE AS should behave more like a utility
> statement rather than a regular query. So I think that this patch can be
> useful in some use-cases and I definitely don't see any harm it could cause.
> Even the comment in the current code suggests that it is an option.

I agree with Tom's point to leave this stuff alone, and just remove
this XXX comment.  An extra issue I can see is that you would bypass
ExecutorCheckPerms_hook_type when using WITH NO DATA.  This could
silently break the users of this hook.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Bharath Rupireddy
On Wed, Nov 11, 2020 at 12:05 PM Michael Paquier <[hidden email]> wrote:

>
> On Mon, Nov 09, 2020 at 10:48:09PM +0300, Anastasia Lubennikova wrote:
> > I see Tom's objection above. Still, I tend to agree that if 'WITH NO DATA'
> > was specified explicitly, CREATE AS should behave more like a utility
> > statement rather than a regular query. So I think that this patch can be
> > useful in some use-cases and I definitely don't see any harm it could cause.
> > Even the comment in the current code suggests that it is an option.
>
> I agree with Tom's point to leave this stuff alone, and just remove
> this XXX comment.  An extra issue I can see is that you would bypass
> ExecutorCheckPerms_hook_type when using WITH NO DATA.  This could
> silently break the users of this hook.
>

The ExecCheckRTPerms() with ACL_INSERT permission will be called
before inserting the data to the table that's created with CREATE AS
WITH NO DATA. The insertion into the table can happen either with
INSERT command(ExecCheckRTPerms() with ACL_INSERT permission will be
called from InitPlan()) or with COPY FROM command(ExecCheckRTPerms()
with ACL_INSERT permission will be called from DoCopy()).

Effectively, we are not bypassing the call to
ExecutorCheckPerms_hook_type. Unless I miss anything else, I think it
makes sense to skip ExecCheckRTPerms() for CTAS WITH NO DATA.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Bharath Rupireddy
In reply to this post by Anastasia Lubennikova
On Tue, Nov 10, 2020 at 1:18 AM Anastasia Lubennikova
<[hidden email]> wrote:
>
> I took a look at the patch. It is quite simple, so no comments about the
> code. It would be good to add a test to select_into.sql to show that it
> only applies to 'WITH NO DATA' and that subsequent insertions will fail
> if permissions are not set.
>

Done.

>
> Maybe we should also mention it a documentation, but I haven't found any
> specific paragraph about permissions on CTAS.
>

Yes we do not have anything related to permissions mentioned in the
documentation. So, I'm not adding it now.

Apart from the above, I also noticed that we unnecessarily allocate
bulk insert state(16MB memory) in case of WITH NO DATA, just to free
it in intorel_shutdown() without actually using it. So, in the v2
patch I have made changes to not allocate bulk insert state.

Attaching v2 patch. Consider it for further review.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

v2-Skip-Insert-Perm-Check-Bulk-Insert-State-alloc-in-WITH-NO-DATA.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Michael Paquier-2
In reply to this post by Bharath Rupireddy
On Wed, Nov 11, 2020 at 01:34:05PM +0530, Bharath Rupireddy wrote:
> The ExecCheckRTPerms() with ACL_INSERT permission will be called
> before inserting the data to the table that's created with CREATE AS
> WITH NO DATA.

Perhaps you meant s/WITH NO DATA/WITH DATA/ here?

> The insertion into the table can happen either with
> INSERT command(ExecCheckRTPerms() with ACL_INSERT permission will be
> called from InitPlan()) or with COPY FROM command(ExecCheckRTPerms()
> with ACL_INSERT permission will be called from DoCopy()).
>
> Effectively, we are not bypassing the call to
> ExecutorCheckPerms_hook_type. Unless I miss anything else, I think it
> makes sense to skip ExecCheckRTPerms() for CTAS WITH NO DATA.

Oh, I see what you mean here.  If you have a EXPLAIN ANALYZE CTAS or
CTAS EXECUTE, then we forbid the creation of the table if the user has
no INSERT rights, while we actually allow the creation of the table
when using WITH NO DATA for a plain CTAS:
--- a/src/test/regress/sql/select_into.sql
+++ b/src/test/regress/sql/select_into.sql
@@ -34,6 +34,9 @@ SELECT oid AS clsoid, relname, relnatts + 10 AS x
 CREATE TABLE selinto_schema.tmp3 (a,b,c)
        AS SELECT oid,relname,relacl FROM pg_class
        WHERE relname like '%c%';    -- Error
+CREATE TABLE selinto_schema.tmp4 (a,b,c)
+      AS SELECT oid,relname,relacl FROM pg_class
+      WHERE relname like '%c%' WITH NO DATA; -- ok
+EXPLAIN ANALYZE CREATE TABLE selinto_schema.tmp5 (a,b,c)
+           AS SELECT oid,relname,relacl FROM pg_class
+          WHERE relname like '%c%' WITH NO DATA; -- error
 RESET SESSION AUTHORIZATION;

What your patch set does is to allow the second case to pass (or even
the EXECUTE case to pass).  HEAD is indeed a bit inconsistent as it is
now in this area.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Michael Paquier-2
In reply to this post by Bharath Rupireddy
On Wed, Nov 11, 2020 at 07:31:49PM +0530, Bharath Rupireddy wrote:
> Yes we do not have anything related to permissions mentioned in the
> documentation. So, I'm not adding it now.

It would be good to clarify that in the docs while we are on it.

> Apart from the above, I also noticed that we unnecessarily allocate
> bulk insert state(16MB memory) in case of WITH NO DATA, just to free
> it in intorel_shutdown() without actually using it. So, in the v2
> patch I have made changes to not allocate bulk insert state.
>
> Attaching v2 patch. Consider it for further review.

+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tmp0 (a) AS
+    SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK

I don't think this is sufficient.  Could you add more test cases here?
I can think of, coming down actually to the callers of
CreateIntoRelDestReceiver:
- A plain CTAS WITH NO DATA, that should pass,
- CTAS EXECUTE WITH NO DATA, that should pass.
- CTAS EXECUTE WITH DATA, that should not pass.
- EXPLAIN CTAS WITH DATA, that should not pass.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Bharath Rupireddy
Thanks for the comments.

On Thu, Nov 12, 2020 at 2:36 PM Michael Paquier <[hidden email]> wrote:
>
> On Wed, Nov 11, 2020 at 07:31:49PM +0530, Bharath Rupireddy wrote:
> > Yes we do not have anything related to permissions mentioned in the
> > documentation. So, I'm not adding it now.
>
> It would be good to clarify that in the docs while we are on it.
>

Added.

>
> I don't think this is sufficient.  Could you add more test cases here?
> I can think of, coming down actually to the callers of
> CreateIntoRelDestReceiver:
> - A plain CTAS WITH NO DATA, that should pass,
> - CTAS EXECUTE WITH NO DATA, that should pass.
> - CTAS EXECUTE WITH DATA, that should not pass.
> - EXPLAIN CTAS WITH DATA, that should not pass.
>

Done.

On HEAD/master, the behaviour is as follows: a) for plain CTAS WITH NO
DATA, ExecCheckRTPerms() will not be called. b) for explain analyze
CTAS WITH NO DATA, ExecCheckRTPerms() will be called. So, we need a).
This is what exactly this patch does i.e. ExecCheckRTPerms() will not
be called for both cases.

Attaching V3 patch, please review it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

v3-0001-Skip-Insert-Perm-Check-Bulk-Insert-State-alloc-in.patch (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Michael Paquier-2
On Thu, Nov 12, 2020 at 04:17:43PM +0530, Bharath Rupireddy wrote:
> On HEAD/master, the behaviour is as follows: a) for plain CTAS WITH NO
> DATA, ExecCheckRTPerms() will not be called. b) for explain analyze
> CTAS WITH NO DATA, ExecCheckRTPerms() will be called. So, we need a).
> This is what exactly this patch does i.e. ExecCheckRTPerms() will not
> be called for both cases.
>
> Attaching V3 patch, please review it.

+CREATE MATERIALIZED VIEW selinto_schema.mv_nodata4 (a) AS
+       SELECT oid FROM pg_class WHERE relname like '%c%' WITH DATA;
-- Error
+ERROR:  permission denied for materialized view mv_nodata4
Let's move any tests related to matviews to matviews.sql.  It does not
seem consistent to me to have those tests in a test path reserved to
CTAS, though I agree that there is some overlap and that setting up
the permissions requires a bit of duplication.

+   refreshed later using <command>REFRESH MATERIALIZED VIEW</command>. Insert
+   permission is checked on the materialized view before populating the data
+   (unless <command>WITH NO DATA</command> is specified).
Let's document that in a new paragraph, using "privilege" instead of
"permission", say (comment applies as well to the CTAS page):
CREATE MATERIALIZED VIEW requires CREATE privileges on the schema used
for the materialized view.  If using WITH DATA, the default, INSERT
privileges are also required.

+    * Check INSERT permission on the constructed table. Skip this check if
+    * WITH NO DATA is specified as we do not actually insert the tuples, we
+    * just create the table. The insert permissions will be checked anyways
+    * while inserting tuples into the table.
I would also use "privilege" here.  A nit.

    myState->reladdr = intoRelationAddr;
-   myState->output_cid = GetCurrentCommandId(true);
    myState->ti_options = TABLE_INSERT_SKIP_FSM;
-   myState->bistate = GetBulkInsertState();
+   myState->output_cid = GetCurrentCommandId(true);
The changes related to the bulk-insert state data look fine per se.
One nit: I would set bistate to NULL for the data-skip case here.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Bharath Rupireddy
Thanks for the comments.

On Fri, Nov 13, 2020 at 9:19 AM Michael Paquier <[hidden email]> wrote:
>
> Let's move any tests related to matviews to matviews.sql.  It does not
> seem consistent to me to have those tests in a test path reserved to
> CTAS, though I agree that there is some overlap and that setting up
> the permissions requires a bit of duplication.
>

Done.

>
> "permission", say (comment applies as well to the CTAS page):
> CREATE MATERIALIZED VIEW requires CREATE privileges on the schema used
> for the materialized view.  If using WITH DATA, the default, INSERT
> privileges are also required.
>

Done.

>
> +    * Check INSERT permission on the constructed table. Skip this check if
> +    * WITH NO DATA is specified as we do not actually insert the tuples, we
> +    * just create the table. The insert permissions will be checked anyways
> +    * while inserting tuples into the table.
> I would also use "privilege" here.  A nit.
>

Done.

>     myState->reladdr = intoRelationAddr;
> -   myState->output_cid = GetCurrentCommandId(true);
>     myState->ti_options = TABLE_INSERT_SKIP_FSM;
> -   myState->bistate = GetBulkInsertState();
> +   myState->output_cid = GetCurrentCommandId(true);
> The changes related to the bulk-insert state data look fine per se.
> One nit: I would set bistate to NULL for the data-skip case here.
>

It's not required to set bistate to null as we have allocated myState
with palloc0 in CreateIntoRelDestReceiver, it will anyways be null.
  if (!into->skipData)
        myState->bistate = GetBulkInsertState();

Attaching v4 patch. Please review it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

v4-0001-Skip-Insert-Perm-Check-Bulk-Insert-State-alloc-in.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Michael Paquier-2
On Fri, Nov 13, 2020 at 12:58:52PM +0530, Bharath Rupireddy wrote:
> It's not required to set bistate to null as we have allocated myState
> with palloc0 in CreateIntoRelDestReceiver, it will anyways be null.
>   if (!into->skipData)
>         myState->bistate = GetBulkInsertState();
>
> Attaching v4 patch. Please review it.

I have reviewed this one this morning, and applied it after some
tweaks.  I have reworded some of the comments, fixed some typos, and
largely refactored the test cases to stress all the combinations
possible.  Please note that your patch would have caused failures
in the buildfarm, as any role created needs to be prefixed with
"regress_".
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Peter Eisentraut-7
On 2020-11-16 04:04, Michael Paquier wrote:

> On Fri, Nov 13, 2020 at 12:58:52PM +0530, Bharath Rupireddy wrote:
>> It's not required to set bistate to null as we have allocated myState
>> with palloc0 in CreateIntoRelDestReceiver, it will anyways be null.
>>    if (!into->skipData)
>>          myState->bistate = GetBulkInsertState();
>>
>> Attaching v4 patch. Please review it.
>
> I have reviewed this one this morning, and applied it after some
> tweaks.  I have reworded some of the comments, fixed some typos, and
> largely refactored the test cases to stress all the combinations
> possible.  Please note that your patch would have caused failures
> in the buildfarm, as any role created needs to be prefixed with
> "regress_".

While this patch was nice enough to update the documentation about the
requirement of the INSERT privilege, this is maybe more confusing now:
How could a new table not have INSERT privilege?  Yes, you can do that
with default privileges, but that's not well known and should be
clarified in the documentation.

The SQL standard says that for CREATE TABLE AS, the INSERT "is
effectively executed without further Access Rule checking", which means
the INSERT privilege shouldn't be required at all.  I suggest we
consider doing that instead.  I don't see a use for the current behavior.


Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Michael Paquier-2
On Mon, Nov 16, 2020 at 04:01:33PM +0100, Peter Eisentraut wrote:

> While this patch was nice enough to update the documentation about the
> requirement of the INSERT privilege, this is maybe more confusing now: How
> could a new table not have INSERT privilege?  Yes, you can do that with
> default privileges, but that's not well known and should be clarified in the
> documentation.
>
> The SQL standard says that for CREATE TABLE AS, the INSERT "is effectively
> executed without further Access Rule checking", which means the INSERT
> privilege shouldn't be required at all.  I suggest we consider doing that
> instead.  I don't see a use for the current behavior.
Hmm.  Is there anything specific for materialized views?  They've
checked for INSERT privileges at this phase since their introduction
in 3bf3ab8c.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Peter Eisentraut-7
On 2020-11-17 02:32, Michael Paquier wrote:
>> The SQL standard says that for CREATE TABLE AS, the INSERT "is effectively
>> executed without further Access Rule checking", which means the INSERT
>> privilege shouldn't be required at all.  I suggest we consider doing that
>> instead.  I don't see a use for the current behavior.
> Hmm.  Is there anything specific for materialized views?  They've
> checked for INSERT privileges at this phase since their introduction
> in 3bf3ab8c.

Materialized views are not in the SQL standard.

But if you consider materialized views as a variant of normal views,
then the INSERT privilege would be applicable if you pass an INSERT on
the materialized view through to the underlying tables, like for a view.

Also note that REFRESH on a materialized view does not check any
privileges (only ownership), so having a privilege that only applies
when the materialized view is created doesn't make sense.


Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Bharath Rupireddy
On Thu, Nov 19, 2020 at 8:47 PM Peter Eisentraut
<[hidden email]> wrote:

>
> On 2020-11-17 02:32, Michael Paquier wrote:
> >> The SQL standard says that for CREATE TABLE AS, the INSERT "is effectively
> >> executed without further Access Rule checking", which means the INSERT
> >> privilege shouldn't be required at all.  I suggest we consider doing that
> >> instead.  I don't see a use for the current behavior.
> > Hmm.  Is there anything specific for materialized views?  They've
> > checked for INSERT privileges at this phase since their introduction
> > in 3bf3ab8c.
>
> Materialized views are not in the SQL standard.
>
> But if you consider materialized views as a variant of normal views,
> then the INSERT privilege would be applicable if you pass an INSERT on
> the materialized view through to the underlying tables, like for a view.
>
> Also note that REFRESH on a materialized view does not check any
> privileges (only ownership), so having a privilege that only applies
> when the materialized view is created doesn't make sense.
>

So, should we be doing it this way?

For CTAS: retain the existing CREATE privilege check and remove the
INSERT privilege check altogether for all the cases i.e. with data,
with no data, explain analyze, plain, with execute?
For CREATE MATERIALIZED VIEW: same as CTAS, that is retain the
existing CREATE privilege check and remove the INSERT privilege check
for with data, with no data, explain analyze, plain?
For REFRESH MATERIALIZED VIEW: retain the existing behaviour i.e. no
privilege check.

If okay, I can make a patch.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Michael Paquier-2
On Thu, Nov 19, 2020 at 10:05:19PM +0530, Bharath Rupireddy wrote:
> On Thu, Nov 19, 2020 at 8:47 PM Peter Eisentraut <[hidden email]> wrote:
>> Materialized views are not in the SQL standard.
>>
>> But if you consider materialized views as a variant of normal views,
>> then the INSERT privilege would be applicable if you pass an INSERT on
>> the materialized view through to the underlying tables, like for a view.

INSERT to materialized views is not supported, but perhaps you mean
having a variant of auto updatable for matviews?  I am not sure how to
clearly define that.

> For CTAS: retain the existing CREATE privilege check and remove the
> INSERT privilege check altogether for all the cases i.e. with data,
> with no data, explain analyze, plain, with execute?
> For CREATE MATERIALIZED VIEW: same as CTAS, that is retain the
> existing CREATE privilege check and remove the INSERT privilege check
> for with data, with no data, explain analyze, plain?
> For REFRESH MATERIALIZED VIEW: retain the existing behaviour i.e. no
> privilege check.

Thanks.  Based on what Peter has said, the ACL_INSERT check in
intorel_startup() could just be removed, and the tests of matview.sql
and select_into.sql would need some cleanup.  We could keep around
some scenarios with some follow-up INSERT queries after the initial
creation.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Skip ExecCheckRTPerms in CTAS with no data

Peter Eisentraut-7
In reply to this post by Bharath Rupireddy
On 2020-11-19 17:35, Bharath Rupireddy wrote:

> So, should we be doing it this way?
>
> For CTAS: retain the existing CREATE privilege check and remove the
> INSERT privilege check altogether for all the cases i.e. with data,
> with no data, explain analyze, plain, with execute?
> For CREATE MATERIALIZED VIEW: same as CTAS, that is retain the
> existing CREATE privilege check and remove the INSERT privilege check
> for with data, with no data, explain analyze, plain?
> For REFRESH MATERIALIZED VIEW: retain the existing behaviour i.e. no
> privilege check.

That sounds reasonable to me.


12