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

wait for postgres, possibly fixes https://github.com/matrix-discord/mx-puppet-discord/issues/78 #64

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

erulabs
Copy link

@erulabs erulabs commented Nov 11, 2020

Hello!

This PR ensures that we always wait for postgres to be connected before we continue on in the code - this prevents a problem where getSchemaVersion fails, and then returns 0 as the schema version, which causes some thrashing of the database :( - this ensures we never run getSchemaVersion() until we know that query will fail for non-connection reasons. Rebooting a machine is a good example where postgres and the bridge will both start at the same time, which can lead to this issue.

I'm still testing to ensure that this 100% resolves https://github.com/matrix-discord/mx-puppet-discord/issues/78 (I think it will), but at any rate it's probably a good idea to ensure PG is ready before running :)

Thanks!

@erulabs erulabs changed the title wait for postgres, possibly fixes #78 wait for postgres, possibly fixes https://github.com/matrix-discord/mx-puppet-discord/issues/78 Nov 11, 2020
@Sorunome
Copy link
Owner

Thanks for the PR, lint fails though (as seen at the failing CI) with

/home/travis/build/Sorunome/mx-puppet-bridge/src/store.ts:205:4
ERROR: 205:4 await-promise Invalid 'await' of a non-Promise value.

src/db/postgres.ts Outdated Show resolved Hide resolved
@erulabs
Copy link
Author

erulabs commented Jan 13, 2021

sqlite doesn't return a promise for db.Open() so I went ahead and added a Promise.resolve() around db.Open in store such that it is always a promise, even if it isn't. Should fix the typescript lint (and probably also fix sqlite)

I've only tested this with postgres as in my setup we're always using psql, and never using sqlite. That said, I can confirm this does resolve a number of the schema issues!

@Dessix
Copy link

Dessix commented Jan 13, 2021

The multi-layer promise unpacking is so unintuitive, but ... Yeah, that hack should work.

@Sorunome
Copy link
Owner

The CI says the unit tests are failing

// Hide username:password
const logConnString = this.connectionString.substr(
this.connectionString.indexOf("@") || 0,
);
log.info(`Opening ${logConnString}`);
this.db = pgp(this.connectionString);
// Wait for postgres to be ready by returning a promise for a connection
return this.db.connect();
Copy link

Choose a reason for hiding this comment

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

Changing this return to await fixes the compilation error:

Suggested change
return this.db.connect();
await this.db.connect();

@Dessix
Copy link

Dessix commented Aug 20, 2021

If you'd like, I've rebased this changeset to the latest upstream HEAD, applied the compilation fix I commented above, and pushed it to Dessix/mx-puppet-bridge/postgres-wait.

@Dessix
Copy link

Dessix commented Aug 24, 2021

@erulabs I received a message notification from you on a commit but it doesn't appear to have made it into this PR?

Additionally, my branch doesn't appear to actually succeed- I think it may be a problem with my build or test environments, but once I get to the await for Connect, my Node instance silently exits with status code 0. Can you confirm that your code - when merged with upstream - actually produces correct / expected behaviour? If so, that would indicate that it's actually my environment and I can work to simplify it further.

@dsonck92
Copy link
Collaborator

@Dessix what's the status on your side? Since it does sound like a good feature.

@Dessix
Copy link

Dessix commented Dec 30, 2021

@dsonck92 I had a repro but I couldn't figure out what was leading to Node exiting. Strace and node-debug (and even rr) gave me nothing at the point of failure. After 2 days at it, I ended up deciding to abandon my codebase rather than investigate it any further, and planned to rewrite it in Rust to avoid Node in the future.

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.

Issue updating database from v12 to v13
4 participants