Re: Adding support for Default partition in partitioning

On Thu, May 11, 2017 at 10:07 AM, Rahila Syed <rahilasyed90(at)gmail(dot)com> wrote:
> Please find attached an updated patch with review comments and bugs reported
> till date implemented.

You haven't done anything about the repeated suggestion that this
should also cover range partitioning.

+ /*
+ * If the partition is the default partition switch
+ * back to PARTITION_STRATEGY_LIST
+ */
+ if (spec->strategy == PARTITION_DEFAULT)
+ result_spec->strategy = PARTITION_STRATEGY_LIST;
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("invalid bound specification for a list
partition"),
parser_errposition(pstate, exprLocation(bound))));

This is incredibly ugly. I don't know exactly what should be done
about it, but I think PARTITION_DEFAULT is a bad idea and has got to
go. Maybe add a separate isDefault flag to PartitionBoundSpec.

+ /*
+ * Skip if it's a partitioned table. Only RELKIND_RELATION
+ * relations (ie, leaf partitions) need to be scanned.
+ */
+ if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)

What about foreign table partitions?

Doesn't it strike you as a bit strange that get_qual_for_default()
doesn't return a qual? Functions should generally have names that
describe what they do.

+ bound_datums = list_copy(spec->listdatums);
+
+ boundspecs = get_qual_for_default(parent, defid);
+
+ foreach(cell, bound_datums)
+ {
+ Node *value = lfirst(cell);
+ boundspecs = lappend(boundspecs, value);
+ }

There's an existing function that you can use to concatenate two lists
instead of open-coding it.

Also, I think that before you ask anyone to spend too much more time
and energy reviewing this, you should really add the documentation and
regression tests which you mentioned as a TODO. And run the code
through pgindent.

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