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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to