Hi Rahila,
I tried to go through your v7 patch, and following are my comments:
1.
With -Werrors I see following compilation failure:
parse_utilcmd.c: In function ‘transformPartitionBound’:
parse_utilcmd.c:3309:4: error: implicit declaration of function
‘isDefaultPartitionBound’ [-Werror=implicit-function-declaration]
if (!(isDefaultPartitionBound(value)))
^
cc1: all warnings being treated as errors
You need to include, "catalog/partitions.h".
2.
Once I made above change pass, I see following error:
tablecmds.c: In function ‘DefineRelation’:
tablecmds.c:762:17: error: unused variable ‘partdesc’
[-Werror=unused-variable]
PartitionDesc partdesc;
^
cc1: all warnings being treated as errors
3.
Please remove the extra line at the end of the function
check_new_partition_bound:
+ MemoryContextSwitchTo(oldCxt);
+ heap_endscan(scan);
+ UnregisterSnapshot(snapshot);
+ heap_close(defrel, AccessExclusiveLock);
+ ExecDropSingleTupleTableSlot(tupslot);
+ }
+
}
4.
In generate_qual_for_defaultpart() you do not need 2 pointers for looping
over
bound specs:
+ ListCell *cell1;
+ ListCell *cell3;
You can iterate twice using one pointer itself.
Same is for:
+ ListCell *cell2;
+ ListCell *cell4;
Similarly, in get_qual_from_partbound(), you can use one pointer below,
instead of cell1 and cell3:
+ PartitionBoundSpec *bspec;
+ ListCell *cell1;
+ ListCell *cell3;
5.
Should this have a break in if block?
+ foreach(cell1, bspec->listdatums)
+ {
+ Node *value = lfirst(cell1);
+ if (isDefaultPartitionBound(value))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }
+ }
6.
I am wondering, isn't it possible to retrieve the has_default and
default_index
here to find out if default partition exists and if exist then find it's oid
using rd_partdesc, that would avoid above(7) loop to check if partition
bound is
default.
7.
The output of describe needs to be improved.
Consider following case:
postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
(4,5,4,4,4,6,2);
ERROR: relation "list_partitioned" does not exist
postgres=# CREATE TABLE list_partitioned (
a int
) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
(4,5,4,4,4,6,2);
CREATE TABLE
postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT, 3, DEFAULT, 3, DEFAULT);
CREATE TABLE
postgres=# \d+ part_1;
Table "public.part_1"
Column | Type | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain |
|
Partition of: list_partitioned FOR VALUES IN (4, 5, 6, 2)
postgres=# \d+ part_default;
Table "public.part_default"
Column | Type | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain |
|
Partition of: list_partitioned FOR VALUES IN (DEFAULT3DEFAULTDEFAULT)
As you can see in above example, part_1 has multiple entries for 4 while
creating the partition, but describe shows only one entry for 4 in values
set.
Similarly, part_default has multiple entries for 3 and DEFAULT while
creating
the partition, but the describe shows a weired output. Instead, we should
have
just one entry saying "VALUES IN (DEFAULT, 3)":
postgres=# \d+ part_default;
Table "public.part_default"
Column | Type | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain |
|
Partition of: list_partitioned FOR VALUES IN (DEFAULT, 3)
8.
Following call to find_inheritance_children() in
generate_qual_for_defaultpart()
is an overhead, instead we can simply use an array of oids in rd_partdesc.
+ spec = (PartitionBoundSpec *) bound;
+
+ inhoids = find_inheritance_children(RelationGetRelid(parent), NoLock);
+
+ foreach(cell2, inhoids)
Same is for the call in get_qual_from_partbound:
+ /* Collect bound spec nodes in a list. This is done if the partition is
+ * a default partition. In case of default partition, constraint is formed
+ * by performing <> operation over the partition constraints of the
+ * existing partitions.
+ */
+ inhoids = find_inheritance_children(RelationGetRelid(parent), NoLock);
+ foreach(cell2, inhoids)
9.
How about rephrasing following error message:
postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
(14);
ERROR: new default partition constraint is violated by some row
To,
"ERROR: some existing row in default partition violates new default
partition constraint"
10.
Additionally, I did test your given sample test in first post and the one
mentioned by Keith; both of them are passing without errors.
Also, I did a pg_dump test and it is dumping the partitions and data
correctly.
But as mentioned earlier, it would be good if you have them in your patch.
I will do further review and let you know comments if any.
Regards,
Jeevan Ladhe
On Mon, Apr 24, 2017 at 5:44 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:
> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmh...@gmail.com>
> wrote:
> > On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasye...@gmail.com>
> wrote:
> >> Following can also be considered as it specifies more clearly that the
> >> partition holds default values.
> >>
> >> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
> >
> > Yes, that could be done. But I don't think it's correct to say that
> > the partition holds default values. Let's back up and ask what the
> > word "default" means. The relevant definition (according to Google or
> > whoever they stole it from) is:
> >
> > a preselected option adopted by a computer program or other mechanism
> > when no alternative is specified by the user or programmer.
> >
> > So, a default *value* is the value that is used when no alternative is
> > specified by the user or programmer. We have that concept, but it's
> > not what we're talking about here: that's configured by applying the
> > DEFAULT property to a column. Here, we're talking about the default
> > *partition*, or in other words the *partition* that is used when no
> > alternative is specified by the user or programmer. So, that's why I
> > proposed the syntax I did. The partition doesn't contain default
> > values; it is itself a default.
>
> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more
> natural.
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
