Re: Multi-Column List Partitioning - Mailing list pgsql-hackers

On Fri, May 21, 2021 at 1:02 PM Amit Langote <amitlangote09@gmail.com> wrote:
> I will now take a look at the patch itself.

Some quick observations:

* I get a lot of instances of the following 2 warnings when compiling
the patched code:

Warning #1:

partprune.c: In function ‘get_matching_list_bounds’:
partprune.c:2731:11: warning: passing argument 5 of
‘partition_list_bsearch’ makes pointer from integer without a cast
[enabled by default]
           nvalues, value, &is_equal);
           ^
In file included from partprune.c:53:0:
../../../src/include/partitioning/partbounds.h:117:12: note: expected
‘Datum *’ but argument is of type ‘Datum’
 extern int partition_list_bsearch(FmgrInfo *partsupfunc,

Warning #2:

partprune.c:2781:12: warning: incompatible integer to pointer
conversion passing 'Datum'
      (aka 'unsigned long') to parameter of type 'Datum *' (aka
'unsigned long *'); take the
      address with & [-Wint-conversion]

          value, &is_equal);

          ^~~~~

          &
../../../src/include/partitioning/partbounds.h:120:32: note: passing
argument to parameter 'value'
      here
  ...int nvalues, Datum *value, bool *is_equal);

* I think this code:

===
        /* Get the only column's name in case we need to output an error */
        if (key->partattrs[0] != 0)
            colname = get_attname(RelationGetRelid(parent),
                                  key->partattrs[0], false);
        else
            colname = deparse_expression((Node *) linitial(partexprs),

deparse_context_for(RelationGetRelationName(parent),

RelationGetRelid(parent)),
                                         false, false);
        /* Need its type data too */
        coltype = get_partition_col_typid(key, 0);
        coltypmod = get_partition_col_typmod(key, 0);
        partcollation = get_partition_col_collation(key, 0);
===

belongs in the new function transformPartitionListBounds that you
added, because without doing so, any errors having to do with
partitioning columns other than the first one will report the first
column's name in the error message:

postgres=# create table foo (a bool, b bool) partition by list (a, b);
CREATE TABLE

-- this is fine!
postgres=# create table foo_true_true partition of foo for values in (1, true);
ERROR:  specified value cannot be cast to type boolean for column "a"
LINE 1: ...able foo_true_true partition of foo for values in (1, true);

-- not this!
postgres=# create table foo_true_true partition of foo for values in (true, 1);
ERROR:  specified value cannot be cast to type boolean for column "a"
LINE 1: ...able foo_true_true partition of foo for values in (true, 1);

* The following prototype of transformPartitionListBounds() means that
all values in a given bound list are analyzed with the first
partitioning column's colname, type, typmod, etc., which is wrong:

+static List *
+transformPartitionListBounds(ParseState *pstate, PartitionBoundSpec *spec,
+                            char *colname, Oid coltype, int32 coltypmod,
+                            Oid partcollation, int partnatts)
+{

An example of wrong behavior because of that:

postgres=# create table foo (a bool, b text) partition by list (a, b);
CREATE TABLE
Time: 3.967 ms
postgres=# create table foo_true_true partition of foo for values in
(true, 'whatever');
ERROR:  invalid input syntax for type boolean: "whatever"
LINE 1: ...o_true_true partition of foo for values in (true, 'whatever'...

"whatever" should've been accepted but because it's checked with a's
type, it is wrongly flagged.

Please take a look at how transformPartitionRangeBound() handles this,
especially how it uses the correct partitioning column's info to
analyze the corresponding bound value expression.

I will continue looking next week.

--
Amit Langote
EDB: http://www.enterprisedb.com