Re: Multi-Column List Partitioning

Thanks for reviewing.

> Ok, colname can be fetched in advance but I don't think it worth it to
> fetch coltype, coltypmod & partcollation; and, store in the
> explicitly allocated memory, instead, you can directly call
> get_partition_col_* inline functions.

Fixed.
---

> /*
> * If the bound datums can be NULL, check that the datums on
> * both sides are either both NULL or not NULL.
> */
> if (b1->isnulls)
> {
> if (b1->isnulls[i][j] != b2->isnulls[i][j])
> return false;
>
> /* Must not pass NULL datums to datumIsEqual(). */
> if (b1->isnulls[i][j])
> continue;
> }
>
> /* < the long comment snipped >*/
> if (!datumIsEqual(b1->datums[i][j], b2->datums[i][j],
> parttypbyval[j], parttyplen[j]))
> return false;

Make sense. Fixed as per the suggestion.
---

> + i = 0;
> + foreach(cell2, rowexpr->args)
> + {
>
> It's up to you, rather than using a separate index variable and
> incrementing that at the end, I think we can use
> foreach_current_index(cell2) which would look much nicer.

Thanks for the suggestion. I have removed the increment operation and
retained the index variable with a call to foreach_current_index()
since the index variable is required in 3 places. It looks better than
before.
---

> + all_values[j].values = (Datum *) palloc0(key->partnatts *
> sizeof(Datum));
> + all_values[j].isnulls = (bool *) palloc0(key->partnatts *
> sizeof(bool));
> + all_values[j].index = i;
>
> palloc0 is unnecessary for the "values".

Fixed.
---

> dest->datums[i] = &boundDatums[i * natts];
> + if (src->isnulls)
> + dest->isnulls[i] = (bool *) palloc(sizeof(bool) * natts);
>
> I think you can allocate memory for isnulls the same way you do
> allocate boundDatums and just do the memcpy.

Fixed.
---

> + for (i = 0; i < partnatts; i++)
> + {
> + if (outer_isnull && outer_isnull[i])
> + {
> + outer_has_null = true;
> + if (outer_map.merged_indexes[outer_index] == -1)
> + consider_outer_null = true;
> + }
>
> I am wondering why you are not breaking the loop once you set
> consider_outer_null?
> Note that if you do that then you need a separate loop for the
> inner_isnull part.

Right. Fixed.
---

> I have doubts about the condition that allows reaching
> merge_null_partitions() but I am not sure I am correct. I think if the
> list values missing from the __inner side__ then we might need to
> check only "inner_has_null" & "consider_inner_null" and merge the
> same, but why is this code also checking "outer_has_null" &
> "consider_outer_null". Correct me if I am missing something.

You are correct. These conditions are not required. Fixed.
---

> + if (isnulls && isnulls[i])
> + cmpval = 0; /* NULL "=" NULL */
> + else
> + cmpval = 1; /* NULL ">" not-NULL */
> + }
> + else if (isnulls && isnulls[i])
> + cmpval = -1; /* not-NULL "<" NULL */
>
> I really doubt this assumption is correct; aren't those strict operators?

Now there are possibilities of multiple NULL values. We should have a
mechanism to sort it when the bound values contain Non NULL and NULL
values. As per the above logic we put the NULL values at the end.
Please let me know if I am wrong.
---

> +get_list_partbound_value_string(List *bound_value)
> +{
> + StringInfo buf = makeStringInfo();
> + StringInfo boundconstraint = makeStringInfo();
>
> boundconstraint should be declared inside "if (ncols > 1)" block.

Fixed.
---

> + foreach(cell, bound_value)
> + {
> + Const *val = castNode(Const, lfirst(cell));
> +
> + appendStringInfoString(buf, sep);
> + get_const_expr(val, &context, -1);
> + sep = ", ";
> + ncols++;
> + }
>
> I think no need to increment ncols every time, you have a list and you
> can get that. Also, I think since you have ncols already, you can
> prepend and append parenthesis before and after so that you can avoid
> extra StringInfo.

Fixed.
---

> typedef struct PartitionBoundInfoData
> {
> char strategy; /* hash, list or range? */
> + int partnatts; /* number of partition key columns */
> int ndatums; /* Length of the datums[] array */
> Datum **datums;
> + bool **isnulls;
>
> Adding "partnatts" to this struct seems to be unnecessary, AFAIUC,
> added that for partition_bound_accepts_nulls(), but we can easily get
> that value from the partitioning key & pass an additional argument.
> Also, no information about the length of the "isnulls" array.

This is required during merge_list_bounds(). AFAIK partition key
information is not available here.

> I think it would be helpful if you could split the patch: one for
> multi-value list partitioning and another for the partition wise join, thanks.

I have split the patch into 2 patches. One is for the multi column
list partitioning core changes and the other is for partition-wise
join support. Each patch has its respective test cases in the
regression suit and regression tests run successfully on each patch.
Kindly let me know if any other changes are required here.

Thanks & Regards,
Nitin Jadhav

On Tue, Dec 21, 2021 at 6:30 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Tue, Dec 21, 2021 at 2:47 PM Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
> > On Mon, Dec 20, 2021 at 7:04 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >> On Mon, Dec 13, 2021 at 11:37 PM Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
> >> >
> >> > Hi,
> >> >
> >> > Is this okay?
> >> >
> >> > postgres=# CREATE TABLE t1 (a int, b int) PARTITION BY LIST ( a, a, a );
> >> > CREATE TABLE
> >> >
> >> > postgres=# CREATE TABLE t1_1 PARTITION OF t1 FOR VALUES IN ((1, 2, 3), (4, 5, 6));
> >> > CREATE TABLE
> >> >
> >> > postgres=# \d t1
> >> > Partitioned table "public.t1"
> >> > Column | Type | Collation | Nullable | Default
> >> > --------+---------+-----------+----------+---------
> >> > a | integer | | |
> >> > b | integer | | |
> >> > Partition key: LIST (a, a, a)
> >> > Number of partitions: 1 (Use \d+ to list them.)
> >>
> >> I'd say it's not okay for a user to expect this to work sensibly, and
> >> I don't think it would be worthwhile to write code to point that out
> >> to the user if that is what you were implying.
> >
> > OK. As you wish.
>
> Actually, we *do* have some code in check_new_partition_bound() to
> point it out if an empty range is specified for a partition, something
> that one (or a DDL script) may accidentally do:
>
> /*
> * First check if the resulting range would be empty with
> * specified lower and upper bounds...
> */
> cmpval = partition_rbound_cmp(key->partnatts,
> key->partsupfunc,
> key->partcollation,
> lower->datums, lower->kind,
> true, upper);
> Assert(cmpval != 0);
> if (cmpval > 0)
> {
> /* Point to problematic key in the lower datums list. */
> PartitionRangeDatum *datum = list_nth(spec->lowerdatums,
> cmpval - 1);
>
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> errmsg("empty range bound specified for
> partition \"%s\"",
> relname),
> errdetail("Specified lower bound %s is
> greater than or equal to upper bound %s.",
>
> get_range_partbound_string(spec->lowerdatums),
>
> get_range_partbound_string(spec->upperdatums)),
> parser_errposition(pstate, datum->location)));
> }
>
> So one may wonder why we don't catch and point out more such user
> mistakes, like the one in your example. It may not be hard to
> implement a proof that the partition bound definition a user entered
> results in a self-contradictory partition constraint using the
> facilities given in predtest.c. (The empty-range proof seemed simple
> enough to implement as the above block of code.) I don't however see
> why we should do that for partition constraints if we don't do the
> same for CHECK constraints; for example, the following definition,
> while allowed, is not very useful:
>
> create table foo (a int check (a = 1 and a = 2));
> \d foo
> Table "public.foo"
> Column | Type | Collation | Nullable | Default
> --------+---------+-----------+----------+---------
> a | integer | | |
> Check constraints:
> "foo_a_check" CHECK (a = 1 AND a = 2)
>
> Maybe partitioning should be looked at differently than the free-form
> CHECK constraints, but I'm not so sure. Or if others insist that it
> may be worthwhile to improve the user experience in such cases, we
> could do that as a separate patch than the patch to implement
> multi-column list partitioning.
>
> --
> Amit Langote
> EDB: http://www.enterprisedb.com