Postgres Pro
Downloads
Home   >   mailing lists

Re: [HACKERS] Adding support for Default partition in partitioning - Mailing list pgsql-hackers

From Jeevan Ladhe
Subject Re: [HACKERS] Adding support for Default partition in partitioning
Date
Msg-id CAOgcT0POGndkF2hbTPExb4=wihWrpz_FOB7XD=C5ifucp_x6eA@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] Adding support for Default partition in partitioning  (Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>)
Responses Re: [HACKERS] Adding support for Default partition in partitioning
List pgsql-hackers
Tree view
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.bapat@enterprisedb.com> wrote:
On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasyed90@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

pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: [HACKERS] OK, so culicidae is *still* broken
Next
From: Jeevan Ladhe
Date:
Subject: Re: [HACKERS] Adding support for Default partition in partitioning