On 2017/04/12 2:41, Robert Haas wrote:
> On Tue, Apr 11, 2017 at 9:45 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> Amit Langote <langote_amit...@lab.ntt.co.jp> writes:
>>> 2. DefineQueryRewrite() may try to scan a partitioned table in the case of
>>> converting a table to view, where we must make sure that the table being
>>> converted is empty. It's checked by scanning the heap, which we should
>>> not do for a partitioned table. Nor should we try to drop the storage
>>> once ready to make the table into a REKIND_VIEW relation (because all
>>> other checks passed okaying the conversion).
>>
>> It looks like this patch intends to allow converting a partitioned table
>> to a view. I would lobby for refusing the command, instead. There is
>> no good reason to allow it, and it might well be a user error.
>
> Yeah, I agree.
Alright. So I made it into two patches instead: 0001 fixes the bug that
validateCheckConstraint() tries to scan partitioned tables and 0002 makes
trying to convert a partitioned table to a view a user error.
Thanks,
Amit
>From a71b2d8c38c1ca2ca3430bdc3f4f850d66e486d6 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 11 Apr 2017 16:04:50 +0900
Subject: [PATCH 1/2] Fix a few places still scanning partitioned tables
Commit c94e6942ce missed validateCheckConstraint.
---
src/backend/commands/tablecmds.c | 8 ++++++--
src/test/regress/expected/alter_table.out | 6 ++++++
src/test/regress/sql/alter_table.sql | 7 +++++++
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a6a9f54b13..a02904c85c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8120,8 +8120,12 @@ validateCheckConstraint(Relation rel, HeapTuple constrtup)
bool isnull;
Snapshot snapshot;
- /* VALIDATE CONSTRAINT is a no-op for foreign tables */
- if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
+ /*
+ * VALIDATE CONSTRAINT is a no-op for foreign tables and partitioned
+ * tables.
+ */
+ if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
+ rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
return;
constrForm = (Form_pg_constraint) GETSTRUCT(constrtup);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 2227f2d977..883a5c9864 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3372,3 +3372,9 @@ ERROR: partition constraint is violated by some row
-- cleanup
drop table p;
drop table p1;
+-- validate constraint on partitioned tables should only scan leaf partitions
+create table parted_validate_test (a int) partition by list (a);
+create table parted_validate_test_1 partition of parted_validate_test for values in (0, 1);
+alter table parted_validate_test add constraint parted_validate_test_chka check (a > 0) not valid;
+alter table parted_validate_test validate constraint parted_validate_test_chka;
+drop table parted_validate_test;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 8cd6786a90..eb1b4b536f 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2228,3 +2228,10 @@ alter table p attach partition p1 for values from (1, 2) to (1, 10);
-- cleanup
drop table p;
drop table p1;
+
+-- validate constraint on partitioned tables should only scan leaf partitions
+create table parted_validate_test (a int) partition by list (a);
+create table parted_validate_test_1 partition of parted_validate_test for values in (0, 1);
+alter table parted_validate_test add constraint parted_validate_test_chka check (a > 0) not valid;
+alter table parted_validate_test validate constraint parted_validate_test_chka;
+drop table parted_validate_test;
--
2.11.0
>From e788b5663db1e62ee4d6f6a7a9c111b049f25fbe Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 12 Apr 2017 11:12:11 +0900
Subject: [PATCH 2/2] Disallow converting partitioned tables to a view
---
src/backend/rewrite/rewriteDefine.c | 6 ++++++
src/test/regress/sql/rules.sql | 5 +++++
2 files changed, 11 insertions(+)
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index df32f2c3ae..eab3f6062d 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -422,6 +422,12 @@ DefineQueryRewrite(char *rulename,
HeapScanDesc scanDesc;
Snapshot snapshot;
+ if (event_relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("could not convert partitioned table \"%s\" to a view",
+ RelationGetRelationName(event_relation))));
+
snapshot = RegisterSnapshot(GetLatestSnapshot());
scanDesc = heap_beginscan(event_relation, snapshot, 0, NULL);
if (heap_getnext(scanDesc, ForwardScanDirection) != NULL)
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index d4a61813e4..4fff266216 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -898,6 +898,11 @@ select reltoastrelid, relkind, relfrozenxid
drop view fooview;
+-- trying to convert a partitioned table to view is not allowed
+create table fooview (x int, y text) partition by list (x);
+create rule "_RETURN" as on select to fooview do instead
+ select 1 as x, 'aaa'::text as y;
+
--
-- check for planner problems with complex inherited UPDATES
--
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
