On 2017/02/13 14:21, Amit Langote wrote:
> On 2017/02/10 19:19, Simon Riggs wrote:
>> * The OID inheritance needs work - you shouldn't need to specify a
>> partition needs OIDS if the parent has it already.
>
> That sounds right. It's better to keep the behavior same as for regular
> inheritance. Will post a patch to get rid of the unnecessary check.
>
> FWIW, the check was added to prevent the command from succeeding in the
> case where WITHOUT OIDS has been specified for a partition and the parent
> partitioned table has the OID column. Regular inheritance simply
> *overrides* the WITHOUT OIDS specification, which might be seen as surprising.
0001 of the attached patches takes care of this.
>> * There's no tab completion, which prevents people from testing this
>> (maybe better now with some docs)
>
> Will post a patch as well.
...and 0002 for this.
>> * ERROR: no partition of relation "measurement_year_month" found for row
>> DETAIL: Failing row contains (2016-12-02, 1, 1).
>> should not return the whole row, just the partition keys
>
> I think that makes sense. Something along the lines of
> BuildIndexValueDescription() for partition keys will be necessary. Will
> post a patch.
Let me spend a bit more time on this one.
Thanks,
Amit
>From 42682394d3d40aeaa5b2565a4f0ca23828a0dda0 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 13 Feb 2017 13:32:18 +0900
Subject: [PATCH 1/3] Inherit OID system column automatically for partitions
Currently, WITH OIDS must be explicitly specified when creating a
partition if the parent table has the OID system column. Instead,
inherit it automatically, possibly overriding any explicit WITHOUT
OIDS specification. Per review comment from Simon Riggs
---
src/backend/commands/tablecmds.c | 21 ++++++++-------------
src/test/regress/expected/create_table.out | 18 +++++++++++++-----
src/test/regress/sql/create_table.sql | 10 ++++++----
3 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 37a4c4a3d6..96650e69df 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -634,19 +634,14 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
relkind == RELKIND_PARTITIONED_TABLE));
descriptor->tdhasoid = (localHasOids || parentOidCount > 0);
- if (stmt->partbound)
- {
- /* If the parent has OIDs, partitions must have them too. */
- if (parentOidCount > 0 && !localHasOids)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot create table without OIDs as partition of table with OIDs")));
- /* If the parent doesn't, partitions must not have them. */
- if (parentOidCount == 0 && localHasOids)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot create table with OIDs as partition of table without OIDs")));
- }
+ /*
+ * If a partitioned table doesn't have the system OID column, then none
+ * of its partitions should have it.
+ */
+ if (stmt->partbound && parentOidCount == 0 && localHasOids)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot create table with OIDs as partition of table without OIDs")));
/*
* Find columns with default values and prepare for insertion of the
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index fc92cd92dd..20eb3d35f9 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -524,16 +524,24 @@ DROP TABLE temp_parted;
CREATE TABLE no_oids_parted (
a int
) PARTITION BY RANGE (a) WITHOUT OIDS;
-CREATE TABLE fail_part PARTITION OF no_oids_parted FOR VALUES FROM (1) TO (10 )WITH OIDS;
+CREATE TABLE fail_part PARTITION OF no_oids_parted FOR VALUES FROM (1) TO (10) WITH OIDS;
ERROR: cannot create table with OIDs as partition of table without OIDs
DROP TABLE no_oids_parted;
--- likewise, the reverse if also true
+-- If the partitioned table has oids, then the partition must have them.
+-- If the WITHOUT OIDS option is specified for partition, it is overridden.
CREATE TABLE oids_parted (
a int
) PARTITION BY RANGE (a) WITH OIDS;
-CREATE TABLE fail_part PARTITION OF oids_parted FOR VALUES FROM (1) TO (10 ) WITHOUT OIDS;
-ERROR: cannot create table without OIDs as partition of table with OIDs
-DROP TABLE oids_parted;
+CREATE TABLE part_forced_oids PARTITION OF oids_parted FOR VALUES FROM (1) TO (10) WITHOUT OIDS;
+\d+ part_forced_oids
+ Table "public.part_forced_oids"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ a | integer | | not null | | plain | |
+Partition of: oids_parted FOR VALUES FROM (1) TO (10)
+Has OIDs: yes
+
+DROP TABLE oids_parted, part_forced_oids;
-- check for partition bound overlap and other invalid specifications
CREATE TABLE list_parted2 (
a varchar
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 5f25c436ee..f41dd71475 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -493,15 +493,17 @@ DROP TABLE temp_parted;
CREATE TABLE no_oids_parted (
a int
) PARTITION BY RANGE (a) WITHOUT OIDS;
-CREATE TABLE fail_part PARTITION OF no_oids_parted FOR VALUES FROM (1) TO (10 )WITH OIDS;
+CREATE TABLE fail_part PARTITION OF no_oids_parted FOR VALUES FROM (1) TO (10) WITH OIDS;
DROP TABLE no_oids_parted;
--- likewise, the reverse if also true
+-- If the partitioned table has oids, then the partition must have them.
+-- If the WITHOUT OIDS option is specified for partition, it is overridden.
CREATE TABLE oids_parted (
a int
) PARTITION BY RANGE (a) WITH OIDS;
-CREATE TABLE fail_part PARTITION OF oids_parted FOR VALUES FROM (1) TO (10 ) WITHOUT OIDS;
-DROP TABLE oids_parted;
+CREATE TABLE part_forced_oids PARTITION OF oids_parted FOR VALUES FROM (1) TO (10) WITHOUT OIDS;
+\d+ part_forced_oids
+DROP TABLE oids_parted, part_forced_oids;
-- check for partition bound overlap and other invalid specifications
--
2.11.0
>From c09837eff10725bc33265cf6d91e8e8e81dd2c55 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 13 Feb 2017 18:18:48 +0900
Subject: [PATCH 2/3] Tab completion for the new partitioning syntax
---
src/bin/psql/tab-complete.c | 59 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 58 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6e759d0b76..73958cdebf 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -463,6 +463,21 @@ static const SchemaQuery Query_for_list_of_tables = {
NULL
};
+static const SchemaQuery Query_for_list_of_partitioned_tables = {
+ /* catname */
+ "pg_catalog.pg_class c",
+ /* selcondition */
+ "c.relkind IN ('P')",
+ /* viscondition */
+ "pg_catalog.pg_table_is_visible(c.oid)",
+ /* namespace */
+ "c.relnamespace",
+ /* result */
+ "pg_catalog.quote_ident(c.relname)",
+ /* qualresult */
+ NULL
+};
+
static const SchemaQuery Query_for_list_of_constraints_with_schema = {
/* catname */
"pg_catalog.pg_constraint c",
@@ -913,6 +928,16 @@ static const SchemaQuery Query_for_list_of_matviews = {
" SELECT 'DEFAULT' ) ss "\
" WHERE pg_catalog.substring(name,1,%%d)='%%s'"
+/* the silly-looking length condition is just to eat up the current word */
+#define Query_for_partition_of_table \
+"SELECT pg_catalog.quote_ident(c2.relname) "\
+" FROM pg_catalog.pg_class c1, pg_catalog.pg_class c2, pg_catalog.pg_inherits i"\
+" WHERE c1.oid=i.inhparent and i.inhrelid=c2.oid"\
+" and (%d = pg_catalog.length('%s'))"\
+" and pg_catalog.quote_ident(c1.relname)='%s'"\
+" and pg_catalog.pg_table_is_visible(c2.oid)"\
+" and c2.relispartition = 'true'"
+
/*
* This is a list of all "things" in Pgsql, which can show up after CREATE or
* DROP; and there is also a query to get a list of them.
@@ -1741,7 +1766,8 @@ psql_completion(const char *text, int start, int end)
static const char *const list_ALTER2[] =
{"ADD", "ALTER", "CLUSTER ON", "DISABLE", "DROP", "ENABLE", "INHERIT",
"NO INHERIT", "RENAME", "RESET", "OWNER TO", "SET",
- "VALIDATE CONSTRAINT", "REPLICA IDENTITY", NULL};
+ "VALIDATE CONSTRAINT", "REPLICA IDENTITY", "ATTACH PARTITION",
+ "DETACH PARTITION", NULL};
COMPLETE_WITH_LIST(list_ALTER2);
}
@@ -1922,6 +1948,26 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_LIST4("FULL", "NOTHING", "DEFAULT", "USING");
else if (Matches4("ALTER", "TABLE", MatchAny, "REPLICA"))
COMPLETE_WITH_CONST("IDENTITY");
+ /*
+ * If we have ALTER TABLE <foo> ATTACH PARTITION, provide a list of
+ * tables.
+ */
+ else if (Matches5("ALTER", "TABLE", MatchAny, "ATTACH", "PARTITION"))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, "");
+ /* Limited completion support for partition bound specification */
+ else if (TailMatches3("ATTACH", "PARTITION", MatchAny))
+ COMPLETE_WITH_CONST("FOR VALUES");
+ else if (TailMatches5("ATTACH", "PARTITION", MatchAny, "FOR", "VALUES"))
+ COMPLETE_WITH_LIST2("FROM (", "IN (");
+ /*
+ * If we have ALTER TABLE <foo> DETACH PARTITION, provide a list of
+ * partitions of <foo>.
+ */
+ else if (Matches5("ALTER", "TABLE", MatchAny, "DETACH", "PARTITION"))
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_partition_of_table);
+ }
/* ALTER TABLESPACE <foo> with RENAME TO, OWNER TO, SET, RESET */
else if (Matches3("ALTER", "TABLESPACE", MatchAny))
@@ -2299,6 +2345,17 @@ psql_completion(const char *text, int start, int end)
/* Complete "CREATE UNLOGGED" with TABLE or MATVIEW */
else if (TailMatches2("CREATE", "UNLOGGED"))
COMPLETE_WITH_LIST2("TABLE", "MATERIALIZED VIEW");
+ /* Complete PARTITION BY with RANGE ( or LIST ( or ... */
+ else if (TailMatches2("PARTITION", "BY"))
+ COMPLETE_WITH_LIST2("RANGE (", "LIST (");
+ /* If we have xxx PARTITION OF, provide a list of partitioned tables */
+ else if (TailMatches2("PARTITION", "OF"))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_partitioned_tables, "");
+ /* Limited completion support for partition bound specification */
+ else if (TailMatches3("PARTITION", "OF", MatchAny))
+ COMPLETE_WITH_CONST("FOR VALUES");
+ else if (TailMatches5("PARTITION", "OF", MatchAny, "FOR", "VALUES"))
+ COMPLETE_WITH_LIST2("FROM (", "IN (");
/* CREATE TABLESPACE */
else if (Matches3("CREATE", "TABLESPACE", MatchAny))
--
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
