rom-rb / rom-sql

SQL support for rom-rb
https://rom-rb.org
MIT License
217 stars 93 forks source link

Don't automatically discover the database tables #132

Closed TiteiKo closed 7 years ago

TiteiKo commented 7 years ago

ATM, the database tables are automatically discovered. Any table without relation being auto-discovered too.

Is there a way to disable this and only rely on relations?

solnic commented 7 years ago

You can exclude some tables via :not_inferrable_relations connection option which should be set to an array with a list of symbols matching table names ie:

ROM.container(
  :sql, 'postgres://localhost/my_db', not_inferrable_relations: %i[schema_migrations]
)
AMHOL commented 7 years ago

You can also find the table names with corresponding relations using something like: config.relation_classes.select { |relation| relation.adapter == :sql }.map(&:dataset)

TiteiKo commented 7 years ago

My point is to be able to ensure that no request will be send to unused tables, for them to be able to be safely deleted at any time without having even the slightest chance to make a request fail...

Using continuous deployment, you could have the following workflow to delete a table:

Which is sadly not possible ATM. Would an option such as not_inferrable: true be considered or should I just add a third step to the workflow to delete the table in the not_inferrable_relations array?

solnic commented 7 years ago

You can explicitly list which are allowed to be inferred via :inferrable_relations but there's no way to disable it completely, we could add it though.

I have to say I don't entirely understand your use case. When you say "no request" are you referring to queries/statements that rom issues or your own code?

TiteiKo commented 7 years ago

Setting inferrable_relations: [] basically does the trick! Thanks a lot :)


I'll still explain a bit more my use case (as this was written before testing inferrable_relations and I still think it's worth reading and pondering, but feel free to say "nah there's a solution already" and close)


TL;DR: At medium scale (>= 10 app servers, >= 100 tables), the risks that a server will hit a sql error is real when dropping a table

Also:


Full length explanation:

I'm referring to those that rom issues.

When connecting the gateway, first it lists every table in the database.

SELECT "relname" FROM "pg_class" INNER JOIN "pg_namespace" ON ("pg_namespace"."oid" = "pg_class"."relnamespace") WHERE (("relkind" = 'r') AND ("pg_namespace"."nspname" = any(current_schemas(false))));

Then, for all tables that have a relation + non inferred schema, it runs the following query:

SELECT NULL AS "nil" FROM "the_table" LIMIT 1;

Then, for all tables that don't have a relation, or have a relation with inferred schema, it runs the following queries:

SELECT "pg_attribute"."attname" AS "name", CAST("pg_attribute"."atttypid" AS integer) AS "oid", CAST("basetype"."oid" AS integer) AS "base_oid", format_type("basetype"."oid", "pg_type"."typtypmod") AS "db_base_type", format_type("pg_type"."oid", "pg_attribute"."atttypmod") AS "db_type", pg_get_expr("pg_attrdef"."adbin", "pg_class"."oid") AS "default", NOT "pg_attribute"."attnotnull" AS "allow_null", COALESCE(("pg_attribute"."attnum" = ANY("pg_index"."indkey")), false) AS "primary_key" FROM "pg_class" INNER JOIN "pg_attribute" ON ("pg_attribute"."attrelid" = "pg_class"."oid") INNER JOIN "pg_type" ON ("pg_type"."oid" = "pg_attribute"."atttypid") LEFT OUTER JOIN "pg_type" AS "basetype" ON ("basetype"."oid" = "pg_type"."typbasetype") LEFT OUTER JOIN "pg_attrdef" ON (("pg_attrdef"."adrelid" = "pg_class"."oid") AND ("pg_attrdef"."adnum" = "pg_attribute"."attnum")) LEFT OUTER JOIN "pg_index" ON (("pg_index"."indrelid" = "pg_class"."oid") AND ("pg_index"."indisprimary" IS TRUE)) WHERE (("pg_attribute"."attisdropped" IS FALSE) AND ("pg_attribute"."attnum" > 0) AND ("pg_class"."oid" = CAST(CAST('"the_table"' AS regclass) AS oid))) ORDER BY "pg_attribute"."attnum";
SELECT NULL AS "nil" FROM "the_table" LIMIT 1

All those queries currently take almost a second on the DB I'm working with, on my computer.

Let's say you deploy your new version, with the migration to remove table X, on different servers, one of them actually doing the removal.

Server 1 launches the migration while server 2 is booting up the service. Server 2 lists the tables, sees table X, starts to collect attributes of table A, B, ... Server 1 drops table X Server 2 tries to collect attributes of table X and the database then answers "Error: relation "x" does not exist"

At small scale, I do realize it would be bad luck, and I know removing tables does not happen that often.

solnic commented 7 years ago

@TiteiKo aah of course, this makes sense. Thank you so much for this detailed explanation. I guess that's why we prefer explicit vs implicit. I added a bunch of features to rom that I actually don't like at all. This includes table inference & schema inference. Unfortunately that's what ruby folks like to have. It's also the reason why there's an option to fall back to explicit definitions and have full control. It's actually really cool you discovered that passing an empty array as inferrable_relations will disable this (I didn't know it would work like that even though I implemented it ¯_(⊙ʖ⊙)/¯).

I hope that in the future we'll be able to convince people that all this automation is not desirable and we'll be able to remove it. For instance having explicit schema definitions (and explicit relation classes) is a much nicer way of defining your application database backend because you can SEE it. We can even build tools for automatic migrations based on schema definitions (good old DataMapper had that, but it was limited). We have a very solid foundation for this kind of functionality with schemas backed by dry-types.

I'm closing this since it seems like you got it sorted it out :) Thanks again!

TiteiKo commented 7 years ago

Just a last thought @solnic: As the project is not already in v1.0.0 and it's what you would like to push people to, wouldn't it be the right time to switch from defaulting to inferring to defaulting to not inferring? Like adding an infer_tables: true option?

solnic commented 7 years ago

@TiteiKo hmm, it crossed my mind but I decided not to do it in 1.0.0 (and we're about to release v1.0.0.rc1...) so we may want to wait with this until we can release 2.0.0. The reason is purely pragmatic, this (disabling inference by default) is one of the things that most people don't expect to see, so starting with rom-sql would be harder for them.