fix(replication): add proper handling for pool restarts during replication#915
fix(replication): add proper handling for pool restarts during replication#915
Conversation
| port = addr.port, | ||
| database = %addr.database_name, | ||
| user = %addr.user, | ||
| "pool offline: connection pool shut down" |
There was a problem hiding this comment.
You don't want to log that. This "shutdown" isn't actually shutdown, it just destroys this object. We replace it with a brand new one atomically. This could make people think we are shutting down causing panic :D
There was a problem hiding this comment.
the intention was to add more logs that will help with tracing the similar issues.
I can drop it or change the severity/message
There was a problem hiding this comment.
Yup makes sense. You can set this to trace level for sure.
| // Validate all tables support replication before committing to | ||
| // what can be a multi-hour copy. A table with no primary key or | ||
| // unique replica-identity index cannot be replicated correctly. | ||
| for tables in self.tables.values() { |
There was a problem hiding this comment.
This is great, we should of done it long time ago.
There was a problem hiding this comment.
updated this to show more relevant error and to collect multiple errors
| /// Check that the table supports replication. | ||
| /// | ||
| /// Requires at least one column with a replica identity flag. Tables with | ||
| /// REPLICA IDENTITY FULL or NOTHING have no identity columns and fail here |
There was a problem hiding this comment.
I would double check that. If this is true, we need to have a special query to detect REPLICA IDENTITY FULL and use that as the key.
There was a problem hiding this comment.
it is, the added tests validate that. I'll investigate if we can identify this actually
There was a problem hiding this comment.
so, this will require custom handling in validation and in query generation. Let me actually do this as a separate pr to make it more focused
| /// the copy. Instead, only the cluster reference inside the existing | ||
| /// publisher is updated so that subsequent pool.get() calls target the | ||
| /// live pool rather than a stale, potentially-offline one. | ||
| pub(crate) async fn refresh_before_replicate(&mut self) -> Result<(), Error> { |
There was a problem hiding this comment.
I think we have code to do this already somewhere. If not, we should re-use this function wherever we run these 3 statements.
There was a problem hiding this comment.
yes, there was the usual refresh but it was resetting the publication as well. I refactored this part and added tests, so now there is only one single refresh method
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
meskill
force-pushed
the
fix/replication-pool-shutdown
branch
from
a67a82f to
70adcdc
Compare
| let databases = from_config(&new_config); | ||
|
|
||
| info!( | ||
| "reloading pools from config file: {}", |
There was a problem hiding this comment.
| "reloading pools from config file: {}", | |
| "reloading configuration", |
This will reload users.toml too. Also load above logs this info already.
| use crate::backend::{databases::reload_from_existing, Error}; | ||
|
|
||
| pub(crate) fn schema_changed() -> Result<(), Error> { | ||
| debug!("schema change detected: reloading pools to refresh schema cache"); |
There was a problem hiding this comment.
| debug!("schema change detected: reloading pools to refresh schema cache"); | |
| debug!("schema change detected, refreshing schema cache"); |
| -- that have no primary key and no `REPLICA IDENTITY USING INDEX`. Without a PK | ||
| -- (or unique index promoted via REPLICA IDENTITY USING INDEX), no column carries | ||
| -- the replica identity flag, Table::valid() fails with NoIdentityColumns, and the | ||
| -- table is rejected before the copy starts. See docs/FIX_ISSUE_914.md, Fix 2. |
There was a problem hiding this comment.
I wonder how it worked before...maybe because we never used upserts? But we do actually check that each table has a replica identity, so I'm not sure.
There was a problem hiding this comment.
I think it's because the previous test didn't generate any replication messages to new database to hit the path where we do the table.valid, just a COPY thing only for data.
levkk
left a comment
levkk
left a comment
There was a problem hiding this comment.
LGTM, minor logging changes only
2 participants
No description provided.