Обсуждение: [BUG]Missing REPLICA IDENTITY check when DROP NOT NULL

Hi,

I think I found a problem related to replica identity. According to PG doc at [1], replica identity includes only
columnsmarked NOT NULL.  
But in fact users can accidentally break this rule as follows:

create table tbl (a int not null unique);
alter table tbl replica identity using INDEX tbl_a_key;
alter table tbl alter column a drop not null;
insert into tbl values (null);

As a result, some operations on newly added null value will cause unexpected failure as below:

postgres=# delete from tbl;
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

In the log, I can also see an assertion failure when deleting null value:
TRAP: FailedAssertion("!nulls[i]", File: "heapam.c", Line: 8439, PID: 274656)

To solve the above problem, I think it's better to add a check when executing  ALTER COLUMN DROP NOT NULL,
and report an error if this column is part of replica identity.

Attaching a patch that disallow DROP NOT NULL on a column if it's in a REPLICA IDENTITY index. Also added a test in it.
Thanks Hou for helping me write/review this patch.

By the way, replica identity was introduced in PG9.4, so this problem exists in
all supported versions.

[1] https://www.postgresql.org/docs/current/sql-altertable.html

Regards,
Tang

On Wed, Nov 24, 2021 at 07:04:51AM +0000, tanghy.fnst@fujitsu.com wrote:
> create table tbl (a int not null unique);
> alter table tbl replica identity using INDEX tbl_a_key;
> alter table tbl alter column a drop not null;
> insert into tbl values (null);

Oops.  Yes, that's obviously not good.

> To solve the above problem, I think it's better to add a check when
> executing  ALTER COLUMN DROP NOT NULL,
> and report an error if this column is part of replica identity.

I'd say that you are right to block the operation.  I'll try to play a
bit with this stuff tomorrow.

> Attaching a patch that disallow DROP NOT NULL on a column if it's in
> a REPLICA IDENTITY index. Also added a test in it.

       if (indexStruct->indkey.values[i] == attnum)
           ereport(ERROR,
                   (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
-                   errmsg("column \"%s\" is in a primary key",
+                   errmsg(ngettext("column \"%s\" is in a primary key",
+                                   "column \"%s\" is in a REPLICA IDENTITY index",
+                                   indexStruct->indisprimary),
                    colName)));
Using ngettext() looks incorrect to me here as it is used to get the
plural form of a string, so you'd better make these completely
separated instead.
--
Michael

On Wed, Nov 24, 2021 7:29 PM, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Nov 24, 2021 at 07:04:51AM +0000, tanghy.fnst@fujitsu.com wrote:
> > create table tbl (a int not null unique);
> > alter table tbl replica identity using INDEX tbl_a_key;
> > alter table tbl alter column a drop not null;
> > insert into tbl values (null);
>
> Oops.  Yes, that's obviously not good.
>
> > To solve the above problem, I think it's better to add a check when
> > executing  ALTER COLUMN DROP NOT NULL,
> > and report an error if this column is part of replica identity.
>
> I'd say that you are right to block the operation.  I'll try to play a
> bit with this stuff tomorrow.
>
> > Attaching a patch that disallow DROP NOT NULL on a column if it's in
> > a REPLICA IDENTITY index. Also added a test in it.
>
>        if (indexStruct->indkey.values[i] == attnum)
>            ereport(ERROR,
>                    (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> -                   errmsg("column \"%s\" is in a primary key",
> +                   errmsg(ngettext("column \"%s\" is in a primary key",
> +                                   "column \"%s\" is in a REPLICA IDENTITY index",
> +                                   indexStruct->indisprimary),
>                     colName)));
> Using ngettext() looks incorrect to me here as it is used to get the
> plural form of a string, so you'd better make these completely
> separated instead.

Thanks for your comment. I agree with you.
I have fixed it and attached v2 patch.

Regards,
Tang

On Thu, Nov 25, 2021 at 8:21 AM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:
>
> On Wed, Nov 24, 2021 7:29 PM, Michael Paquier <michael@paquier.xyz> wrote:

>
> Thanks for your comment. I agree with you.
> I have fixed it and attached v2 patch.

Good catch, your patch looks fine to me and solves the reported problem.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Nov 25, 2021 at 10:44:53AM +0530, Dilip Kumar wrote:
> Good catch, your patch looks fine to me and solves the reported problem.

And applied down to 10.  A couple of comments in the same area did not
get the call, though.
--
Michael