The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed
Thank you for your contribution.
This is a useful feature. Although, there are so many places we alter a column
which don't support IF EXISTS. For example: ALTER COLUMN IF EXISTS. Why don't
we include the necessary changes across different use cases to this patch?
> + | ALTER TABLE IF_P EXISTS relation_expr RENAME opt_column IF_P EXISTS
> name TO name
Since this is my first review patch, can you help me understand why some
keywords are written with "_P" suffix?
> + | ALTER TABLE relation_expr RENAME opt_column IF_P EXISTS name TO name
> + {
> + RenameStmt *n = makeNode(RenameStmt);
> + n->renameType = OBJECT_COLUMN;
> + n->relationType = OBJECT_TABLE;
> + n->relation = $3;
> + n->subname = $8;
> + n->newname = $10;
> + n->missing_ok = false;
> + n->sub_missing_ok = true;
> + $$ = (Node *)n;
> + }
Copying alter table combinations (with and without IF EXISTS statements) makes
this patch hard to review and bloats the gram. Instead of copying, perhaps we
can use an optional syntax, like opt_if_not_exists of ALTER TYPE.
> + if (attnum == InvalidAttrNumber)
> + {
> + if (!stmt->sub_missing_ok)
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_COLUMN),
> + errmsg("column \"%s\" does not exist",
> + stmt->subname)));
> + else
> + {
> + ereport(NOTICE,
> + (errmsg("column \"%s\" does not exist, skipping",
> + stmt->subname)));
> + return InvalidObjectAddress;
> + }
> + }
> +
Other statements in gram.y includes sub_missing_ok = true and missing_ok =
false. Why don't we add sub_missing_ok = false to existing declarations where
IF EXISTS is not used?
> - <term><literal>RENAME ATTRIBUTE</literal></term>
> + <term><literal>RENAME ATTRIBUTE [ IF EXISTS ]</literal></term>
It seems that ALTER VIEW, ALTER TYPE, and ALTER MATERIALIZED VIEW does not have
any tests for this feature.
