feat: postgres db pool and session implementation

[taddes]

Description

We need to create the Postgres specific Db struct and associated methods. The majority of API methods will be implemented upon this struct, so this is to initialize the main logic so that the traits can be implemented.

This also encompasses additions to the Postgres-specific database pool implementation.

Several of the trait bounds (methods) have to be satisfied, but this stubs them out so that the PgDb has a common interface that is the same as those for MySQL and Spanner.

Doc comments added in shared error.rs crate for clarity, since it is a shared interface for all databases.

Removed dead code annotations for clippy that are no longer needed in Tokenserver.

Closes STOR-399.

[chenba]

Multiple tests are simultaneously trying to run the migrations, causing the tests to fail.

[taddes]

I wouldn't expect most if any tests to work yet until more of the impl is stubbed out. What did you try running? May be that the mutability issue experienced in MySQL is not the case here and I can remove the conn2 = conn here. Let me push that as a change and see if it's different for you.

[chenba]

What did you try running?

cargo test --no-default-features --features=syncstorage-db/postgres --features=tokenserver-db/postgres --features=py_verifier

[taddes]

Yeah, it's still too soon to trust those outputs.

[chenba]

To be clear, I'm not suggesting the tests are failing due to any impl. I'm saying the tests error out on the migrations step, which is enabled in this patch.

[chenba]

I can paste in the error message (and stack trace) if you are not getting the same result in your env.

[taddes]

Sure, or just slack it. I don't get that, though I have the expected could not init db_pool in get_test_state: DbError { kind: Diesel(SqlError { kind: DieselConnection(CouldntSetupConfiguration(DatabaseError(UnableToSendCommand, "invalid connection string: unexpected EOF"))), for example, which is expected since the db config isn't fully set up.

[chenba]

which is expected since the db config isn't fully set up.

It's the same database url as SYNC_TOKENSERVER__DATABASE_URL though. I did not do any additional setup. I just set the same value for SYNC_SYNCSTORAGE__DATABASE_URL.

[chenba]

I learned from @pjenvey that we use RUST_TEST_THREADS=1 for our unit tests (see the test and test_with_coverage make targets). That fixes this error.

[taddes]

Ach, yes, the RUST_TEST_THREADS=1. I have frequently forgotten about those when not using the make targets. Glad that resolves it.

[chenba]

@pjenvey if it can be summarized in a couple sentences, why ?Send?

[taddes]

Let @pjenvey give you his full answer too, but simply put, ?Send negates the need to satisfy the trait bound of Send, with ? being the negation (why not !, I'll never know). Send allows for the sharing of references across threads.

Rust doesn't natively allow for the implementation of async functions as traits, which is why importing async_trait and using the macro in trait and their implementation blocks is a way to get around that. Perfect example is all our Db traits. They all implement the same Db trait signature, so this is a way to "make" them async.

#[async_trait(?Send)] makes it such that you can avoid needing to make the Send non-threadsafe portion async. While not the clearest read, there's some info in the Rust async_trait crate

[chenba]

Thanks for the write-up. I'm interested in why we don't or can't implement Send in this specific context.

[taddes]

Sure! We don't need or want to implement Send, since it's just a default from async_trait. In this case since we're dealing with a scenario where we'd get errors since the future can't be sent between threads safely, since our database transactions are not thread-safe and have to be executed on a single thread. Since we can't guarantee that, we want to remove the Send bound. IIRC, when we start any db transaction, the object holds a mutable reference back to the original connection.

[pjenvey]

@chenba I almost forgot -- I had written up something about this very question on a recent PR here (albeit more than a few sentences): #1799 (comment)

[pjenvey]

Like what the spanner impl has, let's move this impl Db into a db/db_impl.rs and impl BatchDb into db/batch_impl.rs.

Also we don't need the older style, extra level of redirection here either: the actual impl of these methods should reside in these trait method definitions themselves, not in e.g. PgDb::commit. So the commit/rollback/begin/check/timestamp methods above in PgDb should move into here, and every other stub'd method should be a todo!() for now instead.

Suggested change

	impl Db for PgDb {
	async fn commit(&mut self) -> Result<(), Self::Error> {
	PgDb::commit(self).await
	}
	async fn rollback(&mut self) -> Result<(), Self::Error> {
	PgDb::rollback(self).await
	}
	async fn begin(&mut self, for_write: bool) -> Result<(), Self::Error> {
	PgDb::begin(self, for_write).await
	}
	async fn check(&mut self) -> Result<results::Check, Self::Error> {
	PgDb::check(self).await
	}
	async fn lock_for_read(
	&mut self,
	params: params::LockCollection,
	) -> Result<results::LockCollection, Self::Error> {
	PgDb::lock_for_read(self, params).await
	}
	impl Db for PgDb {
	async fn commit(&mut self) -> Result<(), Self::Error> {
	<impl from above>
	}
	async fn rollback(&mut self) -> Result<(), Self::Error> {
	<impl from above>
	}
	async fn begin(&mut self, for_write: bool) -> Result<(), Self::Error> {
	<impl from above>
	}
	async fn check(&mut self) -> Result<results::Check, Self::Error> {
	<impl from above>
	}
	async fn lock_for_read(
	&mut self,
	params: params::LockCollection,
	) -> Result<results::LockCollection, Self::Error> {
	todo!()
	}

[pjenvey]

FYI I've moved syncstorage-mysql into this style here: #1882

[taddes]

Should we merge this in first @pjenvey and fold into your changes in #1882 or other way around?

[pjenvey]

Sure -- either way should work, I don't think these PRs touch any of the same files (I didn't touch syncstorage-postgres)