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

sql/pgwire: Elixir Connection Issues - Postgrex Protocol Errors - First Test #5582

Closed
Linicks opened this issue Mar 25, 2016 · 75 comments
Closed
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-pgwire pgwire protocol issues. C-question A question rather than an issue. No code/spec/doc change needed. meta-issue Contains a list of several other issues. O-community Originated from the community

Comments

@Linicks
Copy link
Contributor

Linicks commented Mar 25, 2016

All,
When trying to connect to CockroachDB with the Elixir Postgresql driver "Postgrex" (https://github.com/ericmj/postgrex) I get some Errors related to the Postgresql protocol. I'm guessing that there may be some missing pieces in the CockroachDB implementation of the Postgresql protocol that Postgrex is looking for. Here are some details from this initial connection test:


Build Vers: go1.6
Build Tag: alpha.v1-1423-ga8c1cb4
Build Time: 2016/03/25 18:03:44


Example Error Messages:

16:03:46.617 [error] GenServer #PID<0.3938.0> terminating
** (Postgrex.Error) ERROR (internal_error): syntax error at or near "("
SELECT t.oid, t.typname, t.typsend, t.typreceive, t.typoutput, t.typinput,
t.typelem, coalesce(r.rngsubtype, 0), ARRAY (
^

(postgrex) lib/postgrex/protocol.ex:87: Postgrex.Protocol.disconnect/2
(postgrex) lib/postgrex/protocol.ex:79: Postgrex.Protocol.connected/1
(db_connection) lib/db_connection/connection.ex:114: DBConnection.Connection.connect/2
(connection) lib/connection.ex:623: Connection.enter_connect/5
(stdlib) proc_lib.erl:240: :proc_lib.init_p_do_apply/3

Last message: nil
State: nil

16:03:46.619 [error] GenServer #PID<0.3939.0> terminating
** (Postgrex.Error) ERROR (internal_error): syntax error at or near "("
SELECT t.oid, t.typname, t.typsend, t.typreceive, t.typoutput, t.typinput,
t.typelem, coalesce(r.rngsubtype, 0), ARRAY (
^

(postgrex) lib/postgrex/protocol.ex:87: Postgrex.Protocol.disconnect/2
(postgrex) lib/postgrex/protocol.ex:79: Postgrex.Protocol.connected/1
(db_connection) lib/db_connection/connection.ex:114: DBConnection.Connection.connect/2
(connection) lib/connection.ex:623: Connection.enter_connect/5
(stdlib) proc_lib.erl:240: :proc_lib.init_p_do_apply/3

Last message: nil
State: nil

-- Nick

@tamird
Copy link
Contributor

tamird commented Mar 25, 2016

Seems like we don't quite see the whole query, but we do see ARRAY - AFAIK we don't support that yet.

@Linicks
Copy link
Contributor Author

Linicks commented Mar 25, 2016

"ARRAY - AFAIK we don't support that yet"

I just noticed issue #2115, after you mentioned the lack of ARRAY support. I will try again once it's closed.

@petermattis petermattis changed the title Elixir Connection Issues - Postgrex Protocol Errors - First Test sql/pgwire: Elixir Connection Issues - Postgrex Protocol Errors - First Test Mar 31, 2016
@jrimmer
Copy link

jrimmer commented Apr 1, 2016

I tried a similar series of tests but using Phoenix Framework hence Ecto w/Postgrex as a starting point. The initial database create failed upon an ENCODING declaration thusly:
** (Mix) The database for HelloCockroachdb.Repo couldn't be created, reason given: ERROR: XX000: syntax error at or near "ENCODING" CREATE DATABASE "hello_cockroachdb_dev" ENCODING='UTF8'

I proceeded to manually create the database but upon creation of a basic model encountered an error related to the lack of ARRAY support mentioned here upon migration:

** (Postgrex.Error) ERROR (internal_error): syntax error at or near "(" SELECT t.oid, t.typname, t.typsend, t.typreceive, t.typoutput, t.typinput, t.typelem, coalesce(r.rngsubtype, 0), ARRAY (

I acknowledge this is more of an FYI at this point.

@tlvenn
Copy link
Contributor

tlvenn commented Jul 29, 2016

We would very much like to be able to use Cockroach with Elixir but it seems that for the driver (postgrex) to support it, the following issues would need to be resolved:

  • Ignores closing prepared query protocol packets
  • Savepoints aren't supported and when followed by a sync message only returns an error message and not the expected ready message
  • Sends errors messages with code internal_error when postgreSQL would return a more specific error code

Source: elixir-ecto/postgrex#213

Is there any chance the above could be solved to help out on the elixir side ?

@bdarnell
Copy link
Contributor

I thought we already supported the "close" packet for prepared statements. Can you provide more detail about what's not working for you?

There are no plans to support savepoints (beyond the limited form that we currently support for transaction retries).

For error codes, we're unlikely to ever be 100% compatible with postgres in this respect, but if you can identify specific error codes that you check for we can prioritize supporting those cases.

@fishcakez
Copy link

@bdarnell I am using beta-20160728 @ 2016/07/28 17:02:34 (go1.6.3)

A Close is sent to cockroach but a CloseComplete is never received. There are at least three situations where this is occurring for us:

  • Close Sync, where the first received message is ReadyForQuery.
  • Close Parse Describe Flush, where the first received message is ParseComplete
  • Parse Describe Flush Close Sync, where the next received message after the description is ReadyForQuery

The issue relating to savepoint error handling was a bug in the client, not cockroach. However there is a surprise that a savepoint error does not cause the transaction to enter the failed status.

The different error codes were causing some test failures.

There is also an issue with numeric values. The DataRow message from SELECT 42::numeric has 1 column value in length. However it will include 2 empty column values \x00\x00 after the numeric column value.

@tlvenn
Copy link
Contributor

tlvenn commented Jul 31, 2016

Regarding matched error codes in postgrex test files, here is my finding:
All postgres error codes are loaded from pg source code and mapped to given atom

https://github.com/elixir-ecto/postgrex/blob/master/lib/postgrex/errcodes.txt

As far as test goes, I have ignored error code matched in error_code_test.exs given it's to test the mapping logic between int error codes and atoms.

Only 8 error codes are matched in the tests as follow:

error code tests
feature_not_supported alter_test
invalid_catalog_name login_test
syntax_error query_test
invalid_text_representation query_test, transaction_test
unique_violation query_test, transaction_test
in_failed_sql_transaction query_test, transaction_test
query_canceled query_test
invalid_savepoint_specification transaction_test

@bdarnell
Copy link
Contributor

Thanks, we'll look into these.

@knz
Copy link
Contributor

knz commented Aug 3, 2016

I have created separate issues to track your various findings.

Regarding the error codes, what do you propose would work the best for you? As Ben explained earlier we're not keen on matching pg one-for-one, but what would be the next best thing?

@fishcakez
Copy link

@knz could we revisit this question after #8296 because is preventing most of our test suite from running?

@knz
Copy link
Contributor

knz commented Aug 3, 2016

Ok, let's do that

@fishcakez
Copy link

fishcakez commented Aug 3, 2016

Fixing #8296 gets us down to 151 failures (from 213). A lot of these failures are because of unsupported types, tables, functions, SAVEPOINT, COPY, different error codes or #8298. There are only two cases I can think of where error codes really matter to us but the tests fail beforehand because of type errors so I will investigate those shortly (our tests like int4).

The remaining failures are:

  • The RowDescription when describing the query SELECT NULL gives a single column oid 0 but there isn't an oid of 0 postgresql will give 705 (unknown).
  • An empty query will fail with an error, postgresql handles this with an EmptyQueryResponse message, instead of CommandComplete. This may not matter to cockroach.
  • Float decoding NaN::float8 (this is an error in our driver as the data is a valid NaN encoded float). This is a bug in the client and not cockroach.

@knz
Copy link
Contributor

knz commented Sep 4, 2016

Hi @fishcakez we've addressed #8298 and I have filed the remaining issues that you found. Is there anything else you've found since then?

@tlvenn
Copy link
Contributor

tlvenn commented Oct 14, 2016

@fishcakez any chance to give it another try now that pretty much all issues have been closed and a new beta has been released ? I wonder if the following from changelog can help a bit as well:

Added new information_schema metatables as well as initial support for the pg_catalog database.

@fishcakez
Copy link

I will try to catch up with this again soon.

@tlvenn
Copy link
Contributor

tlvenn commented Nov 16, 2016

I had a little chat with @fishcakez a few days back and it appears there are still too many issues using the wire protocol/SQL to really make Postgrex working with CockroachDB :(

So the futur of Elixir/Ecto with CockroachDB does not look too bright right now, it seems it will take some substantial efforts to get something working.

It's pretty sad, was very eager to use CockroachDB in my new project but it looks like I have no other choice but to stick with Postgres for the time being.

@knz
Copy link
Contributor

knz commented Nov 16, 2016

It would really help if you could still give us an overview of what you've run into this time.
"Substantial efforts" is exactly the kind of things we're willing to spend on compatibility!

@tlvenn
Copy link
Contributor

tlvenn commented Nov 16, 2016

@fishcakez care to elaborate a little bit on the subject please ?

@fishcakez
Copy link

@knz the most significant barrier is lack of pg_type, pg_attribute and savepoints. I think without these postgrex won't support cockroachdb, especially savepoints. I know this isn't what you want to hear and I hope you understand that our incentives do not align which is unfortunate. Postgrex has existing API around savepoints, which will not work with cockroachdb. This means it is awkward for us to support it because our existing API won't work. This also means I am not inclined to merge changes to postgrex to provide partial or incremental support for cockroachdb when we aren't going to support it officially.

Also Postgrex is very tied to Ecto (like an ORM but without objects), it is even maintained by the same group of people. The feature set of postgrex is based on Ecto and we only add features to postgrex that we want in Ecto. This leads to a very opinionated client, for example we only support extended queries. To run Ecto's integration test suite or to use Ecto's testing tools for user projects requires the savepoint API. This means we can't support cockroachdb in the main project that determines postgrex's features either.

A postgrex connection uses the pg_type and pg_attribute tables to determine how to encode/decode types and there is a system for custom decoders to be used based on the values from these tables. If the tables don't exist then we can't start a connection. It might be that you don't support the SQL query we run against these tables, it is approximately:

    SELECT t.oid, t.typname, t.typsend, t.typreceive, t.typoutput, t.typinput,
           t.typelem, 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
    )
    FROM pg_type as t
    WHERE t.oid NOT IN (SELECT unnest(ARRAY[1,2]))"

I no longer have my branch of postgrex that could run the postgrex test suite against cockroachdb. If we try to run master (or any recent tag) we get an error setting up the tests:

ERROR:  syntax error at or near "TEMPLATE"
CREATE DATABASE postgrex_test TEMPLATE=template0 ENCODING='UTF8' LC_COLLATE='en_US.UTF-8' LC_CTYPE='en_US.UTF-8';

I am sorry that I have quite the opposite of "substantial effort" to help you. I reported the previous issues because I was testing postgrex against cockroachdb to discover issues in postgrex. My advise to anyone wanting to use cockroachdb is to fork postgrex and remove features until it works or use a different client. There are numerous Erlang ones.

@yonkeltron
Copy link

Can confirm that I get problems with a fresh Phoenix app using stock Ecto and stock Postgrex.

Error:

** (Mix) The database for Cockroach.Repo couldn't be created: ERROR XX000 (internal_error): source name "t" not found in FROM clause

15:04:56.418 [error] GenServer #PID<0.3204.0> terminating
** (Postgrex.Error) ERROR XX000 (internal_error): source name "t" not found in FROM clause
    (db_connection) lib/db_connection/connection.ex:148: DBConnection.Connection.connect/2
    (connection) lib/connection.ex:622: Connection.enter_connect/5
    (stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3
Last message: nil
State: Postgrex.Protocol

Elixir-side versions:

  defp deps do
    [{:phoenix, "~> 1.2.4"},
     {:phoenix_pubsub, "~> 1.0"},
     {:phoenix_ecto, "~> 3.0"},
     {:postgrex, ">= 0.0.0"},
     {:phoenix_html, "~> 2.6"},
     {:phoenix_live_reload, "~> 1.0", only: :dev},
     {:gettext, "~> 0.11"},
     {:cowboy, "~> 1.0"}]
  end

Cockroach-side versions:

$ cockroach version
cockroach version
Build Tag:    v1.0.2
Build Time:   2017/06/15 15:11:55
Distribution: CCL
Platform:     darwin amd64
Go Version:   go1.8.3
C Compiler:   4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)
Build SHA-1:  9e3606bd2863ce7a460fd0c842414673d62f0533
Build Type:   development

@tamird
Copy link
Contributor

tamird commented Jun 30, 2017

Is it possible to get the query that ecto is trying to execute?

@yonkeltron
Copy link

Not sure of the query but I get it when running mix ecto.create and am unable to get even past that!

@tlvenn
Copy link
Contributor

tlvenn commented Jul 3, 2017

@yonkeltron you have to use my Postgrex fork for the time being

https://github.com/jumpn/postgrex

@fire
Copy link

fire commented Jul 22, 2017

@tlvenn Can you make it a proper postgrex fork with a name change?

@jordanlewis
Copy link
Member

We should sync up about this again - we've got proper UUID support now, and #12526/#14556 are solved as far as I can tell. What's remaining?

@yonkeltron
Copy link

Is this ready to be tested again with Ecto? Happy to spin up a fresh app and give it a shot.

@tlvenn
Copy link
Contributor

tlvenn commented Sep 21, 2017

@fire this is finally done, I have published the fork at:

https://hexdocs.pm/postgrex_cdb

And to run your test, given CDB does not support arbitrary savepoints, you cant use the sandbox that is bundled with Ecto but fear not, I have created EctoReplaySandbox to leverage a log replay approach to do it:

https://hexdocs.pm/ecto_replay_sandbox

For reference @jordanlewis , the only remaining issue so that we would not need my postgrex fork is support for correlated subqueries which is tracked here: #3288

Happy hacking and testing ;)

@cohawk
Copy link

cohawk commented Dec 4, 2017

@tlvenn et all,

Thank you for your work on this CockroachDB fork of the postgrex adapter.

Over the weekend I spun up a new Elixir :ecto, "2.2.7" project that I was able to get working with CockroachDB and this adapter fork - with some caveats I wanted to share to help others:

https://gist.github.com/cohawk/df29c1c54abd858dd19d8327e862822a

@knz knz added this to Backlog in (DEPRECATED) SQL Front-end, Lang & Semantics via automation Apr 24, 2018
@knz knz added O-community Originated from the community meta-issue Contains a list of several other issues. and removed O-community-questions labels Apr 24, 2018
@jordanlewis jordanlewis added the C-question A question rather than an issue. No code/spec/doc change needed. label Apr 27, 2018
@knz knz added the A-sql-pgwire pgwire protocol issues. label Apr 27, 2018
@knz knz moved this from Triage to ORM/Tool Compat in (DEPRECATED) SQL Front-end, Lang & Semantics May 4, 2018
@petermattis petermattis removed this from the Later milestone Oct 5, 2018
@knz knz moved this from ORM/Tool Compat Meta-issues to Feature requests / pie-in-the-skie in (DEPRECATED) SQL Front-end, Lang & Semantics Oct 23, 2018
@BramGruneir
Copy link
Member

I just wanted to update this issue a bit.

@vilterp and I tried running an example app with Ecto and it looks like we've made significant progress here.

We did run into #32917 during the migration step. So that will be a bit of a blocker. However, once we hacked around that in cockroach, the app actually ran.
I'd really like to be able to run an Ecto or Postgrex test suite against cockroach, but I haven't been able to find the appropriate tests that run against a real DB yet.
So there is still more work to be done here, but we're significantly closer.

@tchoutri
Copy link

Hi @BramGruneir! Any update on this?

@fire
Copy link

fire commented Feb 25, 2019

@fishcakez Are you able to provide instructions, the test suite is a bit convoluted to run and the documentation is lacking. There's two suites, one is the sql syntax and the other is an ecto adapter on a real database.

@fishcakez
Copy link

MIX_ENV=pg mix test

@jordanlewis
Copy link
Member

I'm going to close this in favor of #33441.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-pgwire pgwire protocol issues. C-question A question rather than an issue. No code/spec/doc change needed. meta-issue Contains a list of several other issues. O-community Originated from the community
Projects
No open projects
(DEPRECATED) SQL Front-end, Lang & Se...
  
Feature requests / pie-in-the-skie
Development

No branches or pull requests