Re: [HACKERS] multi-column range partition constraint

On Wed, May 10, 2017 at 10:21 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Next update on this issue by Thursday 5/11.
>
> Attached updated patches.

Thanks.  0001, at least, really needs a pgindent run.  Also, my
compiler has this apparently-justifiable complaint:

partition.c:1767:5: error: variable 'cur_op_intp' is used uninitialized whenever     'for' loop exits because its
conditionis false     [-Werror,-Wsometimes-uninitialized]                               foreach(opic, op_infos)
                     ^~~~~~~~~~~~~~~~~~~~~~~
 
../../../src/include/nodes/pg_list.h:162:30: note: expanded from macro 'foreach'       for ((cell) = list_head(l);
(cell)!= NULL; (cell) = lnext(cell))                                   ^~~~~~~~~~~~~~
 

If by some mischance the condition intp->opfamily_id ==
key->partopfamily[l - 1] is never satisfied, this is going to seg
fault, which isn't good.  It's pretty easy to imagine how this could
be caused by corrupted system catalog contents or some other bug, so I
think you should initialize the variable to NULL and elog() if it's
still NULL when the loop exits.  There is a similar problem in one
other place.

But actually, I think maybe this logic should just go away altogether,
because backtracking when we realize that an unbounded bound follows
is pretty ugly. Can't we just fix the loop that precedes it so that it
treats next-bound-unbounded the same as this-is-the-last-bound (i.e.
when it is not the case that j - i < num_or_arms)?

+        if (partexprs_item)
+            partexprs_item_saved = *partexprs_item;

Is there a reason why you're saving the contents of the ListCell
instead of just a pointer to it?  If key->partexprs == NIL,
partexprs_item_saved will not get initialized, but the next loop will
still point to it.  That's dangerously close to a live bug, and at any
rate a compiler warning seems likely.

I don't really understand the motivation behind the
nulltest_generated[] array.  In the upper loop, we'll use any given
value of i in only one loop iteration, so having logic to prevent a
nulltest more than once doesn't seem like it will ever actually do
anything.  In the lower loop, it's actually doing something, but if
(num_or_arms == 0) /* Only do this the first time through */ would
have the same effect, I think.

I suggest num_or_arms -> current_or_arm and maxnum_or_arms -> num_or_arms.

The for_both_cell loop seems to have two different exit conditions:
either we can run off the lists, or we can find j - i sufficiently
large.  But shouldn't those things happen at the same time?

+     * If both lower_or_arms and upper_or_arms are empty, we append a
+     * constant-true expression.  That happens if all of the literal values in
+     * both the lower and upper bound lists are UNBOUNDED.     */
-    if (!OidIsValid(operoid))
+    if (lower_or_arms == NIL && upper_or_arms == NIL)
+        result = lappend(result, makeBoolConst(true, false));

I think it would be better to instead say, at the very end after all
else is done:

if (result == NIL)   result = make_list1(makeBoolConst(true, false));

Next update by tomorrow.

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