Skip ExecCheckRTPerms in CTAS with no data
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
Attachments:
v1-Skip-ExecCheckRTPerms-in-CTAS-with-no-data.patchapplication/octet-stream; name=v1-Skip-ExecCheckRTPerms-in-CTAS-with-no-data.patchDownload
From fe41f03db274c0988bc6c2c1def8a83fbed01213 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Mon, 28 Sep 2020 17:44:56 +0530
Subject: [PATCH v1] Skip ExecCheckRTPerms in CTAS with no data
In 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.
---
src/backend/commands/createas.c | 38 ++++++++++++++++++---------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index d53ec952d0..b36422280f 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -436,7 +436,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
List *attrList;
ObjectAddress intoRelationAddr;
Relation intoRelationDesc;
- RangeTblEntry *rte;
ListCell *lc;
int attnum;
@@ -507,23 +506,28 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
intoRelationDesc = table_open(intoRelationAddr.objectId, AccessExclusiveLock);
/*
- * Check INSERT permission on the constructed table.
- *
- * XXX: It would arguably make sense to skip this check if into->skipData
- * is true.
+ * Check INSERT permission on the constructed table. Skip this check if
+ * into->skipData is true, as we do not actually insert the tuples, we
+ * just create the table. The insert permissions will be checked later,
+ * while inserting tuples into the table.
*/
- rte = makeNode(RangeTblEntry);
- rte->rtekind = RTE_RELATION;
- rte->relid = intoRelationAddr.objectId;
- rte->relkind = relkind;
- rte->rellockmode = RowExclusiveLock;
- rte->requiredPerms = ACL_INSERT;
-
- for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
- rte->insertedCols = bms_add_member(rte->insertedCols,
- attnum - FirstLowInvalidHeapAttributeNumber);
-
- ExecCheckRTPerms(list_make1(rte), true);
+ if (!into->skipData)
+ {
+ RangeTblEntry *rte;
+
+ rte = makeNode(RangeTblEntry);
+ rte->rtekind = RTE_RELATION;
+ rte->relid = intoRelationAddr.objectId;
+ rte->relkind = relkind;
+ rte->rellockmode = RowExclusiveLock;
+ rte->requiredPerms = ACL_INSERT;
+
+ for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
+ rte->insertedCols = bms_add_member(rte->insertedCols,
+ attnum - FirstLowInvalidHeapAttributeNumber);
+
+ ExecCheckRTPerms(list_make1(rte), true);
+ }
/*
* Make sure the constructed table does not have RLS enabled.
--
2.25.1
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
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
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
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
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
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
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
Attachments:
v2-Skip-Insert-Perm-Check-Bulk-Insert-State-alloc-in-WITH-NO-DATA.patchapplication/octet-stream; name=v2-Skip-Insert-Perm-Check-Bulk-Insert-State-alloc-in-WITH-NO-DATA.patchDownload
From 9777849fd2b05e4f55101d7423dd18c3642e5829 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Wed, 11 Nov 2020 19:17:32 +0530
Subject: [PATCH v2] Skip Insert Perm Check & Bulk Insert State alloc in CTAS
with no data
In 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. And also, do not
allocate bulk insert state i.e. 16MB of data just to free it
in intorel_shutdown() without having to use it.
---
src/backend/commands/createas.c | 87 ++++++++++++++---------
src/test/regress/expected/select_into.out | 17 ++++-
src/test/regress/sql/select_into.sql | 5 ++
3 files changed, 73 insertions(+), 36 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index d53ec952d0..b17c5b4dfa 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -436,7 +436,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
List *attrList;
ObjectAddress intoRelationAddr;
Relation intoRelationDesc;
- RangeTblEntry *rte;
ListCell *lc;
int attnum;
@@ -507,23 +506,28 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
intoRelationDesc = table_open(intoRelationAddr.objectId, AccessExclusiveLock);
/*
- * Check INSERT permission on the constructed table.
- *
- * XXX: It would arguably make sense to skip this check if into->skipData
- * is true.
+ * 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.
*/
- rte = makeNode(RangeTblEntry);
- rte->rtekind = RTE_RELATION;
- rte->relid = intoRelationAddr.objectId;
- rte->relkind = relkind;
- rte->rellockmode = RowExclusiveLock;
- rte->requiredPerms = ACL_INSERT;
+ if (!into->skipData)
+ {
+ RangeTblEntry *rte;
+
+ rte = makeNode(RangeTblEntry);
+ rte->rtekind = RTE_RELATION;
+ rte->relid = intoRelationAddr.objectId;
+ rte->relkind = relkind;
+ rte->rellockmode = RowExclusiveLock;
+ rte->requiredPerms = ACL_INSERT;
- for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
- rte->insertedCols = bms_add_member(rte->insertedCols,
- attnum - FirstLowInvalidHeapAttributeNumber);
+ for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
+ rte->insertedCols = bms_add_member(rte->insertedCols,
+ attnum - FirstLowInvalidHeapAttributeNumber);
- ExecCheckRTPerms(list_make1(rte), true);
+ ExecCheckRTPerms(list_make1(rte), true);
+ }
/*
* Make sure the constructed table does not have RLS enabled.
@@ -550,9 +554,15 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
*/
myState->rel = intoRelationDesc;
myState->reladdr = intoRelationAddr;
- myState->output_cid = GetCurrentCommandId(true);
myState->ti_options = TABLE_INSERT_SKIP_FSM;
- myState->bistate = GetBulkInsertState();
+ myState->output_cid = GetCurrentCommandId(true);
+
+ /*
+ * In case if WITH NO DATA is specified, we do not allocate bulk insert
+ * state as we do not have tuples to insert.
+ */
+ if (!into->skipData)
+ myState->bistate = GetBulkInsertState();
/*
* Valid smgr_targblock implies something already wrote to the relation.
@@ -569,20 +579,23 @@ intorel_receive(TupleTableSlot *slot, DestReceiver *self)
{
DR_intorel *myState = (DR_intorel *) self;
- /*
- * Note that the input slot might not be of the type of the target
- * relation. That's supported by table_tuple_insert(), but slightly less
- * efficient than inserting with the right slot - but the alternative
- * would be to copy into a slot of the right type, which would not be
- * cheap either. This also doesn't allow accessing per-AM data (say a
- * tuple's xmin), but since we don't do that here...
- */
-
- table_tuple_insert(myState->rel,
- slot,
- myState->output_cid,
- myState->ti_options,
- myState->bistate);
+ /* Do not insert in case if WITH NO DATA is specified. */
+ if (!myState->into->skipData)
+ {
+ /*
+ * Note that the input slot might not be of the type of the target
+ * relation. That's supported by table_tuple_insert(), but slightly
+ * less efficient than inserting with the right slot - but the
+ * alternative would be to copy into a slot of the right type, which
+ * would not be cheap either. This also doesn't allow accessing per-AM
+ * data (say a tuple's xmin), but since we don't do that here...
+ */
+ table_tuple_insert(myState->rel,
+ slot,
+ myState->output_cid,
+ myState->ti_options,
+ myState->bistate);
+ }
/* We know this is a newly created relation, so there are no indexes */
@@ -597,9 +610,15 @@ intorel_shutdown(DestReceiver *self)
{
DR_intorel *myState = (DR_intorel *) self;
- FreeBulkInsertState(myState->bistate);
-
- table_finish_bulk_insert(myState->rel, myState->ti_options);
+ /*
+ * In case if WITH NO DATA is specified, we do not allocate bulk insert
+ * state as we do not have tuples to insert.
+ */
+ if (!myState->into->skipData)
+ {
+ FreeBulkInsertState(myState->bistate);
+ table_finish_bulk_insert(myState->rel, myState->ti_options);
+ }
/* close rel, but keep lock until commit */
table_close(myState->rel, NoLock);
diff --git a/src/test/regress/expected/select_into.out b/src/test/regress/expected/select_into.out
index f373fae679..0e10eb39cf 100644
--- a/src/test/regress/expected/select_into.out
+++ b/src/test/regress/expected/select_into.out
@@ -31,6 +31,18 @@ CREATE TABLE selinto_schema.tmp3 (a,b,c)
AS SELECT oid,relname,relacl FROM pg_class
WHERE relname like '%c%'; -- Error
ERROR: permission denied for table tmp3
+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
+ QUERY PLAN
+---------------------------------------
+ Seq Scan on pg_class (never executed)
+ Filter: (relname ~~ '%c%'::text)
+(2 rows)
+
+INSERT INTO selinto_schema.tmp0
+ SELECT oid FROM pg_class WHERE relname like '%c%'; -- Error
+ERROR: permission denied for table tmp0
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
GRANT INSERT ON TABLES TO regress_selinto_user;
@@ -45,8 +57,9 @@ CREATE TABLE selinto_schema.tmp3 (a,b,c)
WHERE relname like '%c%'; -- OK
RESET SESSION AUTHORIZATION;
DROP SCHEMA selinto_schema CASCADE;
-NOTICE: drop cascades to 3 other objects
-DETAIL: drop cascades to table selinto_schema.tmp1
+NOTICE: drop cascades to 4 other objects
+DETAIL: drop cascades to table selinto_schema.tmp0
+drop cascades to table selinto_schema.tmp1
drop cascades to table selinto_schema.tmp2
drop cascades to table selinto_schema.tmp3
DROP USER regress_selinto_user;
diff --git a/src/test/regress/sql/select_into.sql b/src/test/regress/sql/select_into.sql
index a708fef0ea..c6e157a465 100644
--- a/src/test/regress/sql/select_into.sql
+++ b/src/test/regress/sql/select_into.sql
@@ -34,6 +34,11 @@ 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
+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
+INSERT INTO selinto_schema.tmp0
+ SELECT oid FROM pg_class WHERE relname like '%c%'; -- Error
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
--
2.25.1
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
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; -- OKI 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
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
Attachments:
v3-0001-Skip-Insert-Perm-Check-Bulk-Insert-State-alloc-in.patchapplication/octet-stream; name=v3-0001-Skip-Insert-Perm-Check-Bulk-Insert-State-alloc-in.patchDownload
From fdfeacac480fe09a38083c01c4f665799872605d Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Thu, 12 Nov 2020 16:02:37 +0530
Subject: [PATCH v3] Skip Insert Perm Check & Bulk Insert State alloc in CTAS
with no data
In 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. And also, do not
allocate bulk insert state i.e. 16MB of data just to free it
in intorel_shutdown() without having to use it.
---
.../sgml/ref/create_materialized_view.sgml | 4 +-
doc/src/sgml/ref/create_table_as.sgml | 4 +-
src/backend/commands/createas.c | 87 +++++++++++--------
src/test/regress/expected/select_into.out | 59 ++++++++++++-
src/test/regress/sql/select_into.sql | 27 ++++++
5 files changed, 143 insertions(+), 38 deletions(-)
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml
index 5ba851b687..b8ef54f45b 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -38,7 +38,9 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable>
<command>CREATE MATERIALIZED VIEW</command> defines a materialized view of
a query. The query is executed and used to populate the view at the time
the command is issued (unless <command>WITH NO DATA</command> is used) and may be
- refreshed later using <command>REFRESH MATERIALIZED VIEW</command>.
+ 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).
</para>
<para>
diff --git a/doc/src/sgml/ref/create_table_as.sgml b/doc/src/sgml/ref/create_table_as.sgml
index bcbd73b227..416a2c0f94 100644
--- a/doc/src/sgml/ref/create_table_as.sgml
+++ b/doc/src/sgml/ref/create_table_as.sgml
@@ -41,7 +41,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
The table columns have the
names and data types associated with the output columns of the
<command>SELECT</command> (except that you can override the column
- names by giving an explicit list of new column names).
+ names by giving an explicit list of new column names). Insert permission is
+ checked on the created table before filling the data (unless
+ <command>WITH NO DATA</command> is specified).
</para>
<para>
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index d53ec952d0..b17c5b4dfa 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -436,7 +436,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
List *attrList;
ObjectAddress intoRelationAddr;
Relation intoRelationDesc;
- RangeTblEntry *rte;
ListCell *lc;
int attnum;
@@ -507,23 +506,28 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
intoRelationDesc = table_open(intoRelationAddr.objectId, AccessExclusiveLock);
/*
- * Check INSERT permission on the constructed table.
- *
- * XXX: It would arguably make sense to skip this check if into->skipData
- * is true.
+ * 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.
*/
- rte = makeNode(RangeTblEntry);
- rte->rtekind = RTE_RELATION;
- rte->relid = intoRelationAddr.objectId;
- rte->relkind = relkind;
- rte->rellockmode = RowExclusiveLock;
- rte->requiredPerms = ACL_INSERT;
+ if (!into->skipData)
+ {
+ RangeTblEntry *rte;
+
+ rte = makeNode(RangeTblEntry);
+ rte->rtekind = RTE_RELATION;
+ rte->relid = intoRelationAddr.objectId;
+ rte->relkind = relkind;
+ rte->rellockmode = RowExclusiveLock;
+ rte->requiredPerms = ACL_INSERT;
- for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
- rte->insertedCols = bms_add_member(rte->insertedCols,
- attnum - FirstLowInvalidHeapAttributeNumber);
+ for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
+ rte->insertedCols = bms_add_member(rte->insertedCols,
+ attnum - FirstLowInvalidHeapAttributeNumber);
- ExecCheckRTPerms(list_make1(rte), true);
+ ExecCheckRTPerms(list_make1(rte), true);
+ }
/*
* Make sure the constructed table does not have RLS enabled.
@@ -550,9 +554,15 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
*/
myState->rel = intoRelationDesc;
myState->reladdr = intoRelationAddr;
- myState->output_cid = GetCurrentCommandId(true);
myState->ti_options = TABLE_INSERT_SKIP_FSM;
- myState->bistate = GetBulkInsertState();
+ myState->output_cid = GetCurrentCommandId(true);
+
+ /*
+ * In case if WITH NO DATA is specified, we do not allocate bulk insert
+ * state as we do not have tuples to insert.
+ */
+ if (!into->skipData)
+ myState->bistate = GetBulkInsertState();
/*
* Valid smgr_targblock implies something already wrote to the relation.
@@ -569,20 +579,23 @@ intorel_receive(TupleTableSlot *slot, DestReceiver *self)
{
DR_intorel *myState = (DR_intorel *) self;
- /*
- * Note that the input slot might not be of the type of the target
- * relation. That's supported by table_tuple_insert(), but slightly less
- * efficient than inserting with the right slot - but the alternative
- * would be to copy into a slot of the right type, which would not be
- * cheap either. This also doesn't allow accessing per-AM data (say a
- * tuple's xmin), but since we don't do that here...
- */
-
- table_tuple_insert(myState->rel,
- slot,
- myState->output_cid,
- myState->ti_options,
- myState->bistate);
+ /* Do not insert in case if WITH NO DATA is specified. */
+ if (!myState->into->skipData)
+ {
+ /*
+ * Note that the input slot might not be of the type of the target
+ * relation. That's supported by table_tuple_insert(), but slightly
+ * less efficient than inserting with the right slot - but the
+ * alternative would be to copy into a slot of the right type, which
+ * would not be cheap either. This also doesn't allow accessing per-AM
+ * data (say a tuple's xmin), but since we don't do that here...
+ */
+ table_tuple_insert(myState->rel,
+ slot,
+ myState->output_cid,
+ myState->ti_options,
+ myState->bistate);
+ }
/* We know this is a newly created relation, so there are no indexes */
@@ -597,9 +610,15 @@ intorel_shutdown(DestReceiver *self)
{
DR_intorel *myState = (DR_intorel *) self;
- FreeBulkInsertState(myState->bistate);
-
- table_finish_bulk_insert(myState->rel, myState->ti_options);
+ /*
+ * In case if WITH NO DATA is specified, we do not allocate bulk insert
+ * state as we do not have tuples to insert.
+ */
+ if (!myState->into->skipData)
+ {
+ FreeBulkInsertState(myState->bistate);
+ table_finish_bulk_insert(myState->rel, myState->ti_options);
+ }
/* close rel, but keep lock until commit */
table_close(myState->rel, NoLock);
diff --git a/src/test/regress/expected/select_into.out b/src/test/regress/expected/select_into.out
index f373fae679..415c281019 100644
--- a/src/test/regress/expected/select_into.out
+++ b/src/test/regress/expected/select_into.out
@@ -31,6 +31,55 @@ CREATE TABLE selinto_schema.tmp3 (a,b,c)
AS SELECT oid,relname,relacl FROM pg_class
WHERE relname like '%c%'; -- Error
ERROR: permission denied for table tmp3
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata1 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+ QUERY PLAN
+---------------------------------------
+ Seq Scan on pg_class (never executed)
+ Filter: (relname ~~ '%c%'::text)
+(2 rows)
+
+INSERT INTO selinto_schema.tbl_nodata1
+ SELECT oid FROM pg_class WHERE relname like '%c%'; -- Error
+ERROR: permission denied for table tbl_nodata1
+CREATE TABLE selinto_schema.tbl_nodata2 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+PREPARE nodata_sel AS
+ SELECT oid FROM pg_class WHERE relname like '%c%';
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata3 (a) AS
+ EXECUTE nodata_sel WITH NO DATA; -- OK
+ QUERY PLAN
+---------------------------------------
+ Seq Scan on pg_class (never executed)
+ Filter: (relname ~~ '%c%'::text)
+(2 rows)
+
+CREATE TABLE selinto_schema.tbl_nodata4 (a) AS
+ EXECUTE nodata_sel WITH NO DATA; -- OK
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata5 (a) AS
+ EXECUTE nodata_sel WITH DATA; -- Error
+ERROR: permission denied for table tbl_nodata5
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE MATERIALIZED VIEW selinto_schema.mv_nodata1 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+ QUERY PLAN
+---------------------------------------
+ Seq Scan on pg_class (never executed)
+ Filter: (relname ~~ '%c%'::text)
+(2 rows)
+
+CREATE MATERIALIZED VIEW selinto_schema.mv_nodata2 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE MATERIALIZED VIEW selinto_schema.mv_nodata3 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH DATA; -- Error
+ERROR: permission denied for materialized view mv_nodata3
+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
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
GRANT INSERT ON TABLES TO regress_selinto_user;
@@ -45,8 +94,14 @@ CREATE TABLE selinto_schema.tmp3 (a,b,c)
WHERE relname like '%c%'; -- OK
RESET SESSION AUTHORIZATION;
DROP SCHEMA selinto_schema CASCADE;
-NOTICE: drop cascades to 3 other objects
-DETAIL: drop cascades to table selinto_schema.tmp1
+NOTICE: drop cascades to 9 other objects
+DETAIL: drop cascades to table selinto_schema.tbl_nodata1
+drop cascades to table selinto_schema.tbl_nodata2
+drop cascades to table selinto_schema.tbl_nodata3
+drop cascades to table selinto_schema.tbl_nodata4
+drop cascades to materialized view selinto_schema.mv_nodata1
+drop cascades to materialized view selinto_schema.mv_nodata2
+drop cascades to table selinto_schema.tmp1
drop cascades to table selinto_schema.tmp2
drop cascades to table selinto_schema.tmp3
DROP USER regress_selinto_user;
diff --git a/src/test/regress/sql/select_into.sql b/src/test/regress/sql/select_into.sql
index a708fef0ea..8eb2fda40c 100644
--- a/src/test/regress/sql/select_into.sql
+++ b/src/test/regress/sql/select_into.sql
@@ -34,6 +34,33 @@ 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
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata1 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+INSERT INTO selinto_schema.tbl_nodata1
+ SELECT oid FROM pg_class WHERE relname like '%c%'; -- Error
+CREATE TABLE selinto_schema.tbl_nodata2 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+PREPARE nodata_sel AS
+ SELECT oid FROM pg_class WHERE relname like '%c%';
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata3 (a) AS
+ EXECUTE nodata_sel WITH NO DATA; -- OK
+CREATE TABLE selinto_schema.tbl_nodata4 (a) AS
+ EXECUTE nodata_sel WITH NO DATA; -- OK
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata5 (a) AS
+ EXECUTE nodata_sel WITH DATA; -- Error
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE MATERIALIZED VIEW selinto_schema.mv_nodata1 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+CREATE MATERIALIZED VIEW selinto_schema.mv_nodata2 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE MATERIALIZED VIEW selinto_schema.mv_nodata3 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH DATA; -- Error
+CREATE MATERIALIZED VIEW selinto_schema.mv_nodata4 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH DATA; -- Error
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
--
2.25.1
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.
--
MichaelThanks 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
Attachments:
v4-0001-Skip-Insert-Perm-Check-Bulk-Insert-State-alloc-in.patchapplication/octet-stream; name=v4-0001-Skip-Insert-Perm-Check-Bulk-Insert-State-alloc-in.patchDownload
From 31a42dd6ca2fe23cdfe46c5e0cdc21ebd9d0639a Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Fri, 13 Nov 2020 12:43:05 +0530
Subject: [PATCH v4] Skip Insert Perm Check & Bulk Insert State alloc in CTAS
with no data
In 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. And also, do not
allocate bulk insert state i.e. 16MB of data just to free it
in intorel_shutdown() without having to use it.
---
.../sgml/ref/create_materialized_view.sgml | 7 ++
doc/src/sgml/ref/create_table_as.sgml | 7 ++
src/backend/commands/createas.c | 87 +++++++++++--------
src/test/regress/expected/matview.out | 33 +++++++
src/test/regress/expected/select_into.out | 39 ++++++++-
src/test/regress/sql/matview.sql | 26 ++++++
src/test/regress/sql/select_into.sql | 17 ++++
7 files changed, 180 insertions(+), 36 deletions(-)
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml
index 5ba851b687..e4ea049eff 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -48,6 +48,13 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable>
A materialized view has many of the same properties as a table, but there
is no support for temporary materialized views.
</para>
+
+ <para>
+ <command>CREATE MATERIALIZED VIEW</command> requires
+ <literal>CREATE</literal> privilege on the schema used for the materialized
+ view. If using <command>WITH DATA</command>, the default,
+ <literal>INSERT</literal> privilege is also required.
+ </para>
</refsect1>
<refsect1>
diff --git a/doc/src/sgml/ref/create_table_as.sgml b/doc/src/sgml/ref/create_table_as.sgml
index bcbd73b227..2cac4e3ec0 100644
--- a/doc/src/sgml/ref/create_table_as.sgml
+++ b/doc/src/sgml/ref/create_table_as.sgml
@@ -53,6 +53,13 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
defining <command>SELECT</command> statement whenever it is
queried.
</para>
+
+ <para>
+ <command>CREATE TABLE AS</command> requires <literal>CREATE</literal>
+ privilege on the schema used for the table. If using
+ <command>WITH DATA</command>, the default, <literal>INSERT</literal>
+ privilege is also required.
+ </para>
</refsect1>
<refsect1>
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index d53ec952d0..13bfd7711d 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -436,7 +436,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
List *attrList;
ObjectAddress intoRelationAddr;
Relation intoRelationDesc;
- RangeTblEntry *rte;
ListCell *lc;
int attnum;
@@ -507,23 +506,28 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
intoRelationDesc = table_open(intoRelationAddr.objectId, AccessExclusiveLock);
/*
- * Check INSERT permission on the constructed table.
- *
- * XXX: It would arguably make sense to skip this check if into->skipData
- * is true.
+ * Check INSERT privilege 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 privilege will be checked anyways
+ * while inserting tuples into the table.
*/
- rte = makeNode(RangeTblEntry);
- rte->rtekind = RTE_RELATION;
- rte->relid = intoRelationAddr.objectId;
- rte->relkind = relkind;
- rte->rellockmode = RowExclusiveLock;
- rte->requiredPerms = ACL_INSERT;
+ if (!into->skipData)
+ {
+ RangeTblEntry *rte;
+
+ rte = makeNode(RangeTblEntry);
+ rte->rtekind = RTE_RELATION;
+ rte->relid = intoRelationAddr.objectId;
+ rte->relkind = relkind;
+ rte->rellockmode = RowExclusiveLock;
+ rte->requiredPerms = ACL_INSERT;
- for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
- rte->insertedCols = bms_add_member(rte->insertedCols,
- attnum - FirstLowInvalidHeapAttributeNumber);
+ for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
+ rte->insertedCols = bms_add_member(rte->insertedCols,
+ attnum - FirstLowInvalidHeapAttributeNumber);
- ExecCheckRTPerms(list_make1(rte), true);
+ ExecCheckRTPerms(list_make1(rte), true);
+ }
/*
* Make sure the constructed table does not have RLS enabled.
@@ -550,9 +554,15 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
*/
myState->rel = intoRelationDesc;
myState->reladdr = intoRelationAddr;
- myState->output_cid = GetCurrentCommandId(true);
myState->ti_options = TABLE_INSERT_SKIP_FSM;
- myState->bistate = GetBulkInsertState();
+ myState->output_cid = GetCurrentCommandId(true);
+
+ /*
+ * In case if WITH NO DATA is specified, we do not allocate bulk insert
+ * state as we do not have tuples to insert.
+ */
+ if (!into->skipData)
+ myState->bistate = GetBulkInsertState();
/*
* Valid smgr_targblock implies something already wrote to the relation.
@@ -569,20 +579,23 @@ intorel_receive(TupleTableSlot *slot, DestReceiver *self)
{
DR_intorel *myState = (DR_intorel *) self;
- /*
- * Note that the input slot might not be of the type of the target
- * relation. That's supported by table_tuple_insert(), but slightly less
- * efficient than inserting with the right slot - but the alternative
- * would be to copy into a slot of the right type, which would not be
- * cheap either. This also doesn't allow accessing per-AM data (say a
- * tuple's xmin), but since we don't do that here...
- */
-
- table_tuple_insert(myState->rel,
- slot,
- myState->output_cid,
- myState->ti_options,
- myState->bistate);
+ /* Do not insert in case if WITH NO DATA is specified. */
+ if (!myState->into->skipData)
+ {
+ /*
+ * Note that the input slot might not be of the type of the target
+ * relation. That's supported by table_tuple_insert(), but slightly
+ * less efficient than inserting with the right slot - but the
+ * alternative would be to copy into a slot of the right type, which
+ * would not be cheap either. This also doesn't allow accessing per-AM
+ * data (say a tuple's xmin), but since we don't do that here...
+ */
+ table_tuple_insert(myState->rel,
+ slot,
+ myState->output_cid,
+ myState->ti_options,
+ myState->bistate);
+ }
/* We know this is a newly created relation, so there are no indexes */
@@ -597,9 +610,15 @@ intorel_shutdown(DestReceiver *self)
{
DR_intorel *myState = (DR_intorel *) self;
- FreeBulkInsertState(myState->bistate);
-
- table_finish_bulk_insert(myState->rel, myState->ti_options);
+ /*
+ * In case if WITH NO DATA is specified, we do not allocate bulk insert
+ * state as we do not have tuples to insert.
+ */
+ if (!myState->into->skipData)
+ {
+ FreeBulkInsertState(myState->bistate);
+ table_finish_bulk_insert(myState->rel, myState->ti_options);
+ }
/* close rel, but keep lock until commit */
table_close(myState->rel, NoLock);
diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
index d0121a7b0b..f0ecff8a10 100644
--- a/src/test/regress/expected/matview.out
+++ b/src/test/regress/expected/matview.out
@@ -589,3 +589,36 @@ SELECT * FROM mvtest2;
ERROR: materialized view "mvtest2" has not been populated
HINT: Use the REFRESH MATERIALIZED VIEW command.
ROLLBACK;
+-- Verfiy INSERT privilege, if owner is not allowed to insert.
+CREATE SCHEMA matview_schema;
+CREATE USER matview_user;
+ALTER DEFAULT PRIVILEGES FOR ROLE matview_user
+ REVOKE INSERT ON TABLES FROM matview_user;
+GRANT ALL ON SCHEMA matview_schema TO public;
+SET SESSION AUTHORIZATION matview_user;
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE MATERIALIZED VIEW matview_schema.mv_nodata1 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+ QUERY PLAN
+---------------------------------------
+ Seq Scan on pg_class (never executed)
+ Filter: (relname ~~ '%c%'::text)
+(2 rows)
+
+CREATE MATERIALIZED VIEW matview_schema.mv_nodata2 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE MATERIALIZED VIEW matview_schema.mv_nodata3 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH DATA; -- Error
+ERROR: permission denied for materialized view mv_nodata3
+CREATE MATERIALIZED VIEW matview_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
+RESET SESSION AUTHORIZATION;
+ALTER DEFAULT PRIVILEGES FOR ROLE matview_user
+ GRANT INSERT ON TABLES TO matview_user;
+DROP SCHEMA matview_schema CASCADE;
+NOTICE: drop cascades to 2 other objects
+DETAIL: drop cascades to materialized view matview_schema.mv_nodata1
+drop cascades to materialized view matview_schema.mv_nodata2
+DROP USER matview_user;
diff --git a/src/test/regress/expected/select_into.out b/src/test/regress/expected/select_into.out
index f373fae679..7d85386891 100644
--- a/src/test/regress/expected/select_into.out
+++ b/src/test/regress/expected/select_into.out
@@ -31,6 +31,37 @@ CREATE TABLE selinto_schema.tmp3 (a,b,c)
AS SELECT oid,relname,relacl FROM pg_class
WHERE relname like '%c%'; -- Error
ERROR: permission denied for table tmp3
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata1 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+ QUERY PLAN
+---------------------------------------
+ Seq Scan on pg_class (never executed)
+ Filter: (relname ~~ '%c%'::text)
+(2 rows)
+
+INSERT INTO selinto_schema.tbl_nodata1
+ SELECT oid FROM pg_class WHERE relname like '%c%'; -- Error
+ERROR: permission denied for table tbl_nodata1
+CREATE TABLE selinto_schema.tbl_nodata2 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+PREPARE nodata_sel AS
+ SELECT oid FROM pg_class WHERE relname like '%c%';
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata3 (a) AS
+ EXECUTE nodata_sel WITH NO DATA; -- OK
+ QUERY PLAN
+---------------------------------------
+ Seq Scan on pg_class (never executed)
+ Filter: (relname ~~ '%c%'::text)
+(2 rows)
+
+CREATE TABLE selinto_schema.tbl_nodata4 (a) AS
+ EXECUTE nodata_sel WITH NO DATA; -- OK
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata5 (a) AS
+ EXECUTE nodata_sel WITH DATA; -- Error
+ERROR: permission denied for table tbl_nodata5
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
GRANT INSERT ON TABLES TO regress_selinto_user;
@@ -45,8 +76,12 @@ CREATE TABLE selinto_schema.tmp3 (a,b,c)
WHERE relname like '%c%'; -- OK
RESET SESSION AUTHORIZATION;
DROP SCHEMA selinto_schema CASCADE;
-NOTICE: drop cascades to 3 other objects
-DETAIL: drop cascades to table selinto_schema.tmp1
+NOTICE: drop cascades to 7 other objects
+DETAIL: drop cascades to table selinto_schema.tbl_nodata1
+drop cascades to table selinto_schema.tbl_nodata2
+drop cascades to table selinto_schema.tbl_nodata3
+drop cascades to table selinto_schema.tbl_nodata4
+drop cascades to table selinto_schema.tmp1
drop cascades to table selinto_schema.tmp2
drop cascades to table selinto_schema.tmp3
DROP USER regress_selinto_user;
diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql
index d96175aa26..bed09967a1 100644
--- a/src/test/regress/sql/matview.sql
+++ b/src/test/regress/sql/matview.sql
@@ -236,3 +236,29 @@ SELECT mvtest_func();
SELECT * FROM mvtest1;
SELECT * FROM mvtest2;
ROLLBACK;
+
+-- Verfiy INSERT privilege, if owner is not allowed to insert.
+CREATE SCHEMA matview_schema;
+CREATE USER matview_user;
+ALTER DEFAULT PRIVILEGES FOR ROLE matview_user
+ REVOKE INSERT ON TABLES FROM matview_user;
+GRANT ALL ON SCHEMA matview_schema TO public;
+
+SET SESSION AUTHORIZATION matview_user;
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE MATERIALIZED VIEW matview_schema.mv_nodata1 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+CREATE MATERIALIZED VIEW matview_schema.mv_nodata2 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE MATERIALIZED VIEW matview_schema.mv_nodata3 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH DATA; -- Error
+CREATE MATERIALIZED VIEW matview_schema.mv_nodata4 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH DATA; -- Error
+RESET SESSION AUTHORIZATION;
+
+ALTER DEFAULT PRIVILEGES FOR ROLE matview_user
+ GRANT INSERT ON TABLES TO matview_user;
+
+DROP SCHEMA matview_schema CASCADE;
+DROP USER matview_user;
diff --git a/src/test/regress/sql/select_into.sql b/src/test/regress/sql/select_into.sql
index a708fef0ea..05a620b12d 100644
--- a/src/test/regress/sql/select_into.sql
+++ b/src/test/regress/sql/select_into.sql
@@ -34,6 +34,23 @@ 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
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata1 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+INSERT INTO selinto_schema.tbl_nodata1
+ SELECT oid FROM pg_class WHERE relname like '%c%'; -- Error
+CREATE TABLE selinto_schema.tbl_nodata2 (a) AS
+ SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA; -- OK
+PREPARE nodata_sel AS
+ SELECT oid FROM pg_class WHERE relname like '%c%';
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata3 (a) AS
+ EXECUTE nodata_sel WITH NO DATA; -- OK
+CREATE TABLE selinto_schema.tbl_nodata4 (a) AS
+ EXECUTE nodata_sel WITH NO DATA; -- OK
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
+ CREATE TABLE selinto_schema.tbl_nodata5 (a) AS
+ EXECUTE nodata_sel WITH DATA; -- Error
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
--
2.25.1
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
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.
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
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.
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
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
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.
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.
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
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
Attachments:
v1-0001-Do-not-check-INSERT-privilege-in-CTAS-and-MatView.patchapplication/x-patch; name=v1-0001-Do-not-check-INSERT-privilege-in-CTAS-and-MatView.patchDownload
From 1275f235611a4f8badb1edd6f999b557bb1ed291 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Fri, 20 Nov 2020 14:36:33 +0530
Subject: [PATCH v1] Do not check INSERT privilege in CTAS and MatView
To keep in sync CTAS with SQL standard, do not check for INSERT
privilege.
---
.../sgml/ref/create_materialized_view.sgml | 3 +-
doc/src/sgml/ref/create_table_as.sgml | 5 +-
src/backend/commands/createas.c | 26 +----
src/test/regress/expected/matview.out | 30 +++--
src/test/regress/expected/select_into.out | 109 +++++++++---------
src/test/regress/sql/matview.sql | 16 ++-
src/test/regress/sql/select_into.sql | 59 ++++------
7 files changed, 111 insertions(+), 137 deletions(-)
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml
index e4ea049eff..c8dee0f99e 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -52,8 +52,7 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable>
<para>
<command>CREATE MATERIALIZED VIEW</command> requires
<literal>CREATE</literal> privilege on the schema used for the materialized
- view. If using <command>WITH DATA</command>, the default,
- <literal>INSERT</literal> privilege is also required.
+ view. <literal>INSERT</literal> privilege is not required.
</para>
</refsect1>
diff --git a/doc/src/sgml/ref/create_table_as.sgml b/doc/src/sgml/ref/create_table_as.sgml
index 2cac4e3ec0..6fb4fa6f44 100644
--- a/doc/src/sgml/ref/create_table_as.sgml
+++ b/doc/src/sgml/ref/create_table_as.sgml
@@ -56,9 +56,8 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
<para>
<command>CREATE TABLE AS</command> requires <literal>CREATE</literal>
- privilege on the schema used for the table. If using
- <command>WITH DATA</command>, the default, <literal>INSERT</literal>
- privilege is also required.
+ privilege on the schema used for the table. However,
+ <literal>INSERT</literal> privilege is not required as per the SQL standard.
</para>
</refsect1>
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 37649eafa8..1f47ea99b5 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -432,7 +432,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
DR_intorel *myState = (DR_intorel *) self;
IntoClause *into = myState->into;
bool is_matview;
- char relkind;
List *attrList;
ObjectAddress intoRelationAddr;
Relation intoRelationDesc;
@@ -443,7 +442,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
/* This code supports both CREATE TABLE AS and CREATE MATERIALIZED VIEW */
is_matview = (into->viewQuery != NULL);
- relkind = is_matview ? RELKIND_MATVIEW : RELKIND_RELATION;
/*
* Build column definitions using "pre-cooked" type and collation info. If
@@ -505,29 +503,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
*/
intoRelationDesc = table_open(intoRelationAddr.objectId, AccessExclusiveLock);
- /*
- * Check INSERT permission on the constructed table. Skip this check if
- * WITH NO DATA is specified as only a table gets created with no tuples
- * inserted, that is a case possible when using EXPLAIN ANALYZE or
- * EXECUTE.
- */
- if (!into->skipData)
- {
- RangeTblEntry *rte;
-
- rte = makeNode(RangeTblEntry);
- rte->rtekind = RTE_RELATION;
- rte->relid = intoRelationAddr.objectId;
- rte->relkind = relkind;
- rte->rellockmode = RowExclusiveLock;
- rte->requiredPerms = ACL_INSERT;
-
- for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)
- rte->insertedCols = bms_add_member(rte->insertedCols,
- attnum - FirstLowInvalidHeapAttributeNumber);
-
- ExecCheckRTPerms(list_make1(rte), true);
- }
+ /* We do not check INSERT privilege here for SQL standard compatibility. */
/*
* Make sure the constructed table does not have RLS enabled.
diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
index 328c3118b6..da4c73aa4f 100644
--- a/src/test/regress/expected/matview.out
+++ b/src/test/regress/expected/matview.out
@@ -589,22 +589,29 @@ SELECT * FROM mvtest2;
ERROR: materialized view "mvtest2" has not been populated
HINT: Use the REFRESH MATERIALIZED VIEW command.
ROLLBACK;
--- INSERT privileges if relation owner is not allowed to insert.
+--
+-- INSERT privilege for CREATE and REFRESH MATERIALIZED VIEW.
+-- Queries succeed even though the owner has no INSERT privilege as we do not
+-- check for it.
+--
CREATE SCHEMA matview_schema;
CREATE USER regress_matview_user;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_matview_user
REVOKE INSERT ON TABLES FROM regress_matview_user;
GRANT ALL ON SCHEMA matview_schema TO public;
SET SESSION AUTHORIZATION regress_matview_user;
--- WITH DATA fails.
CREATE MATERIALIZED VIEW matview_schema.mv_withdata1 (a) AS
- SELECT generate_series(1, 10) WITH DATA; -- error
-ERROR: permission denied for materialized view mv_withdata1
+ SELECT generate_series(1, 10) WITH DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE MATERIALIZED VIEW matview_schema.mv_withdata1 (a) AS
- SELECT generate_series(1, 10) WITH DATA; -- error
-ERROR: permission denied for materialized view mv_withdata1
--- WITH NO DATA passes.
+ CREATE MATERIALIZED VIEW matview_schema.mv_withdata2 (a) AS
+ SELECT generate_series(1, 10) WITH DATA;
+ QUERY PLAN
+--------------------------------------
+ ProjectSet (actual rows=10 loops=1)
+ -> Result (actual rows=1 loops=1)
+(2 rows)
+
+REFRESH MATERIALIZED VIEW matview_schema.mv_withdata2;
CREATE MATERIALIZED VIEW matview_schema.mv_nodata1 (a) AS
SELECT generate_series(1, 10) WITH NO DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
@@ -616,11 +623,14 @@ EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
-> Result (never executed)
(2 rows)
+REFRESH MATERIALIZED VIEW matview_schema.mv_nodata2;
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_matview_user
GRANT INSERT ON TABLES TO regress_matview_user;
DROP SCHEMA matview_schema CASCADE;
-NOTICE: drop cascades to 2 other objects
-DETAIL: drop cascades to materialized view matview_schema.mv_nodata1
+NOTICE: drop cascades to 4 other objects
+DETAIL: drop cascades to materialized view matview_schema.mv_withdata1
+drop cascades to materialized view matview_schema.mv_withdata2
+drop cascades to materialized view matview_schema.mv_nodata1
drop cascades to materialized view matview_schema.mv_nodata2
DROP USER regress_matview_user;
diff --git a/src/test/regress/expected/select_into.out b/src/test/regress/expected/select_into.out
index 45068afca7..1ee2313fb0 100644
--- a/src/test/regress/expected/select_into.out
+++ b/src/test/regress/expected/select_into.out
@@ -12,7 +12,9 @@ SELECT *
WHERE onek2.unique1 < 2;
DROP TABLE sitmp1;
--
--- SELECT INTO and INSERT permission, if owner is not allowed to insert.
+-- INSERT privilege for SELECT INTO and CREATE TABLE AS.
+-- Queries succeed even though the owner has no INSERT privilege as we do not
+-- check for it.
--
CREATE SCHEMA selinto_schema;
CREATE USER regress_selinto_user;
@@ -20,79 +22,76 @@ ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
REVOKE INSERT ON TABLES FROM regress_selinto_user;
GRANT ALL ON SCHEMA selinto_schema TO public;
SET SESSION AUTHORIZATION regress_selinto_user;
-SELECT * INTO TABLE selinto_schema.tmp1
- FROM pg_class WHERE relname like '%a%';
+CREATE TABLE selinto_schema.tmp1 (col1 int);
+-- Direct insert fails, as owner has no INSERT privilege.
+INSERT INTO selinto_schema.tmp1 VALUES (1);
ERROR: permission denied for table tmp1
-SELECT oid AS clsoid, relname, relnatts + 10 AS x
- INTO selinto_schema.tmp2
- FROM pg_class WHERE relname like '%b%';
-ERROR: permission denied for table tmp2
--- WITH DATA, fails
-CREATE TABLE selinto_schema.tbl_withdata (a,b,c)
- AS SELECT oid,relname,relacl FROM pg_class
- WHERE relname like '%c%' WITH DATA;
-ERROR: permission denied for table tbl_withdata
+SELECT * INTO TABLE selinto_schema.tmp2 FROM generate_series(1,3);
+-- WITH DATA, passes.
+CREATE TABLE selinto_schema.tmp3 (a) AS SELECT generate_series(1,3) WITH DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE TABLE selinto_schema.tbl_withdata (a,b,c)
- AS SELECT oid,relname,relacl FROM pg_class
- WHERE relname like '%c%' WITH DATA;
-ERROR: permission denied for table tbl_withdata
+ CREATE TABLE selinto_schema.tmp4 (a) AS SELECT generate_series(1,3) WITH DATA;
+ QUERY PLAN
+--------------------------------------
+ ProjectSet (actual rows=3 loops=1)
+ -> Result (actual rows=1 loops=1)
+(2 rows)
+
-- WITH NO DATA, passes.
-CREATE TABLE selinto_schema.tbl_nodata1 (a) AS
- SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA;
+CREATE TABLE selinto_schema.tmp5 (a) AS
+ SELECT generate_series(1,3) WITH NO DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE TABLE selinto_schema.tbl_nodata2 (a) AS
- SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA;
- QUERY PLAN
----------------------------------------
- Seq Scan on pg_class (never executed)
- Filter: (relname ~~ '%c%'::text)
+ CREATE TABLE selinto_schema.tmp6 (a) AS
+ SELECT generate_series(1,3) WITH NO DATA;
+ QUERY PLAN
+-------------------------------
+ ProjectSet (never executed)
+ -> Result (never executed)
(2 rows)
--- EXECUTE and WITH DATA, fails.
-PREPARE data_sel AS
- SELECT oid FROM pg_class WHERE relname like '%c%';
-CREATE TABLE selinto_schema.tbl_withdata (a) AS
- EXECUTE data_sel WITH DATA;
-ERROR: permission denied for table tbl_withdata
+-- EXECUTE and WITH DATA, passes.
+PREPARE data_sel AS SELECT generate_series(1,3);
+CREATE TABLE selinto_schema.tmp7 (a) AS EXECUTE data_sel WITH DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE TABLE selinto_schema.tbl_withdata (a) AS
- EXECUTE data_sel WITH DATA;
-ERROR: permission denied for table tbl_withdata
+ CREATE TABLE selinto_schema.tmp8 (a) AS EXECUTE data_sel WITH DATA;
+ QUERY PLAN
+--------------------------------------
+ ProjectSet (actual rows=3 loops=1)
+ -> Result (actual rows=1 loops=1)
+(2 rows)
+
-- EXECUTE and WITH NO DATA, passes.
-CREATE TABLE selinto_schema.tbl_nodata3 (a) AS
- EXECUTE data_sel WITH NO DATA;
+CREATE TABLE selinto_schema.tmp9 (a) AS EXECUTE data_sel WITH NO DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE TABLE selinto_schema.tbl_nodata4 (a) AS
- EXECUTE data_sel WITH NO DATA;
- QUERY PLAN
----------------------------------------
- Seq Scan on pg_class (never executed)
- Filter: (relname ~~ '%c%'::text)
+ CREATE TABLE selinto_schema.tmp10 (a) AS EXECUTE data_sel WITH NO DATA;
+ QUERY PLAN
+-------------------------------
+ ProjectSet (never executed)
+ -> Result (never executed)
(2 rows)
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
GRANT INSERT ON TABLES TO regress_selinto_user;
SET SESSION AUTHORIZATION regress_selinto_user;
-SELECT * INTO TABLE selinto_schema.tmp1
- FROM pg_class WHERE relname like '%a%'; -- OK
-SELECT oid AS clsoid, relname, relnatts + 10 AS x
- INTO selinto_schema.tmp2
- FROM pg_class WHERE relname like '%b%'; -- OK
-CREATE TABLE selinto_schema.tmp3 (a,b,c)
- AS SELECT oid,relname,relacl FROM pg_class
- WHERE relname like '%c%'; -- OK
+SELECT * INTO TABLE selinto_schema.tmp11 FROM generate_series(1,3); -- OK
+CREATE TABLE selinto_schema.tmp12 (a) AS SELECT generate_series(1,3); -- OK
RESET SESSION AUTHORIZATION;
+DEALLOCATE data_sel;
DROP SCHEMA selinto_schema CASCADE;
-NOTICE: drop cascades to 7 other objects
-DETAIL: drop cascades to table selinto_schema.tbl_nodata1
-drop cascades to table selinto_schema.tbl_nodata2
-drop cascades to table selinto_schema.tbl_nodata3
-drop cascades to table selinto_schema.tbl_nodata4
-drop cascades to table selinto_schema.tmp1
+NOTICE: drop cascades to 12 other objects
+DETAIL: drop cascades to table selinto_schema.tmp1
drop cascades to table selinto_schema.tmp2
drop cascades to table selinto_schema.tmp3
+drop cascades to table selinto_schema.tmp4
+drop cascades to table selinto_schema.tmp5
+drop cascades to table selinto_schema.tmp6
+drop cascades to table selinto_schema.tmp7
+drop cascades to table selinto_schema.tmp8
+drop cascades to table selinto_schema.tmp9
+drop cascades to table selinto_schema.tmp10
+drop cascades to table selinto_schema.tmp11
+drop cascades to table selinto_schema.tmp12
DROP USER regress_selinto_user;
-- Tests for WITH NO DATA and column name consistency
CREATE TABLE ctas_base (i int, j int);
diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql
index 419eba2075..51a94eda2c 100644
--- a/src/test/regress/sql/matview.sql
+++ b/src/test/regress/sql/matview.sql
@@ -237,7 +237,11 @@ SELECT * FROM mvtest1;
SELECT * FROM mvtest2;
ROLLBACK;
--- INSERT privileges if relation owner is not allowed to insert.
+--
+-- INSERT privilege for CREATE and REFRESH MATERIALIZED VIEW.
+-- Queries succeed even though the owner has no INSERT privilege as we do not
+-- check for it.
+--
CREATE SCHEMA matview_schema;
CREATE USER regress_matview_user;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_matview_user
@@ -245,18 +249,18 @@ ALTER DEFAULT PRIVILEGES FOR ROLE regress_matview_user
GRANT ALL ON SCHEMA matview_schema TO public;
SET SESSION AUTHORIZATION regress_matview_user;
--- WITH DATA fails.
CREATE MATERIALIZED VIEW matview_schema.mv_withdata1 (a) AS
- SELECT generate_series(1, 10) WITH DATA; -- error
+ SELECT generate_series(1, 10) WITH DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE MATERIALIZED VIEW matview_schema.mv_withdata1 (a) AS
- SELECT generate_series(1, 10) WITH DATA; -- error
--- WITH NO DATA passes.
+ CREATE MATERIALIZED VIEW matview_schema.mv_withdata2 (a) AS
+ SELECT generate_series(1, 10) WITH DATA;
+REFRESH MATERIALIZED VIEW matview_schema.mv_withdata2;
CREATE MATERIALIZED VIEW matview_schema.mv_nodata1 (a) AS
SELECT generate_series(1, 10) WITH NO DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
CREATE MATERIALIZED VIEW matview_schema.mv_nodata2 (a) AS
SELECT generate_series(1, 10) WITH NO DATA;
+REFRESH MATERIALIZED VIEW matview_schema.mv_nodata2;
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_matview_user
diff --git a/src/test/regress/sql/select_into.sql b/src/test/regress/sql/select_into.sql
index 0faba72bec..2daada106c 100644
--- a/src/test/regress/sql/select_into.sql
+++ b/src/test/regress/sql/select_into.sql
@@ -17,7 +17,9 @@ SELECT *
DROP TABLE sitmp1;
--
--- SELECT INTO and INSERT permission, if owner is not allowed to insert.
+-- INSERT privilege for SELECT INTO and CREATE TABLE AS.
+-- Queries succeed even though the owner has no INSERT privilege as we do not
+-- check for it.
--
CREATE SCHEMA selinto_schema;
CREATE USER regress_selinto_user;
@@ -26,55 +28,40 @@ ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
GRANT ALL ON SCHEMA selinto_schema TO public;
SET SESSION AUTHORIZATION regress_selinto_user;
-SELECT * INTO TABLE selinto_schema.tmp1
- FROM pg_class WHERE relname like '%a%';
-SELECT oid AS clsoid, relname, relnatts + 10 AS x
- INTO selinto_schema.tmp2
- FROM pg_class WHERE relname like '%b%';
--- WITH DATA, fails
-CREATE TABLE selinto_schema.tbl_withdata (a,b,c)
- AS SELECT oid,relname,relacl FROM pg_class
- WHERE relname like '%c%' WITH DATA;
+CREATE TABLE selinto_schema.tmp1 (col1 int);
+-- Direct insert fails, as owner has no INSERT privilege.
+INSERT INTO selinto_schema.tmp1 VALUES (1);
+SELECT * INTO TABLE selinto_schema.tmp2 FROM generate_series(1,3);
+-- WITH DATA, passes.
+CREATE TABLE selinto_schema.tmp3 (a) AS SELECT generate_series(1,3) WITH DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE TABLE selinto_schema.tbl_withdata (a,b,c)
- AS SELECT oid,relname,relacl FROM pg_class
- WHERE relname like '%c%' WITH DATA;
+ CREATE TABLE selinto_schema.tmp4 (a) AS SELECT generate_series(1,3) WITH DATA;
-- WITH NO DATA, passes.
-CREATE TABLE selinto_schema.tbl_nodata1 (a) AS
- SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA;
+CREATE TABLE selinto_schema.tmp5 (a) AS
+ SELECT generate_series(1,3) WITH NO DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE TABLE selinto_schema.tbl_nodata2 (a) AS
- SELECT oid FROM pg_class WHERE relname like '%c%' WITH NO DATA;
--- EXECUTE and WITH DATA, fails.
-PREPARE data_sel AS
- SELECT oid FROM pg_class WHERE relname like '%c%';
-CREATE TABLE selinto_schema.tbl_withdata (a) AS
- EXECUTE data_sel WITH DATA;
+ CREATE TABLE selinto_schema.tmp6 (a) AS
+ SELECT generate_series(1,3) WITH NO DATA;
+-- EXECUTE and WITH DATA, passes.
+PREPARE data_sel AS SELECT generate_series(1,3);
+CREATE TABLE selinto_schema.tmp7 (a) AS EXECUTE data_sel WITH DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE TABLE selinto_schema.tbl_withdata (a) AS
- EXECUTE data_sel WITH DATA;
+ CREATE TABLE selinto_schema.tmp8 (a) AS EXECUTE data_sel WITH DATA;
-- EXECUTE and WITH NO DATA, passes.
-CREATE TABLE selinto_schema.tbl_nodata3 (a) AS
- EXECUTE data_sel WITH NO DATA;
+CREATE TABLE selinto_schema.tmp9 (a) AS EXECUTE data_sel WITH NO DATA;
EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF)
- CREATE TABLE selinto_schema.tbl_nodata4 (a) AS
- EXECUTE data_sel WITH NO DATA;
+ CREATE TABLE selinto_schema.tmp10 (a) AS EXECUTE data_sel WITH NO DATA;
RESET SESSION AUTHORIZATION;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
GRANT INSERT ON TABLES TO regress_selinto_user;
SET SESSION AUTHORIZATION regress_selinto_user;
-SELECT * INTO TABLE selinto_schema.tmp1
- FROM pg_class WHERE relname like '%a%'; -- OK
-SELECT oid AS clsoid, relname, relnatts + 10 AS x
- INTO selinto_schema.tmp2
- FROM pg_class WHERE relname like '%b%'; -- OK
-CREATE TABLE selinto_schema.tmp3 (a,b,c)
- AS SELECT oid,relname,relacl FROM pg_class
- WHERE relname like '%c%'; -- OK
+SELECT * INTO TABLE selinto_schema.tmp11 FROM generate_series(1,3); -- OK
+CREATE TABLE selinto_schema.tmp12 (a) AS SELECT generate_series(1,3); -- OK
RESET SESSION AUTHORIZATION;
+DEALLOCATE data_sel;
DROP SCHEMA selinto_schema CASCADE;
DROP USER regress_selinto_user;
--
2.25.1
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