Add support of IF [NOT] EXISTS for ADD/DROP COLUMN in Postgresql #1627

lachaib · 2025-03-19T07:53:52Z

Description

Enable support of IF NOT EXISTS for AddColumn/DropColumn so that the flow from auto-generation to execution on PostgreSQL will work.

Took the opportunity to mention the if_exists/if_not_exists in a cookbook entry also for CreateTable/CreateIndex

Disclaimer: I'm still not fully proficient with alembic's code so don't hesitate to challenge the implementation.

Checklist

This pull request is:

A documentation / typographical error fix
- Good to go, no issue or tests are needed
A short code fix
- please include the issue number, and create an issue if none exists, which
  must include a complete example of the issue. one line code fixes without an
  issue and demonstration will not be accepted.
- Please include: Fixes: #<issue number> in the commit message
- please include tests. one line code fixes without tests will not be accepted.
A new feature implementation
- please include the issue number, and create an issue if none exists, which must
  include a complete example of how the feature would look.
- Please include: Fixes: #<issue number> in the commit message
- please include tests.

Have a nice day!

lachaib · 2025-03-31T19:16:11Z

@zzzeek @CaselIT , sorry for the mention but I feel like you're usually reactive and this PR may have passed behind your watc. I would really appreciate feedback on it.

zzzeek

hi -

while we can't go with any of the "postgresql_" hardcoding here outside of postgresql.py, there are plenty of other databases that support IF NOT EXISTS/IF EXISTS for alter table add/drop column, just googling shows there are a bunch like snowflake, etc.

So please just remove this from being specific to postgresql and implement in a similar way as if_exists/if_not_exists is done for create_index/drop_index/create_table/drop_table etc.

zzzeek · 2025-03-31T19:25:50Z

alembic/operations/ops.py

