Обсуждение: 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
Вложения
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
Вложения
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; -- 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
Вложения
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
Вложения
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
Вложения
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
Вложения
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
Вложения
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