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

DM-43097: Add autoincrement=false to some columns #204

Merged
merged 1 commit into from Apr 15, 2024

Conversation

andy-slac
Copy link
Contributor

Columns which are primary keys by default have autioncrement=auto in sqlalchemy which for single-column PK enables autoincrement. This does no big harm for APDB schema but creates unnecessary sequences in postgres. To avoid those extra sequences I mark all integer PK columns as autoincrement=false.

@gpdf
Copy link
Collaborator

gpdf commented Apr 12, 2024

Is this the right place for this? Perhaps we should handle this at the Felis-processing level instead, and default to false unless true is explicitly requested in the YAML? @JeremyMcCormick ?

@andy-slac
Copy link
Contributor Author

Is this the right place for this? Perhaps we should handle this at the Felis-processing level instead, and default to false unless true is explicitly requested in the YAML? @JeremyMcCormick ?

From recent felis updates I think we converged on that felis follows sqlalchemy approach, that is autoincrement="auto" option for column constructor by default. I'm not super-happy about the need to specify autoincrement behavior in sdm_schemas. It may be better for sdm_schemas to be pure science data model, and have all backend-specific details in a separate layer (this is more or less what I did for dax_apdb, but for dax_ppdb I'm trying to use felis' native methods).

@JeremyMcCormick
Copy link
Collaborator

JeremyMcCormick commented Apr 12, 2024

Is this the right place for this? Perhaps we should handle this at the Felis-processing level instead, and default to false unless true is explicitly requested in the YAML? @JeremyMcCormick ?

I would approve this for now given that it is only a minor change to four columns.

Revisiting and possibly changing Felis's default behavior for the autoincrement field seems like a separate issue that need not block this update. If we do change autoincrement to False by default, these settings on apdb columns would just mirror the new default value. So it would be a harmless change, anyways, and we could easily revert it later.

I can make a ticket proposing to change the default for autoincrement to False if the consensus is that this would be more reasonable that auto. I would still approve this PR though regardless of what decision we make about a new default.

Columns which are primary keys by default have autioncrement=auto
in sqlalchemy which for single-column PK enables autoincrement.
This does no big harm for APDB schema but creates unnecessary
sequences in postgres. To avoid those extra sequences I mark all
integer PK columns as autoincrement=false.
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now.

We may revisit how autoincrement works in the future within Felis.

@JeremyMcCormick JeremyMcCormick merged commit 68a3f10 into main Apr 15, 2024
4 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-43097 branch April 15, 2024 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants