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

Filter bootstrap more efficiently #352

Merged
merged 5 commits into from Jul 16, 2019

Conversation

@fishcakez
Copy link
Member

commented Nov 25, 2017

  • No longer bootstrap records in connect callback to speed up handshake (especially initial connection). There is 1 record per table and many many tables (e.g. 100,000s) could be very slow to bootstrap.
  • No longer bootstrap on reconnections because we can now bootstrap during describe phase of query (2c026cb) and gracefully handle type changes without disconnecting (e1bb16c)
  • During describe phase only bootstrap the types required to describe the query because it is faster and we want to be as responsive as possible - it is better a few short delays then multiple longer ones.

Closes #310.
Closes #320.

lib/postgrex/types.ex Outdated Show resolved Hide resolved
lib/postgrex/protocol.ex Outdated Show resolved Hide resolved
lib/postgrex/protocol.ex Outdated Show resolved Hide resolved

@fishcakez fishcakez force-pushed the jf-filter-bootstrap branch 2 times, most recently from de24f93 to 0a7e7f1 Nov 25, 2017

@fishcakez

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2017

Unfortunately this change would mean we can hit the following line when trying decode an anonymous record:

msg = "oid `#{oid}` was not bootstrapped and lacks type information"
. This could happen because a type did not exists on first connect and has not been explicitly used before appearing in anonymous record. Notably we can already hit this case today because we can not guarantee to have seen a type that appears in an anonymous record!

Technically we can handle this situation because we decode inside a connection now - we did not use to. It would be possible (but I imagine complex) to lookup the missing oids after a results set, then decode the rows from that result. Is this something we would want to do?

test/type_server_test.exs Show resolved Hide resolved
lib/postgrex/types.ex Show resolved Hide resolved
@fishcakez

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2017

@tlvenn could you try this branch with your cockroachdb ecto adapter?

@michalmuskala do you have thoughts on #352 (comment)

Only bootstrap new oids during describe
Carry out full bootstrap on first connect and then fetch new types when
trying to describe a query. This means reconnects don't run a bootstrap
query and describe runs minimal query.

@fishcakez fishcakez force-pushed the jf-filter-bootstrap branch from 0a7e7f1 to 53585e7 Nov 25, 2017

@sourcelevel-bot

This comment has been minimized.

Copy link

commented Nov 25, 2017

Ebert has finished reviewing this Pull Request and has found:

  • 28 possible new issues (including those that may have been commented here).
  • 3 fixed issues! 🎉

But beware that this branch is 1 commit behind the elixir-ecto:master branch, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/postgrex/pulls/352.

@fishcakez

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2017

Unfortunately this PR has the issue that we may not discover sub-types. For example if a new type is added and the super type (such as a composite or array) uses the new type we may not be able to resolve the new super type because we don't know about the sub type.

@fishcakez fishcakez closed this Nov 25, 2017

@michalmuskala

This comment has been minimized.

Copy link
Member

commented Nov 25, 2017

@fishcakez regarding your comment. What if we went the @josevalim way? Do the least we can get away with and wait until somebody comes with a use case.

If it's broken right now anyway and adds a lot of complexity, maybe we should hold off on this. I also understand that this is "fixable" by adding some explicit type casts to the query, is that right?

Handle recursive reloading of types
When bootstrapping unknown oids also check for their subtypes.
@fishcakez

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2017

@michalmuskala what is Jose's way? Please see #353 as it goes into more detail. One could cast to a named composite yes but the trouble is that its possible to discover the issue in production and not tests. We could also have the wrong types for the named composite.

I have fixed this PR to recursively fetch the types.

@fishcakez

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2017

@ericmj could you take a look at this as I believe you are best placed to understand the consequences?

@tlvenn

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2017

@fishcakez As discussed in Slack, I tested the branch with CDB and the bootstrap query still does not work outside of the box unfortunately for just a single remaining reason:

ARRAY (
      SELECT a.atttypid
      FROM pg_attribute AS a
      WHERE a.attrelid = t.typrelid AND a.attnum > 0 AND NOT a.attisdropped
      ORDER BY a.attnum
    )

Which is a correlated subquery which CDB does not support at the moment

cockroachdb/cockroach#3288

@fishcakez

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2018

@ericmj

This comment has been minimized.

Copy link
Member

commented Jan 15, 2018

@fishcakez I don't understand this comment:

Unfortunately this change would mean we can hit the following line when trying decode an anonymous record:
This could happen because a type did not exists on first connect and has not been explicitly used before appearing in anonymous record. Notably we can already hit this case today because we can not guarantee to have seen a type that appears in an anonymous record!

It is my understand that, right now, when decoding composites we should always know the subtypes. This would be a new limitation?

assert :ok = query("CREATE TYPE missing_comp AS (a int, b missing_enum)", [])
assert :ok = query("CREATE TABLE missing_comp_table (a missing_comp)", [])

assert :ok = query("INSERT INTO missing_comp_table VALUES ($1)", [{1, "missing"}])

This comment has been minimized.

Copy link
@ericmj

ericmj Jan 15, 2018

Member

Is this supposed to test for named or anonymous composites? I believe this will be inferred as named since it's inserting directly into a table.

lib/postgrex/protocol.ex Outdated Show resolved Hide resolved

@michalmuskala michalmuskala added this to the v1.0 milestone May 23, 2018

josevalim added some commits Jul 16, 2019

@ericmj

ericmj approved these changes Jul 16, 2019

@josevalim josevalim merged commit fd94dd8 into master Jul 16, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@sumerman

This comment has been minimized.

Copy link

commented Jul 16, 2019

🎉 thank you, folks!

@josevalim josevalim deleted the jf-filter-bootstrap branch Jul 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.