Обсуждение: BUG #16276: Server crash on an invalid attempt to attach a partition to an index

The following bug has been logged on the website:

Bug reference:      16276
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 12.2
Operating system:   Ubuntu 18.04
Description:

The following query:
create table idxpart(a int) partition by list (a);
create index idxpart_idx on idxpart (a);
create table idxpart1(a int);
alter table idxpart_idx attach partition idxpart1 for values in (0);

leads to a server crash with the following stack trace:
Core was generated by `postgres: law regression [local] ALTER TABLE
                        '.
Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007f323fe8c801 in __GI_abort () at abort.c:79
#2  0x0000561410882373 in ExceptionalCondition
(conditionName=conditionName@entry=0x561410986ee5 "cmd->bound == NULL", 
    errorType=errorType@entry=0x5614108dbf68 "FailedAssertion", 
    fileName=fileName@entry=0x561410986c97 "parse_utilcmd.c",
lineNumber=lineNumber@entry=3702) at assert.c:67
#3  0x000056141052a209 in transformPartitionCmd
(cxt=cxt@entry=0x7fff273a9d20, cmd=cmd@entry=0x561410fd5748)
    at parse_utilcmd.c:3702
#4  0x000056141052aac4 in transformAlterTableStmt (relid=16388,
stmt=0x561410fd55e0, stmt@entry=0x561410fd54f8, 
    queryString=0x561410fad630 "alter table idxpart_idx attach partition
idxpart1 for values in (0);", 
    beforeStmts=beforeStmts@entry=0x7fff273a9e38,
afterStmts=afterStmts@entry=0x7fff273a9e40) at parse_utilcmd.c:3282
#5  0x000056141059aadc in ATParseTransformCmd
(wqueue=wqueue@entry=0x7fff273a9f80, tab=tab@entry=0x561411070bb8, 
    rel=rel@entry=0x7f3240fc4f50, cmd=0x56141103e3c8,
recurse=recurse@entry=false, lockmode=lockmode@entry=4, 
    cur_pass=10, context=0x7fff273aa090) at tablecmds.c:4661
#6  0x000056141059f170 in ATExecCmd (wqueue=wqueue@entry=0x7fff273a9f80,
tab=tab@entry=0x561411070bb8, 
    rel=rel@entry=0x7f3240fc4f50, cmd=<optimized out>,
lockmode=lockmode@entry=4, cur_pass=cur_pass@entry=10, 
    context=0x7fff273aa090) at tablecmds.c:4590
#7  0x000056141059f2de in ATRewriteCatalogs
(wqueue=wqueue@entry=0x7fff273a9f80, lockmode=lockmode@entry=4, 
    context=context@entry=0x7fff273aa090) at tablecmds.c:4286
#8  0x000056141059f53f in ATController
(parsetree=parsetree@entry=0x561410fae2e0, rel=rel@entry=0x7f3240fc4f50, 
    cmds=0x561410fae318, recurse=true, lockmode=lockmode@entry=4,
context=context@entry=0x7fff273aa090)
    at tablecmds.c:3916
#9  0x000056141059f5c5 in AlterTable (stmt=stmt@entry=0x561410fae2e0,
lockmode=lockmode@entry=4, 
    context=context@entry=0x7fff273aa090) at tablecmds.c:3572
#10 0x00005614107637f3 in ProcessUtilitySlow
(pstate=pstate@entry=0x56141103e2b0, pstmt=pstmt@entry=0x561410fae3c0, 
    queryString=queryString@entry=0x561410fad630 "alter table idxpart_idx
attach partition idxpart1 for values in (0);",
context=context@entry=PROCESS_UTILITY_TOPLEVEL, params=params@entry=0x0,
queryEnv=queryEnv@entry=0x0, 
    dest=0x561410fae660, completionTag=0x7fff273aa5c0 "") at
utility.c:1276
#11 0x0000561410763327 in standard_ProcessUtility (pstmt=0x561410fae3c0, 
    queryString=0x561410fad630 "alter table idxpart_idx attach partition
idxpart1 for values in (0);", 
    context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
dest=0x561410fae660, completionTag=0x7fff273aa5c0 "")
    at utility.c:1076
#12 0x00005614107633d5 in ProcessUtility (pstmt=pstmt@entry=0x561410fae3c0,
queryString=<optimized out>, 
    context=context@entry=PROCESS_UTILITY_TOPLEVEL, params=<optimized out>,
queryEnv=<optimized out>, 
    dest=dest@entry=0x561410fae660, completionTag=0x7fff273aa5c0 "") at
utility.c:526
#13 0x000056141075f718 in PortalRunUtility
(portal=portal@entry=0x5614110149f0, pstmt=pstmt@entry=0x561410fae3c0, 
    isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x561410fae660,

    completionTag=completionTag@entry=0x7fff273aa5c0 "") at pquery.c:1175
#14 0x00005614107603b4 in PortalRunMulti
(portal=portal@entry=0x5614110149f0, isTopLevel=isTopLevel@entry=true, 
    setHoldSnapshot=setHoldSnapshot@entry=false,
dest=dest@entry=0x561410fae660, altdest=altdest@entry=0x561410fae660, 
    completionTag=completionTag@entry=0x7fff273aa5c0 "") at pquery.c:1321
#15 0x0000561410761127 in PortalRun (portal=portal@entry=0x5614110149f0,
count=count@entry=9223372036854775807, 
    isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
dest=dest@entry=0x561410fae660, 
    altdest=altdest@entry=0x561410fae660, completionTag=0x7fff273aa5c0 "")
at pquery.c:796
#16 0x000056141075d1db in exec_simple_query (
    query_string=query_string@entry=0x561410fad630 "alter table idxpart_idx
attach partition idxpart1 for values in (0);") at postgres.c:1227
#17 0x000056141075f357 in PostgresMain (argc=<optimized out>,
argv=argv@entry=0x561410fd8ae0, dbname=<optimized out>, 
    username=<optimized out>) at postgres.c:4291
#18 0x00005614106ceb8c in BackendRun (port=port@entry=0x561410fd11c0) at
postmaster.c:4509
#19 0x00005614106d1dbf in BackendStartup (port=port@entry=0x561410fd11c0) at
postmaster.c:4197
#20 0x00005614106d20d6 in ServerLoop () at postmaster.c:1727
#21 0x00005614106d3514 in PostmasterMain (argc=3, argv=<optimized out>) at
postmaster.c:1400
#22 0x0000561410622347 in main (argc=3, argv=0x561410fa7a00) at main.c:210

Reproduced on REL_11_0..master.

On Tue, Feb 25, 2020 at 04:00:01PM +0000, PG Bug reporting form wrote:
> The following query:
> create table idxpart(a int) partition by list (a);
> create index idxpart_idx on idxpart (a);
> create table idxpart1(a int);
> alter table idxpart_idx attach partition idxpart1 for values in (0);
>
> leads to a server crash with the following stack trace:
> Core was generated by `postgres: law regression [local] ALTER TABLE

