Re: [HACKERS] Adding support for Default partition in partitioning

On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> I have fixed the crash in attached patch.
> Also the patch needed bit of adjustments due to recent commit.
> I have re-based the patch on latest commit.

+    bool        has_default;        /* Is there a default partition?
Currently false
+                                 * for a range partitioned table */
+    int            default_index;        /* Index of the default list
partition. -1 for
+                                 * range partitioned tables */

Why do we need both has_default and default_index?  If default_index
== -1 means that there is no default, we don't also need a separate
bool to record the same thing, do we?

get_qual_for_default() still returns a list of things that are not
quals.  I think that this logic is all pretty poorly organized.  The
logic to create a partitioning constraint for a list partition should
be part of get_qual_for_list(), whether or not it is a default.  And
when we have range partitions, the logic to create a default range
partitioning constraint should be part of get_qual_for_range().  The
code the way it's organized today makes it look like there are three
kinds of partitions: list, range, and default.  But that's not right
at all.  There are two kinds: list and range.  And a list partition
might or might not be a default partition, and similarly for range.

+                    ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION),
+                                    errmsg("DEFAULT partition cannot be used"
+                                           " without negator of operator  %s",
+                                           get_opname(operoid))));

I don't think ERRCODE_CHECK_VIOLATION is the right error code here,
and we have a policy against splitting message strings like this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company