[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]https://www.postgresql.org/docs/current/sql-altertable.html, replica identity includes only columns marked 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
Attachments:
0001-Disallow-DROP-NOT-NULL-for-column-in-the-REPLICA-IDE.patchapplication/octet-stream; name=0001-Disallow-DROP-NOT-NULL-for-column-in-the-REPLICA-IDE.patchDownload+22-6
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.
--
MichaelOn 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
Attachments:
v2-0001-Disallow-DROP-NOT-NULL-on-a-column-in-the-REPLICA.patchapplication/octet-stream; name=v2-0001-Disallow-DROP-NOT-NULL-on-a-column-in-the-REPLICA.patchDownload+35-9
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