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

New Driver Interface #14

Open
dhui opened this issue Mar 13, 2018 · 2 comments
Open

New Driver Interface #14

dhui opened this issue Mar 13, 2018 · 2 comments
Labels
backwards incompatible Change is backwards incompatible help wanted Extra attention is needed

Comments

@dhui
Copy link
Member

dhui commented Mar 13, 2018

Goals:

  1. Configurable timeouts
    • Granularity of configuration? e.g. different timeouts for different operations
  2. No possibility of dangling locks. See: postgres driver should wait for lock #13 (comment)

Possible Goals:

  1. Composability
    • Allow drivers to be composed of another driver(s)
      • maybe expose a methods for getting:
        • the db connection (*sql.Conn or *sql.DB or some interface) - for overwriting migration management queries
        • the config - for shared config
  2. Safety
    • Struct fields should not be exported unnecessarily. e.g many DB driver Config structs have all of their fields exported. This is dangerous since the DB driver has a pointer to the Config, whose exported field values may be changed inadvertently. Another option would be to keep a copy of the Configstruct in the DB driver, but if any of the Config struct fields are pointers, we have the same problem...
      • Add NewConfig() method for creating configs with unexported struct fields.
      • Unexported Config struct fields may make sharing config between composed/dependent drivers harder

Ideas:

  1. Require context.Context as the first param for Driver receiver functions
    1. Which receiver functions should require context?
    2. How does a context timeout/cancel affect the DB state? Is it consistent across all DBs?
    3. Which DBs support context.Context?
    4. Will break DB drivers not explicitly maintained by this repo
  2. Add retry with timeout helper for DBs that don't support acquiring a lock within a timeout
  3. Make the Open method on the Driver interface take a url.URL struct instead of a string
  4. Run each migration in a transaction managed by migrate. See: Enhancement: Wrap each migration file in an atomic transaction #196
  5. Add EnsureVersionTable() method
  6. Add a Metadata() method
  7. Remove Drop() method
  8. Create DB
    • See: Cant seem to create a database from migration when it doesnt exist #347
    • Would require migrating migration storage to it's own DB in a DBMS and tracking the migration status for a DB. e.g. the schema migration table would need to track the latest migration, the db, and if it's dirty
      • To be backwards compatible, we'd need to support migrations for the schema migration table
  9. Use migrate to manage version and metadata table schemas
  10. Return concrete types in DB driver implementation with compile time type checks
@Teddy-Schmitz
Copy link
Contributor

I would at least to start. Only add contexts to the Lock and Unlock functions. Since there is already configuration to have a Timeout but this seems to be very inconsistently applied across the drivers. With mysql, setting its own timeout of 10seconds, and postgres only returning instantly. I haven't checked the others.

@dhui
Copy link
Member Author

dhui commented Mar 23, 2019

Should support for DROP be removed? #193 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible Change is backwards incompatible help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants