Thread: [HACKERS] AT detach partition is broken
I noticed that running ALTER TABLE table_name DETACH PARTITION crashes, if table_name is not a partitioned table. That's because of an Assert in ATExecDetachPartition(). We really should error out much sooner in this case, IOW during transformAlterTableStmt(), as is done in the case of ATTACH PARTITION. Attached patch fixes that. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Mon, Feb 13, 2017 at 2:30 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > I noticed that running ALTER TABLE table_name DETACH PARTITION crashes, if > table_name is not a partitioned table. That's because of an Assert in > ATExecDetachPartition(). We really should error out much sooner in this > case, IOW during transformAlterTableStmt(), as is done in the case of > ATTACH PARTITION. > > Attached patch fixes that. - /* assign transformed values */ - partcmd->bound = cxt.partbound; + /* + * Assign transformed value of the partition bound, if + * any. + */ + if (cxt.partbound != NULL) + partcmd->bound = cxt.partbound; This hunk isn't really needed, is it? I mean, if cxt.partbound comes out NULL, then partcmd->bound will be NULL with or without adding an "if" here, won't it? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2017/02/15 2:37, Robert Haas wrote: > On Mon, Feb 13, 2017 at 2:30 AM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> I noticed that running ALTER TABLE table_name DETACH PARTITION crashes, if >> table_name is not a partitioned table. That's because of an Assert in >> ATExecDetachPartition(). We really should error out much sooner in this >> case, IOW during transformAlterTableStmt(), as is done in the case of >> ATTACH PARTITION. >> >> Attached patch fixes that. > > - /* assign transformed values */ > - partcmd->bound = cxt.partbound; > + /* > + * Assign transformed value of the partition bound, if > + * any. > + */ > + if (cxt.partbound != NULL) > + partcmd->bound = cxt.partbound; > > This hunk isn't really needed, is it? I mean, if cxt.partbound comes > out NULL, then partcmd->bound will be NULL with or without adding an > "if" here, won't it? You're right. Took this one out (except slightly tweaking the comment) in the attached updated patch. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Wed, Feb 15, 2017 at 7:45 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > You're right. Took this one out (except slightly tweaking the comment) in > the attached updated patch. Committed after tweaking the other comment in that function. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company