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

Issue 147 postgres integration #297

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

AcquaDiGiorgio
Copy link

Full integration with PostgreSQL

Used asyncpg as the driver because it is the fastest and one of the most used by the community

Had to change a few aspects that were MySQL specific. Those being unnamed Enums, which are not permitted in Postgres, and the way of returning the last inserted ID, which used result.lastrowid and postgres needs result.inserted_primary_key[0]

@fstagni fstagni linked an issue Sep 16, 2024 that may be closed by this pull request
4 tasks
Comment on lines +61 to +66
SubmissionTime = NullColumn("SubmissionTime", DateTime(timezone=True))
RescheduleTime = NullColumn("RescheduleTime", DateTime(timezone=True))
LastUpdateTime = NullColumn("LastUpdateTime", DateTime(timezone=True))
StartExecTime = NullColumn("StartExecTime", DateTime(timezone=True))
HeartBeatTime = NullColumn("HeartBeatTime", DateTime(timezone=True))
EndExecTime = NullColumn("EndExecTime", DateTime(timezone=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? We are anyway always using UTC.

Copy link
Author

Choose a reason for hiding this comment

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

Is because we are specifying the timezone in datetime.now(tz=timezone.utc).
While this does not throw any errors in MySQL when the table does not accept a timezone, in PostgreSQL stops the execution due to conflicting formatting.

One way of fixing it is just to completely remove the timezone on the datetime.now function, which is not recommended and even RUFF marks it as a warning.
The other way is by telling the column to take into account the timezone, which in my opinion is the better way of implementing it.

MySQL:

  • Changing the table property: 2024-09-26 14:00:32
  • Removing tz=timezone.utc: 2024-09-26 14:05:53

No change, both result in the same date format.

PostgreSQL:

  • Changing the table property: 2024-09-26 13:50:26.213893+00
  • Removing tz=timezone.utc: 2024-09-26 13:55:57.840623

Timezone is or is not added at the end.

@chaen
Copy link
Contributor

chaen commented Oct 18, 2024

thanks a lot for the excellent work.
The big problem with this PR (and with running Postgres) is indeed the timezone awareness of the datetime.

We unfortunately cannot have timezone aware datetime in our DB as long as we use legacy DIRAC, as this would require too much change. This effectively mean that we cannot have Postgress for DBs which come from Legacy DIRAC as long as they are there.

2 options:

  • have two "running modes" for the code, by which we have either the timezone awareness or not, depending on whether we still need to support DIRAC. This could maybe be hidden to some extent in the column type. We would need to asses how much work that is.
  • just accept that we won't support timezone aware timestamp (and hence Postgres) as long as there is legacy DIRAC around.

In any case, there's the priority is quite clear: first port all the services to diracx before trying to change any db schemas (which hopefully will just be an alambic job by then).
The question is whether we want to try to implement these 2 running mode in the meantime.
I am not sure whether its worth it, but it needs investigations.

@AcquaDiGiorgio just to make things clear : these problems have nothing to do with the PR you have done ! You've just helped us discovering these problems ! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Try to run the demo and integration tests with Postgress
3 participants