Re: Multi-Column List Partitioning

> I have been testing these patches. Patches applied cleanly on the head. While testing I found below a case where update row movement is not working properly.
> Please find the test case below.

Thanks for testing and sharing the details of the issue.

> Yeah, contrary to my earlier assessment, it seems the partition
> constraint on each of those partitions fails to explicitly include an
> IS NOT NULL test for each column that has a non-NULL value assigned.
> So, for example, the constraint of p01 should actually be:
>
> (a IS NOT NULL) AND (a = 1) AND (b IS NOT NULL) AND (b = 1) AND (c IS
> NOT NULL) AND (c = true)

Yes. It should add an IS NOT NULL test for each column. I have
modified the patch accordingly and verified with the test case shared
by Rajkumar.

> + * isnulls is an array of boolean-tuples with key->partnatts booleans values
> + * each. Currently only used for list partitioning, it stores whether a
>
> I think 'booleans' should be 'boolean'.
> The trailing word 'each' is unnecessary.
>
> bq. Supported new syantx to allow mentioning multiple key information.
>
> syantx -> syntax
>
> + isDuplicate = checkForDuplicates(result, values);
> + if (isDuplicate)
> + continue;
>
> It seems the variable isDuplicate is not needed. The if statement can directly check the return value from checkForDuplicates().

The attached patch also fixes the above comments.

Thanks & Regards,
Nitin Jadhav
On Tue, Aug 31, 2021 at 9:36 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Mon, Aug 30, 2021 at 4:51 PM Rajkumar Raghuwanshi
> <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:
> >
> > Hi Nitin.
> >
> > I have been testing these patches. Patches applied cleanly on the head. While testing I found below a case where update row movement is not working properly.
> > Please find the test case below.
> >
> > postgres=# create table p0 (a int, b text, c bool) partition by list (a,b,c);
> > CREATE TABLE
> > postgres=# create table p01 partition of p0 for values in ((1,1,true));
> > CREATE TABLE
> > postgres=# create table p02 partition of p0 for values in ((1,NULL,false));
> > CREATE TABLE
> > postgres=# insert into p0 values (1,'1',true);
> > INSERT 0 1
> > postgres=# insert into p0 values (1,NULL,false);
> > INSERT 0 1
> > postgres=# select tableoid::regclass,* from p0;
> > tableoid | a | b | c
> > ----------+---+---+---
> > p01 | 1 | 1 | t
> > p02 | 1 | | f
> > (2 rows)
> >
> > postgres=# update p0 set b = NULL;
> > UPDATE 2
> > postgres=# select tableoid::regclass,* from p0;
> > tableoid | a | b | c
> > ----------+---+---+---
> > p01 | 1 | | t
> > p02 | 1 | | f
> > (2 rows)
> >
> > I think this update should fail as there is no partition satisfying update row (1,NULL,true).
>
> Yeah, contrary to my earlier assessment, it seems the partition
> constraint on each of those partitions fails to explicitly include an
> IS NOT NULL test for each column that has a non-NULL value assigned.
> So, for example, the constraint of p01 should actually be:
>
> (a IS NOT NULL) AND (a = 1) AND (b IS NOT NULL) AND (b = 1) AND (c IS
> NOT NULL) AND (c = true)
>
> As per the patch's current implementation, tuple (1, NULL, true)
> passes p01's partition constraint, because only (b = 1) is not
> sufficient to reject a NULL value being assigned to b.
>
> --
> Amit Langote
> EDB: http://www.enterprisedb.com