@@ -2034,16 +2034,20 @@ def __init__(
        column: Column[Any],
        *,
        schema: Optional[str] = None,
+        postgresql_if_not_exists: Optional[bool] = None,


we can't have the name "postgresql" in this file, this is not part of the postgresql implementation. see other op-level dialect-specific arguments we have, which I think at the moment is just mssql_drop_check and mssql_drop_forsign_key, for how to pull dialect-specific arguments from kw.

that said it's not clear if this argument should be postgresql_if_not_exists in the first place

We already have precedent here, like postgresql_using https://alembic.sqlalchemy.org/en/latest/ops.html#alembic.operations.Operations.alter_column.params.postgresql_using

Overall if only postgresql suppots it maybe it makes sense to start with it then expand if there is a request for it?

the word "postgresql" does not appear in the ops.py file outside of a docstring

we cannot have dialect-level rules hardcoded in the core of alembic full stop

the use case for this user is they explicitly want rendering to render this keyword

so that will not work, because we do not render the kw attribute of ops. They can make their own @render function to override that but that's pretty inconvenient

in this specific case, "if exists / if not exists" is extremely common

mariadb - that alone is reason enough
snowflake
jooq has a whole list including bigquery, cockroachdb, databricks, teradata - and they actually have syntaxes for Oracle, SQL Server too using short procedures, so we could even do those too

so in short this flag should be added without any prefix, however postgresql and mysql/mariadb impls can handle the rendering functions, need tests in test_postgresql test_mysql

ok, I guess the main difference here is that with postgresql_using we never expect it to be automatically rendered.

Regarding the availability in other db I missed that mariadb already had it.

zzzeek · 2025-03-31T19:31:25Z

alembic/autogenerate/render.py

+        op.schema,
+        op.table_name,
+        op.column,
+        op.postgresql_if_not_exists,


we can't have postgresql-specific logic in render here. but also, the op should not have a .postgresql_if_not_exists field anyway, it should be in .kw.

while all of the ops have a .kw field that stores additional dialect-specific keyword arguments, these are not included in renders right now. we can try adding these to renders but this would need tests across many

that said it's not clear if this argument should be called postgresql_if_not_exists anyway

lachaib · 2025-04-01T09:48:26Z

hi -

while we can't go with any of the "postgresql_" hardcoding here outside of postgresql.py, there are plenty of other databases that support IF NOT EXISTS/IF EXISTS for alter table add/drop column, just googling shows there are a bunch like snowflake, etc.

Hi, actually I went over the documentation and I couldn't find any list of officially supported engines, so I directed my search to the ones more obvious in the codebase: sqlite, oracle, mysql, mssql and postgresql.
Also I based my change (i.e. using postgreql_ prefix) on recommendation provided by @CaselIT (#524 (comment))

That being said, how should I handle dialects not supporting IF [NOT] EXISTS if I make it streamline for the others?

So please just remove this from being specific to postgresql and implement in a similar way as if_exists/if_not_exists is done for create_index/drop_index/create_table/drop_table etc.

I'd actually be glad to as it's way simpler 😄

CaselIT · 2025-04-01T10:49:44Z

Replied above regarding the prefix

lachaib · 2025-04-30T06:14:53Z

@zzzeek
OK so we're back to is_exists & if_not_exists and I added support for mariadb dialect in mysql, though according to what I saw mysql doesn't support it, so I did register only for mariadb. Is that ok?

zzzeek · 2025-04-30T12:41:11Z

@zzzeek OK so we're back to is_exists & if_not_exists and I added support for mariadb dialect in mysql, though according to what I saw mysql doesn't support it, so I did register only for mariadb. Is that ok?

I think it's best to register for both since use of the explicit "mariadb://" style URL is not common

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 1cd9a39 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-04-30T12:43:36Z

New Gerrit review created for change 1cd9a39: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879

lachaib · 2025-04-30T14:55:44Z

@zzzeek OK so we're back to is_exists & if_not_exists and I added support for mariadb dialect in mysql, though according to what I saw mysql doesn't support it, so I did register only for mariadb. Is that ok?

I think it's best to register for both since use of the explicit "mariadb://" style URL is not common

There you go with an updated version. Let's hope it suits you now :)

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 7702b01 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-04-30T18:48:14Z

Patchset 7702b01 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879

lachaib · 2025-05-02T11:47:18Z

CI is getting failures that seem unrelated to my work, any help would be appreciated to help me go further

zzzeek · 2025-05-02T12:36:12Z

CI is getting failures that seem unrelated to my work, any help would be appreciated to help me go further

that looks as a direct result of the patch here, the error is here: https://jenkins.sqlalchemy.org/job/alembic_gerrit/pyv=py313,sqla=sqlamain/1374/testReport/junit/tests.test_batch/BatchRoundTripPostgresqlTest_postgresql+psycopg2_16_8/test_drop_col_schematype/

File "/home/jenkins/workspace/alembic_gerrit/89e52482/sqlalchemy/alembic/alembic/ddl/impl.py", line 393, in drop_column
    base.DropColumn(
    ~~~~~~~~~~~~~~~^
        table_name,
        ^^^^^^^^^^^
    ...<2 lines>...
        **kw,
        ^^^^^
    )
    ^
TypeError: DropColumn.__init__() got an unexpected keyword argument 'existing_type'

that is code that was added by your patch on line 392:

https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879/2/alembic/ddl/impl.py#392

zzzeek · 2025-05-02T12:37:17Z

federico also made a comment on that code that maybe **kw should not be added there and I agree it seems like that would need to take bigger things into account

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 7154331 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-05-02T14:10:33Z

Patchset 7154331 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879

zzzeek · 2025-05-02T14:11:20Z

once the PR is working maybe I can go and see why kw isnt compatible there

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 5391908 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-05-02T14:14:31Z

Patchset 5391908 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879

sqla-tester

Hi, this is sqla-tester and I see you've pinged me for review. However, user lachaib is not authorized to initiate CI jobs. Please wait for a project member to do this!

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 5391908 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-05-07T15:30:08Z

Patchset 5391908 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879

zzzeek · 2025-05-07T15:33:05Z

hi -

we've requested changes here but I'm not sure they propagated to the PR. can you take a look at the comments listed at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879?tab=comments and make sure all those changes are made here? thanks

Fixes sqlalchemy#1626

lachaib · 2025-05-13T07:40:48Z

@zzzeek thanks for pointing me to gerrit I'm not used to it so I missed them, now all points there should be fixed, let me know

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 82d6319 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-05-13T13:05:19Z

Patchset 82d6319 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879

sqla-tester · 2025-05-14T17:31:24Z

Michael Bayer (zzzeek) wrote:

recheck
pep8-recheck
pep484-recheck

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879

zzzeek

close. can we simplify the rendering implementation? it looks like it added different kinds of methods to postgresql, mysql, mssql, seems like it should be more consolidated

zzzeek · 2025-05-15T03:32:17Z

alembic/ddl/mssql.py

@@ -222,7 +223,9 @@ def drop_column(
        drop_fks = kw.pop("mssql_drop_foreign_key", False)
        if drop_fks:
            self._exec(_ExecDropFKConstraint(table_name, column, schema))
-        super().drop_column(table_name, column, schema=schema, **kw)
+        super().drop_column(
+            table_name, column, schema=schema, if_exists=if_exists, **kw


does mssql support DROP COLUMN IF EXISTS ? there's no test and wondering why this is here

it doesn't, but I had a failure with mypy without it (signature of override differing from parent class)

several options come to my mind:

leave it so; mypy and linters happy, but can be misleading

leave if_exists in signature, but don't pass it through; mypy is happy other linters may complain

leave if_exists in signature, log a warning if not none, don't pass it through; linters are happy, users have a proper feedback of incorrect use

remove it from signature and make add # type: ignore comment; may silently impact any future change on this API

update parent signature to make the if_exist argument implicitly taken from **kw; mypy and linters happy, users and maintainer are not

My preferred option is the 3rd and I'm gonna submit an additional commit in this direction, we'll amend eventually if you select another one

zzzeek · 2025-05-15T03:32:31Z

alembic/ddl/mysql.py

+    element: AddColumn, compiler: MySQLDDLCompiler, **kw
+) -> str:
+
+    return "%s ADD COLUMN %s%s" % (


how come this is done so differently vs. the postgresql version?

Main element of explanation: I privileged local style consistency, and the implem files are way different one from the other. Also I did postgresql way before and worked on this file at another moment, hence privileging local style over daring to harmonise the contribution and make this change seem odd compared to the rest of the file

zzzeek · 2025-05-15T03:33:02Z

alembic/ddl/postgresql.py

+def visit_add_column(element: AddColumn, compiler: PGDDLCompiler, **kw) -> str:
+    return "%s %s" % (
+        alter_table(compiler, element.table_name, element.schema),
+        add_column(


why not just pass through if_exists / if_not_exists in base.py? it's just an argument, the compiler can still ignore it if it chooses. i dont think we need new AddColumn / DropColumn @compiles here

Given that alembic can be used to apply migration on several servers, my primary concern was that the automated generation of if_exist would possibly break for someone using a combination of postgresql and something else.

However you master way more than me the different usages of alembic and if you feel confident that we can move it all to base.py and remove it from implementations, I'd be happy to do so.
(It'll also solve most of the other questions)

I think let's start by doing this with the least code possible with base.py supporting "IF [NOT] EXISTS" completely, then let's look for some way in base.py / impl.py to simply block the "if_[not]_exists" parameter from being propagated by default, with something in mysql.py /postgresql.py that re-enables its propagation.

this will after all make it easier for third party dialects to do this same step

but to be clear, just do the first part of that paragraph unless you have a great idea for the second part. otherwise Ill take the succinct version of the code and figure something out.

zzzeek · 2025-05-15T03:33:15Z

alembic/ddl/postgresql.py

+    text = "ADD COLUMN "
+    if if_not_exists:
+        text += "IF NOT EXISTS "
+


why not put this in base.py? if I have a migration that is adding / dropping columns that may or may not exist already, can't it be assumed I'm using a database that supports this syntax? would I want if_exists / if_not_exists to be silently ignored if I passed it?

even if we dont want it to render we should likely keep this in base.py anyway and then control for when we actually render IF [NOT] EXISTS

I pushed the move to base.py as a distinct commit for you to validate, I'll squash changes prior to merging if you confirm it's ok for you.

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision c503b04 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-05-19T13:11:01Z

Patchset c503b04 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879

sqla-tester · 2025-05-19T16:15:27Z

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5879 has been merged. Congratulations! :)

lachaib · 2025-05-19T18:45:03Z

Thanks a lot for your patience and your help with this contribution

lachaib force-pushed the lachaib/issue1626 branch 2 times, most recently from 0a85a7f to c9a7bc2 Compare March 20, 2025 09:23

zzzeek requested changes Mar 31, 2025

View reviewed changes

zzzeek mentioned this pull request Apr 24, 2025

Add if_exists for dropping postgres constraints #1650

Closed

lachaib force-pushed the lachaib/issue1626 branch from c9a7bc2 to 1cd9a39 Compare April 30, 2025 06:11

lachaib requested review from zzzeek and CaselIT April 30, 2025 06:12

zzzeek requested a review from sqla-tester April 30, 2025 12:43

sqla-tester reviewed Apr 30, 2025

View reviewed changes

lachaib force-pushed the lachaib/issue1626 branch from 1cd9a39 to 7702b01 Compare April 30, 2025 14:54

CaselIT requested a review from sqla-tester April 30, 2025 18:48

sqla-tester reviewed Apr 30, 2025

View reviewed changes

lachaib force-pushed the lachaib/issue1626 branch 2 times, most recently from 89773d4 to 7154331 Compare May 2, 2025 14:07

zzzeek requested a review from sqla-tester May 2, 2025 14:10

sqla-tester reviewed May 2, 2025

View reviewed changes

lachaib force-pushed the lachaib/issue1626 branch from 7154331 to 5391908 Compare May 2, 2025 14:12

zzzeek requested a review from sqla-tester May 2, 2025 14:14

sqla-tester reviewed May 2, 2025

View reviewed changes

lachaib requested a review from sqla-tester May 2, 2025 16:37

sqla-tester reviewed May 2, 2025

View reviewed changes

zzzeek requested a review from sqla-tester May 7, 2025 15:30

sqla-tester reviewed May 7, 2025

View reviewed changes

Add support of IF [NOT] EXISTS for ADD/DROP COLUMN in Postgresql

82d6319

Fixes sqlalchemy#1626

lachaib force-pushed the lachaib/issue1626 branch from 5391908 to 82d6319 Compare May 13, 2025 07:38

zzzeek requested a review from sqla-tester May 13, 2025 13:05

sqla-tester reviewed May 13, 2025

View reviewed changes

zzzeek requested changes May 15, 2025

View reviewed changes

chore: move handling of if_exists to base.py

c503b04

zzzeek requested a review from sqla-tester May 19, 2025 13:10

sqla-tester reviewed May 19, 2025

View reviewed changes

sqlalchemy-bot closed this in cfc9de8 May 19, 2025

Uh oh!

Add support of IF [NOT] EXISTS for ADD/DROP COLUMN in Postgresql #1627

Add support of IF [NOT] EXISTS for ADD/DROP COLUMN in Postgresql #1627

Uh oh!

Conversation

lachaib commented Mar 19, 2025

Description

Checklist

Uh oh!

lachaib commented Mar 31, 2025

Uh oh!

zzzeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lachaib commented Apr 1, 2025

Uh oh!

CaselIT commented Apr 1, 2025

Uh oh!

lachaib commented Apr 30, 2025

Uh oh!

zzzeek commented Apr 30, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Apr 30, 2025

Uh oh!

lachaib commented Apr 30, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Apr 30, 2025

Uh oh!

lachaib commented May 2, 2025

Uh oh!

zzzeek commented May 2, 2025

Uh oh!

zzzeek commented May 2, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented May 2, 2025

Uh oh!

zzzeek commented May 2, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented May 2, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented May 7, 2025

Uh oh!

zzzeek commented May 7, 2025

Uh oh!

lachaib commented May 13, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented May 13, 2025

Uh oh!

sqla-tester commented May 14, 2025