Attempting to attach a table to a partitioned index?  Nice thought.
Without the assertion, RangeVarCallbackForAttachIndex complains that
the relation is not an index, which is right, so I would be tempted to
just remove the culprit assertion.  Any thoughts?
--
Michael

On Wed, Feb 26, 2020 at 05:06:24PM +0900, Michael Paquier wrote:
> Attempting to attach a table to a partitioned index?  Nice thought.
> Without the assertion, RangeVarCallbackForAttachIndex complains that
> the relation is not an index, which is right, so I would be tempted to
> just remove the culprit assertion.  Any thoughts?

FWIW, one can note that ALTER TABLE ATTACH PARTITION with indexes
correctly registers partition indexes without partition bounds in
pg_class, even if the grammar cannot accept ALTER TABLE without any
bounds defined:
create table idxpart(a int) partition by list (a);
create table idxpart1 partition of idxpart for values in (1);
create index idxpart_idx on only idxpart(a);
create index idxpart1_idx on only idxpart1(a);
alter table idxpart_idx attach partition idxpart1_idx for values in (0);

So I would rather not issue an error in that case on compatibility
ground.
--
Michael

On Thu, Feb 27, 2020 at 11:31 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Feb 26, 2020 at 05:06:24PM +0900, Michael Paquier wrote:
> > Attempting to attach a table to a partitioned index?  Nice thought.
> > Without the assertion, RangeVarCallbackForAttachIndex complains that
> > the relation is not an index, which is right, so I would be tempted to
> > just remove the culprit assertion.  Any thoughts?
>
> FWIW, one can note that ALTER TABLE ATTACH PARTITION with indexes
> correctly registers partition indexes without partition bounds in
> pg_class, even if the grammar cannot accept ALTER TABLE without any
> bounds defined:
> create table idxpart(a int) partition by list (a);
> create table idxpart1 partition of idxpart for values in (1);
> create index idxpart_idx on only idxpart(a);
> create index idxpart1_idx on only idxpart1(a);
> alter table idxpart_idx attach partition idxpart1_idx for values in (0);
>
> So I would rather not issue an error in that case on compatibility
> ground.

