Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autogeneration causes invalid SQL with some PostGRES serial columns #1504

Open
adamlogan73 opened this issue Jul 11, 2024 · 3 comments
Open

Comments

@adamlogan73
Copy link

adamlogan73 commented Jul 11, 2024

Describe the bug
Auto-generated PostgreSQL code in 1.13.2 contains invalid code causing migrations to fail. This appears to be around Serial columns and appears to be related to the fix implemented in: #1479. I get the log around detecting a serial and omitting it. We have a ton of tables like this, and not all of them are having this behavior. The table definitions haven't changed, and everything works in 1.13.1.

The id columns definitely have a server default set, but all of them do, not just the ones triggering this behavior. I am having trouble figuring out what is different between them. The python definition is the same, and the database definitions appear to be almost identical.

I tried replicating it in a fresh database to generate a minimum code sample, but couldn't replicate the behavior

Expected behavior
Expect no change from previous behavior.

To Reproduce
Table Definition:

class GEO(Base):
    __tablename__ = "GEO"
    id: Mapped[int] = mapped_column(Identity(on_null=True), primary_key=True)
    name: Mapped[str] = mapped_column(String(4), unique=True)
from typing import Sequence, Union

from alembic import op
import sqlalchemy as sa


# revision identifiers, used by Alembic.
revision: str = 'b6001b86cfc9'
down_revision: Union[str, None] = 'd4e45e048b78'
branch_labels: Union[str, Sequence[str], None] = None
depends_on: Union[str, Sequence[str], None] = None


def upgrade() -> None:
    op.alter_column('GEO', 'id',
               existing_type=sa.INTEGER(),
               server_default=sa.Identity(always=False, on_null=True),
               existing_nullable=False,
               autoincrement=True)



def downgrade() -> None:
    op.alter_column('GEO', 'id',
               existing_type=sa.INTEGER(),
               server_default=None,
               existing_nullable=False,
               autoincrement=True)

Error

Generated SQL it tries to run during migration is: ALTER TABLE "GEO" ALTER COLUMN id;
Which doesn't do anything and isn't valid and throws an error.

Versions.

  • OS: Docker in docker container
  • Python: 3.11
  • Alembic: 1.13.2
  • SQLAlchemy: 2.0.31
  • Database: RDS PostGRES 14.10
  • DBAPI: psycopg2
@adamlogan73 adamlogan73 added the requires triage New issue that requires categorization label Jul 11, 2024
@JabberWocky-22
Copy link

Sorry for the bad experience :(

Serial and identity are not exactly same in postgres. Serial column has default value while identity column not.
In table definition id column use identity, but the log suggest column has default value.
Maybe there is mismatch in python definition and database tables.
You can check identity_generation and column_default from information_schema.columns for more infomations.

@zzzeek
Copy link
Member

zzzeek commented Sep 1, 2024

@CaselIT any thoughts on this?

@CaselIT
Copy link
Member

CaselIT commented Sep 3, 2024

seems like a regression due to the mentioned issue.
we likely need to better distinguish serial vs identity.

@CaselIT CaselIT added bug Something isn't working autogenerate - detection postgresql regression and removed requires triage New issue that requires categorization labels Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants