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 =
falseto 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.