Simply dropping the culprit assertion might be enough for
back-branches, but it seems misleading to silently ignore the bound
IMO.  Maybe, report an error in newer versions, as follows:

@@ -3698,8 +3698,17 @@ transformPartitionCmd(CreateStmtContext *cxt,
PartitionCmd *cmd)
                                                          cmd->bound);
             break;
         case RELKIND_PARTITIONED_INDEX:
-            /* nothing to check */
-            Assert(cmd->bound == NULL);
+            /*
+             * ALTER INDEX, which does not allow partition bound to be
+             * specified, must be used when trying to attach partition of an
+             * index.
+             */
+            if (cmd->bound != NULL)
+                ereport(ERROR,
+                        (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                         errmsg("\"%s\" is not a partitioned table",
+                                RelationGetRelationName(parentRel)),
+                         errhint("Use ALTER INDEX to attach index
partition.")));

?

So,

create table idxpart(a int) partition by list (a);
create index idxpart_idx on idxpart (a);
create table idxpart1(a int);
alter table idxpart_idx attach partition idxpart1 for values in (0);
ERROR:  "idxpart_idx" is not a partitioned table
HINT:  Use ALTER INDEX to attach index partition.

Thanks,
Amit

On Thu, Feb 27, 2020 at 05:14:28PM +0900, Amit Langote wrote:
> Simply dropping the culprit assertion might be enough for
> back-branches, but it seems misleading to silently ignore the bound
> IMO.

Yeah, I have been considering this option, still I am hesitating about
what to do on HEAD.  My own take would be to fix this issue on all
branches by removing the assertion, but I think that we should wait
first a couple of days to see if Alvaro can comment on this issue.
--
Michael

On Thu, Feb 27, 2020 at 09:49:52PM +0900, Michael Paquier wrote:
> Yeah, I have been considering this option, still I am hesitating about
> what to do on HEAD.  My own take would be to fix this issue on all
> branches by removing the assertion, but I think that we should wait
> first a couple of days to see if Alvaro can comment on this issue.

Okay, after sleeping on it for a couple of days, let's block the case
on HEAD and remove the assertion in the back branches.  Could you add
a test case in indexing.sql for the patch of HEAD?
--
Michael

On Mon, Mar 2, 2020 at 12:39 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Feb 27, 2020 at 09:49:52PM +0900, Michael Paquier wrote:
> > Yeah, I have been considering this option, still I am hesitating about
> > what to do on HEAD.  My own take would be to fix this issue on all
> > branches by removing the assertion, but I think that we should wait
> > first a couple of days to see if Alvaro can comment on this issue.
>
> Okay, after sleeping on it for a couple of days, let's block the case
> on HEAD and remove the assertion in the back branches.  Could you add
> a test case in indexing.sql for the patch of HEAD?

Done, please see attached.

Thanks,
Amit

On Mon, Mar 02, 2020 at 07:39:44PM +0900, Amit Langote wrote:
> Done, please see attached.

Thanks.  There are actually some safeguards in ATPrepCmd()@tablecmds.c
when calling ATSimplePermissions().  However, as this is checked for
both (ATT_TABLE | ATT_PARTITIONED_INDEX) then it would not blow up as
ALTER INDEX and ALTER TABLE use the same code path.  Using DETACH
partition triggers this code path's error though, making the hint
added in your patch actually correct.

So, applied this stuff down to 11, with test cases on all branches.  I
have added a test case with ALTER TABLE DETACH PARTITION for an index
while on it.  As the error is triggered at transformation time which
is a tad more generic than the work done in tablecmds.c for each
command, I have also removed the error hint and kept the code
simpler.
--
Michael

On Tue, Mar 3, 2020 at 2:01 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, Mar 02, 2020 at 07:39:44PM +0900, Amit Langote wrote:
> > Done, please see attached.
>
> Thanks.  There are actually some safeguards in ATPrepCmd()@tablecmds.c
> when calling ATSimplePermissions().  However, as this is checked for
> both (ATT_TABLE | ATT_PARTITIONED_INDEX) then it would not blow up as
> ALTER INDEX and ALTER TABLE use the same code path.  Using DETACH
> partition triggers this code path's error though, making the hint
> added in your patch actually correct.
>
> So, applied this stuff down to 11, with test cases on all branches.  I
> have added a test case with ALTER TABLE DETACH PARTITION for an index
> while on it.  As the error is triggered at transformation time which
> is a tad more generic than the work done in tablecmds.c for each
> command, I have also removed the error hint and kept the code
> simpler.

Thank you.  (I totally missed this email.)

- Amit