Skip to content

Avoid loading information about table-types at start#320

Closed
sumerman wants to merge 1 commit intoelixir-ecto:masterfrom
adjust:no_tables_on_boostrap
Closed

Avoid loading information about table-types at start#320
sumerman wants to merge 1 commit intoelixir-ecto:masterfrom
adjust:no_tables_on_boostrap

Conversation

@sumerman
Copy link
Copy Markdown

@sumerman sumerman commented Jul 7, 2017

In one of our analytical databases, we have approx. 200k tables, this way we have more than 400k types in the result when you factor in array types automatically created by postgres. Therefore this bootstrap statement runs long enough to cause all other connection processes that are waiting in fetch to exit which, in turn, causes a restart of the pool's supervisor, thus type info is never obtained. While relaxing pool's max_restarts helps to mask the problem the bootstrap query still runs for tens of seconds and obstructs any useful work.

Proposed change avoids loading potentially enormous chunk of data at start, but will load everything if a subsequent bootstrap is triggered (e.g by prepare)

In the case of our app it cuts initial bootstrap runtime from tens of seconds down to ~300ms and subsequent bootstrap queries are avoided.

@sourcelevel-bot
Copy link
Copy Markdown

Hello, @sumerman! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@fishcakez
Copy link
Copy Markdown
Member

Hi @sumerman. That's a lot of tables! AFAIK we had resolved the bootstrapping issue for people with 10s of thousands, rather than 100s of thousands. I can see this being a real problem for you! I am concerned that this approach may delay bootstrapping to more time sensitive parts in other peoples projects. Perhaps we could offer a switch to make this more manageable for people so people could opt into lazy bootstrapping that might limit to the built-in oids on initial bootstrap and then fetch oids as required when a prepare fails to find those required (I think we call it reload). What do you think?

@sumerman
Copy link
Copy Markdown
Author

sumerman commented Jul 8, 2017

It's not a common workload indeed 😄 Current approach only postpones loading of table-types and their array counterparts, those are unlikely to ever hit a wire. (The only case I can think of is SELECT foo() where foo returns a table-type and that's not common at all).

One way to make it strictly better is to always skip table-types unless an OID that caused a reload is of table type. It would retain current behavior, for the most part, yet touch table types, the main source of bloat, only when absolutely necessary.

@fishcakez
Copy link
Copy Markdown
Member

fishcakez commented Jul 8, 2017

@sumerman I think that sounds like a good. I think this also highlights that we could end up with an expensive bootstrap during a query:

{:reload, oid} ->
. In that situation we could improve the situation by just bootstrapping the missing oids for that query (currenty we just find the first missing oid but do a full bootstrap of missing types). If we avoid bootstrapping tables at connect time (bootstrap) and bootstrap just the oids at query time (reload) i think that would be a very neat improvement as we minimise handshaking and add the smallest possible overhead for a query that misses types. Would you like to handle it?

@fishcakez
Copy link
Copy Markdown
Member

@sumerman actually we could resolve #310 by only doing the bootstrap (minus tables) on first connect and then only doing bootstrap in the lazy fashion of reload for required types.

@sumerman
Copy link
Copy Markdown
Author

sumerman commented Jul 8, 2017

@fishcakez I glanced thru protocol.ex, it seems quite intricate, esp. flow-switching between query execution and bootstrap via reload (e.g. I don't even understand how a spawned process gets messages from a socket, that is supposed to be controlled by a parent process or why the socket is guaranteed to be passive). Given that the change you are proposing seems to imply some refactoring of what was done in #317 I believe you are better equipped to tackle this. I can try to do this on my own by applying the least invasive change to the existing flow and changing bootstrap to load only certain OIDs if provided.

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.

2 participants