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

Keep number of concurrently open files O(1) and low enough even for macOS #172

Closed
mzabani opened this issue Feb 16, 2024 · 1 comment
Closed

Comments

@mzabani
Copy link
Owner

mzabani commented Feb 16, 2024

The maximum number of simultaneously open files in macOS has a very low default. To the point where it can make codd barf just by having a few hundred migrations to apply. Even if this can be fixed with ulimit, it seems sensible for codd to avoid this in the first place, as it's possible without great effort, and who knows which environments out there might have similarly stringent limits.

Codd should open one file at a time, closing the previous file before opening the next (codd already does that for on-disk representations, but not for migrations).
We should be able to do this in a single pass, i.e. no need to read each file twice. But let's assume a nasty case where some migration is invalid. By the time codd finds out it may have applied and even committed other migrations. Codd can parse even invalid SQL, but it cannot accept invalid -- codd top-level comments, and for good reason: we don't want to silently treat something as a regular comment just because the user mistyped what they intended to be a special codd directive in the first place.
One argument against adding two passes is that such migrations should not make it into the migration folders because codd add would flag them.
In the end, we should probably go with whatever is easier to implement.

It would also be nice to test this. Maybe we mock file operations? Maybe we strace and ensure no two .sql files are open at the same time? The latter would be fool-proof and wouldn't complicate the codebase, and it could also be used to ensure the same applies to on-disk representations, so it is tempting..

@mzabani
Copy link
Owner Author

mzabani commented Feb 23, 2024

This was fixed by #173, and it's actually nice that migration headers are parsed once to detect errors, and then parsed again when necessary to be applied.

@mzabani mzabani closed this as completed Feb 23, 2024
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

No branches or pull requests

1 participant