On Thu, May 11, 2017 at 10:07 AM, Rahila Syed <rahilasyed90@gmail.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
