Horton

Thread

Commits

  1. Remove INSERT privilege check at table creation of CTAS and matview

  2. Relax INSERT privilege requirement for CTAS and matviews WITH NO DATA

  1. Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-09-28T12:28:49Z

    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

  2. Re: Skip ExecCheckRTPerms in CTAS with no data

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-28T14:18:17Z

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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

  3. Re: Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-09-29T11:39:06Z

    On Mon, Sep 28, 2020 at 7:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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

  4. Re: Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-10-07T12:38:16Z

    On Tue, Sep 29, 2020 at 5:09 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Sep 28, 2020 at 7:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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

  5. Re: Skip ExecCheckRTPerms in CTAS with no data

    Anastasia Lubennikova <a.lubennikova@postgrespro.ru> — 2020-11-09T19:48:09Z

    On 29.09.2020 14:39, Bharath Rupireddy wrote:
> On Mon, Sep 28, 2020 at 7:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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

  6. Re: Skip ExecCheckRTPerms in CTAS with no data

    Michael Paquier <michael@paquier.xyz> — 2020-11-11T06:35:25Z

    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

  7. Re: Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-11-11T08:04:05Z

    On Wed, Nov 11, 2020 at 12:05 PM Michael Paquier <michael@paquier.xyz> 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

  8. Re: Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-11-11T14:01:49Z

    On Tue, Nov 10, 2020 at 1:18 AM Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> 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

  9. Re: Skip ExecCheckRTPerms in CTAS with no data

    Michael Paquier <michael@paquier.xyz> — 2020-11-12T08:19:09Z

    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

  10. Re: Skip ExecCheckRTPerms in CTAS with no data

    Michael Paquier <michael@paquier.xyz> — 2020-11-12T09:06:07Z

    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

  11. Re: Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-11-12T10:47:43Z

    Thanks for the comments.

On Thu, Nov 12, 2020 at 2:36 PM Michael Paquier <michael@paquier.xyz> 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

  12. Re: Skip ExecCheckRTPerms in CTAS with no data

    Michael Paquier <michael@paquier.xyz> — 2020-11-13T03:49:33Z

    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

  13. Re: Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-11-13T07:28:52Z

    Thanks for the comments.

On Fri, Nov 13, 2020 at 9:19 AM Michael Paquier <michael@paquier.xyz> 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

  14. Re: Skip ExecCheckRTPerms in CTAS with no data

    Michael Paquier <michael@paquier.xyz> — 2020-11-16T03:04:31Z

    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

  15. Re: Skip ExecCheckRTPerms in CTAS with no data

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-11-16T15:01:33Z

    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.

  16. Re: Skip ExecCheckRTPerms in CTAS with no data

    Michael Paquier <michael@paquier.xyz> — 2020-11-17T01:32:16Z

    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

  17. Re: Skip ExecCheckRTPerms in CTAS with no data

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-11-19T15:17:32Z

    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.

  18. Re: Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-11-19T16:35:19Z

    On Thu, Nov 19, 2020 at 8:47 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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

  19. Re: Skip ExecCheckRTPerms in CTAS with no data

    Michael Paquier <michael@paquier.xyz> — 2020-11-20T05:37:28Z

    On Thu, Nov 19, 2020 at 10:05:19PM +0530, Bharath Rupireddy wrote:
> On Thu, Nov 19, 2020 at 8:47 PM Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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

  20. Re: Skip ExecCheckRTPerms in CTAS with no data

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-11-20T07:27:52Z

    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.

  21. Re: Skip ExecCheckRTPerms in CTAS with no data

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-11-20T07:29:19Z

    On 2020-11-20 06:37, Michael Paquier wrote:
>>> 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.

Not currently, but it could be a future feature.  Basically an insert 
would be passed on to the underlying tables (using INSTEAD triggers), 
and then a refresh would be triggered automatically.

  22. Re: Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-11-20T09:21:30Z

    On Fri, Nov 20, 2020 at 12:59 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 2020-11-20 06:37, Michael Paquier wrote:
> >>> 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.
>
> Not currently, but it could be a future feature.  Basically an insert
> would be passed on to the underlying tables (using INSTEAD triggers),
> and then a refresh would be triggered automatically.
>

Sounds interesting! Just a thought: I think instead of just auto
updating/refreshing materialized view for every single row inserted,
maybe we could do it for a bunch of rows.

If not with triggers, another way to achieve the auto updatable
matviews functionality is by having a dedicated bgworker(which is by
default switched off/not spawned). This worker can get the list of
matviews and if the amount of rows changed in the underlying tables
crosses a certain configurable limit, then refresh them using existing
refresh matview infrastructure. Thoughts?

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

  23. Re: Skip ExecCheckRTPerms in CTAS with no data

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-11-20T09:34:57Z

    On Fri, Nov 20, 2020 at 11:07 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.
>

Thanks! Attaching the patch. Please review it.


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

  24. Re: Skip ExecCheckRTPerms in CTAS with no data

    Michael Paquier <michael@paquier.xyz> — 2020-11-21T10:50:05Z

    On Fri, Nov 20, 2020 at 03:04:57PM +0530, Bharath Rupireddy wrote:
> Thanks! Attaching the patch. Please review it.

Thanks.  I have removed the references to the INSERT check in the
comments and the docs, because that would be confusing as it refers
to something we don't do anymore now with this patch, reordered the
tests and applied the patch.
--
Michael