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

> I have been testing these patches. Patches applied cleanly on the head. While testing I found below a case where
updaterow 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@gmail.com> wrote:
>
> On Mon, Aug 30, 2021 at 4:51 PM Rajkumar Raghuwanshi
> <rajkumar.raghuwanshi@enterprisedb.com> wrote:
> >
> > Hi Nitin.
> >
> > I have been testing these patches. Patches applied cleanly on the head. While testing I found below a case where
updaterow 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