[WIP] ALTER COLUMN IF EXISTS
Hi,
I'm interested in adding more ergonomics to DDL commands, in
particular supporting IF EXISTS for ALTER TABLE … ALTER COLUMN, so
that if a column doesn't exist the command is skipped.
IF EXISTS is already supported in various places (e.g. ALTER TABLE …
ADD COLUMN IF NOT EXISTS, and ALTER TABLE … DROP COLUMN IF EXISTS),
but it's not available for any of the ALTER COLUMN sub commands.
The motivation is to make it easier to write idempotent migrations
that can be incrementally authored, such that they can be re-executed
multiple times without having to write an "up" and "down" migration.
https://github.com/graphile/migrate#idempotency elaborates a bit more
on the approach.
The current approach I see is to write something like:
DO $$
BEGIN
IF EXISTS (SELECT 1
FROM information_schema.columns
WHERE table_schema = 'myschema' AND table_name = 'mytable' AND
column_name = 'mycolume')
THEN
ALTER TABLE myschema.mytable RENAME mycolume TO mycolumn;
END IF;
END
$$;
I think ideally the IF EXISTS would be added to all of the ALTER
COLUMN commands, however for the moment I have only added it to the {
SET | DROP } NOT NULL command to demonstrate the approach and see if
there's in-principle support for such a change.
Questions:
1. I assume this is not part of the SQL specification, so this would
introduce more deviation to PostgreSQL. Is that accurate? Is that
problematic?
2. I believe I'm missing some code paths for table inheritance, is that correct?
3. I haven't updated the documentation—is it correct to do that in
doc/src/sgml/ref/alter_table.sgml?
4. This is my first time attempting to contribute to PostgreSQL, have
I missed anything?
--
Cheers,
Brad
Attachments:
v1-0001-Add-IF-EXISTS-support-to-ALTER-COLUMN-SET-DROP-NO.patchapplication/octet-stream; name=v1-0001-Add-IF-EXISTS-support-to-ALTER-COLUMN-SET-DROP-NO.patchDownload
From cbffe37aa9ea8ab4303a0723605b1b8def1b966b Mon Sep 17 00:00:00 2001
From: Bradley Ayers <bradley.ayers@gmail.com>
Date: Fri, 1 Apr 2022 00:25:16 +1100
Subject: [PATCH v1] =?UTF-8?q?Add=20IF=20EXISTS=20support=20to=20ALTER=20C?=
=?UTF-8?q?OLUMN=20=E2=80=A6=20{SET|DROP}=20NOT=20NULL?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This brings the ergonomics of the ADD COLUMN IF EXISTS command to the
ALTER TABLE ... SET/DROP NOT NULL commands.
---
src/backend/commands/tablecmds.c | 50 ++++++++++++++++-------
src/backend/parser/gram.y | 20 +++++++++
src/test/regress/expected/alter_table.out | 12 ++++++
src/test/regress/sql/alter_table.sql | 10 +++++
4 files changed, 78 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3e83f375b5..b28c5fc8ad 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -422,13 +422,13 @@ static bool check_for_column_name_collision(Relation rel, const char *colname,
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
static void add_column_collation_dependency(Oid relid, int32 attnum, Oid collid);
static void ATPrepDropNotNull(Relation rel, bool recurse, bool recursing);
-static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode);
+static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, bool missing_ok, LOCKMODE lockmode);
static void ATPrepSetNotNull(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, bool recursing,
LOCKMODE lockmode,
AlterTableUtilityContext *context);
static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
- const char *colName, LOCKMODE lockmode);
+ const char *colName, bool missing_ok, LOCKMODE lockmode);
static void ATExecCheckNotNull(AlteredTableInfo *tab, Relation rel,
const char *colName, LOCKMODE lockmode);
static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
@@ -4915,10 +4915,10 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
address = ATExecDropIdentity(rel, cmd->name, cmd->missing_ok, lockmode);
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
- address = ATExecDropNotNull(rel, cmd->name, lockmode);
+ address = ATExecDropNotNull(rel, cmd->name, cmd->missing_ok, lockmode);
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
- address = ATExecSetNotNull(tab, rel, cmd->name, lockmode);
+ address = ATExecSetNotNull(tab, rel, cmd->name, cmd->missing_ok, lockmode);
break;
case AT_CheckNotNull: /* check column is already marked NOT NULL */
ATExecCheckNotNull(tab, rel, cmd->name, lockmode);
@@ -7128,7 +7128,7 @@ ATPrepDropNotNull(Relation rel, bool recurse, bool recursing)
* nullable, InvalidObjectAddress is returned.
*/
static ObjectAddress
-ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
+ATExecDropNotNull(Relation rel, const char *colName, bool missing_ok, LOCKMODE lockmode)
{
HeapTuple tuple;
Form_pg_attribute attTup;
@@ -7145,10 +7145,21 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
if (!HeapTupleIsValid(tuple))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg("column \"%s\" of relation \"%s\" does not exist",
- colName, RelationGetRelationName(rel))));
+ {
+ if (!missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" of relation \"%s\" does not exist",
+ colName, RelationGetRelationName(rel))));
+ else
+ {
+ ereport(NOTICE,
+ (errmsg("column \"%s\" of relation \"%s\" does not exist, skipping",
+ colName, RelationGetRelationName(rel))));
+ table_close(attr_rel, RowExclusiveLock);
+ return InvalidObjectAddress;
+ }
+ }
attTup = (Form_pg_attribute) GETSTRUCT(tuple);
attnum = attTup->attnum;
@@ -7336,7 +7347,7 @@ ATPrepSetNotNull(List **wqueue, Relation rel,
*/
static ObjectAddress
ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
- const char *colName, LOCKMODE lockmode)
+ const char *colName, bool missing_ok, LOCKMODE lockmode)
{
HeapTuple tuple;
AttrNumber attnum;
@@ -7351,10 +7362,21 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
if (!HeapTupleIsValid(tuple))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg("column \"%s\" of relation \"%s\" does not exist",
- colName, RelationGetRelationName(rel))));
+ {
+ if (!missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" of relation \"%s\" does not exist",
+ colName, RelationGetRelationName(rel))));
+ else
+ {
+ ereport(NOTICE,
+ (errmsg("column \"%s\" of relation \"%s\" does not exist, skipping",
+ colName, RelationGetRelationName(rel))));
+ table_close(attr_rel, RowExclusiveLock);
+ return InvalidObjectAddress;
+ }
+ }
attnum = ((Form_pg_attribute) GETSTRUCT(tuple))->attnum;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a03b33b53b..aa816247d7 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2247,6 +2247,16 @@ alter_table_cmd:
AlterTableCmd *n = makeNode(AlterTableCmd);
n->subtype = AT_DropNotNull;
n->name = $3;
+ n->missing_ok = false;
+ $$ = (Node *)n;
+ }
+ /* ALTER TABLE <name> ALTER [COLUMN] IF EXISTS <colname> DROP NOT NULL */
+ | ALTER opt_column IF_P EXISTS ColId DROP NOT NULL_P
+ {
+ AlterTableCmd *n = makeNode(AlterTableCmd);
+ n->subtype = AT_DropNotNull;
+ n->name = $5;
+ n->missing_ok = true;
$$ = (Node *)n;
}
/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET NOT NULL */
@@ -2255,6 +2265,16 @@ alter_table_cmd:
AlterTableCmd *n = makeNode(AlterTableCmd);
n->subtype = AT_SetNotNull;
n->name = $3;
+ n->missing_ok = false;
+ $$ = (Node *)n;
+ }
+ /* ALTER TABLE <name> ALTER [COLUMN] IF EXISTS <colname> SET NOT NULL */
+ | ALTER opt_column IF_P EXISTS ColId SET NOT NULL_P
+ {
+ AlterTableCmd *n = makeNode(AlterTableCmd);
+ n->subtype = AT_SetNotNull;
+ n->name = $5;
+ n->missing_ok = true;
$$ = (Node *)n;
}
/* ALTER TABLE <name> ALTER [COLUMN] <colname> DROP EXPRESSION */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 16e0475663..cc6c9065d1 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1787,6 +1787,18 @@ alter table dropColumnExists drop column non_existing; --fail
ERROR: column "non_existing" of relation "dropcolumnexists" does not exist
alter table dropColumnExists drop column if exists non_existing; --succeed
NOTICE: column "non_existing" of relation "dropcolumnexists" does not exist, skipping
+-- ALTER TABLE ... ALTER COLUMN IF EXISTS ... SET NOT NULL test
+create table setNotNullColumnExists ();
+alter table setNotNullColumnExists alter column non_existing set not null; --fail
+ERROR: column "non_existing" of relation "setnotnullcolumnexists" does not exist
+alter table setNotNullColumnExists alter column if exists non_existing set not null; --succeed
+NOTICE: column "non_existing" of relation "setnotnullcolumnexists" does not exist, skipping
+-- ALTER TABLE ... ALTER COLUMN IF EXISTS ... DROP NOT NULL test
+create table dropNotNullColumnExists ();
+alter table dropNotNullColumnExists alter column non_existing drop not null; --fail
+ERROR: column "non_existing" of relation "dropnotnullcolumnexists" does not exist
+alter table dropNotNullColumnExists alter column if exists non_existing drop not null; --succeed
+NOTICE: column "non_existing" of relation "dropnotnullcolumnexists" does not exist, skipping
select relname, attname, attinhcount, attislocal
from pg_class join pg_attribute on (pg_class.oid = pg_attribute.attrelid)
where relname in ('p1','p2','c1','gc1') and attnum > 0 and not attisdropped
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index ac894c0602..18e1440b8c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1271,6 +1271,16 @@ create table dropColumnExists ();
alter table dropColumnExists drop column non_existing; --fail
alter table dropColumnExists drop column if exists non_existing; --succeed
+-- ALTER TABLE ... ALTER COLUMN IF EXISTS ... SET NOT NULL test
+create table setNotNullColumnExists ();
+alter table setNotNullColumnExists alter column non_existing set not null; --fail
+alter table setNotNullColumnExists alter column if exists non_existing set not null; --succeed
+
+-- ALTER TABLE ... ALTER COLUMN IF EXISTS ... DROP NOT NULL test
+create table dropNotNullColumnExists ();
+alter table dropNotNullColumnExists alter column non_existing drop not null; --fail
+alter table dropNotNullColumnExists alter column if exists non_existing drop not null; --succeed
+
select relname, attname, attinhcount, attislocal
from pg_class join pg_attribute on (pg_class.oid = pg_attribute.attrelid)
where relname in ('p1','p2','c1','gc1') and attnum > 0 and not attisdropped
--
2.34.1
On Thu, Mar 31, 2022 at 4:39 PM Bradley Ayers <bradley.ayers@gmail.com>
wrote:
I'm interested in adding more ergonomics to DDL commands, in
particular supporting IF EXISTS for ALTER TABLE … ALTER COLUMN, so
that if a column doesn't exist the command is skipped.IF EXISTS is already supported in various places (e.g. ALTER TABLE …
ADD COLUMN IF NOT EXISTS, and ALTER TABLE … DROP COLUMN IF EXISTS),
but it's not available for any of the ALTER COLUMN sub commands.
At present the project seems to largely consider the IF EXISTS/IF NOT
EXISTS features to have been largely a mistake and while removing it is not
going to happen the desire to change or extend it is not strong.
If you want to make a go at this I would suggest not writing any new code
at first but instead take inventory of what is already implemented, how it
is implemented, what gaps there are, and proposals to fill those gaps.
Write the theory/rules that we follow in our existing (or future)
implementation of this idempotence feature. Then get agreement to
implement the proposals from enough important people that a well-written
patch would be considered acceptable to commit.
I don't know if any amount of planning and presentation will convince
everyone this is a good idea in theory, let alone one that we want to
maintain while the author goes off to other projects (this being your first
patch that seems like a reasonable assumption).
I can say you have some community support in the endeavor but, and maybe
this is biasing me, my (fairly recent) attempt at what I considered
bug-fixing in this area was not accepted. On that note, as part of your
research, you should find the previous email threads on this topic (there
are quite a few I am sure), and make you own judgements from those. Aside
from it being my opinion I don't have any information at hand that isn't in
the email archives.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
On Thu, Mar 31, 2022 at 4:39 PM Bradley Ayers <bradley.ayers@gmail.com>
wrote:I'm interested in adding more ergonomics to DDL commands, in
particular supporting IF EXISTS for ALTER TABLE … ALTER COLUMN, so
that if a column doesn't exist the command is skipped.
At present the project seems to largely consider the IF EXISTS/IF NOT
EXISTS features to have been largely a mistake and while removing it is not
going to happen the desire to change or extend it is not strong.
That might be an overstatement. There's definitely a camp that
doesn't like CREATE IF NOT EXISTS, precisely on the grounds that it's
not idempotent --- success of the command tells you very little about
the state of the object, beyond the fact that some object of that name
now exists. (DROP IF EXISTS, by comparison, *is* idempotent: success
guarantees that the object now does not exist. CREATE OR REPLACE
is also idempotent, or at least much closer than IF NOT EXISTS.)
It's not entirely clear to me whether ALTER IF EXISTS could escape any
of that concern, but offhand it seems like it's close to the CREATE
problem. I do kind of wonder what the use-case for it is, anyway.
One thing to keep in mind is that unlike some other DBMSes, you
can script pretty much any conditional DDL you want in Postgres.
This considerably reduces the pressure to provide conditionalization
built right into the DDL commands. As a result, we (or at least I)
prefer to offer only the most clearly useful, best-defined cases
as built-in DDL features. So there's definitely a hurdle that
an ALTER IF EXISTS patch would have to clear before having a chance
of being accepted.
regards, tom lane
On Thu, Mar 31, 2022 at 8:02 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
At present the project seems to largely consider the IF EXISTS/IF NOT EXISTS features to have been largely a mistake and while removing it is not going to happen the desire to change or extend it is not strong.
I like the IF [NOT] EXISTS stuff quite a bit. I wish it had existed
back when I was doing application programming with PostgreSQL. I would
have used it for exactly the sorts of things that Bradley mentions.
I don't know how far it's worth taking this stuff. I dislike the fact
that when you get beyond what you can do with IF [NOT] EXISTS, you're
suddenly thrown into having to write SQL against system catalog
contents which, if you're the sort of person who really likes the IF
[NOT] EXISTS commands, may well be something you don't feel terribly
comfortable doing. It's almost tempting to propose new SQL functions
just for these kinds of scripts. Like instead of adding support
for....
ALTER TABLE myschema.mytable IF EXISTS RENAME IF EXISTS this TO that;
...and I presume you need IF EXISTS twice, once for the table and once
for the column, we could instead make it possible for people to write:
IF pg_table_exists('myschema.mytable') AND
pg_table_has_column('myschema.mytable', 'this') THEN
ALTER TABLE myschema.mytable RENAME this TO that;
END IF;
An advantage of that approach is that you could also do more
complicated things that are never going to work with any number of
IF-EXISTS clauses. For example, imagine you want to rename foo to bar
and bar to baz, unless that's been done already. Well with these
functions you can just do this:
IF pg_table_has_column('mytab', 'foo') THEN
ALTER TABLE mytab RENAME bar TO baz;
ALTER TABLE mytab RENAME foo TO bar;
END;
There's no way to get there with just IF EXISTS.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 1 Apr 2022, at 02:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
At present the project seems to largely consider the IF EXISTS/IF NOT
EXISTS features to have been largely a mistake and while removing it is not
going to happen the desire to change or extend it is not strong.That might be an overstatement.
ISTR that patches which have been rejected have largely added support for the
syntax for the sake of adding support for the syntax, not because there was a
need or usecase for it. When the patch is accompanied with an actual usecase
it's also easier to reason about.
Now, the usecase of "I wanted to to start working on PostgreSQL and this seemed
like a good first patch" is clearly also very important.
--
Daniel Gustafsson https://vmware.com/