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

Bug with binay protocol and arrays #19669

Closed
tlvenn opened this issue Oct 31, 2017 · 10 comments · Fixed by #20461
Closed

Bug with binay protocol and arrays #19669

tlvenn opened this issue Oct 31, 2017 · 10 comments · Fixed by #20461
Assignees
Labels
C-question A question rather than an issue. No code/spec/doc change needed. O-community Originated from the community

Comments

@tlvenn
Copy link
Contributor

tlvenn commented Oct 31, 2017

I am using the following CDB version:

Build Tag:    v1.1.0-rc.1
Build Time:   2017/10/05 15:31:21
Distribution: CCL
Platform:     darwin amd64
Go Version:   go1.8.3
C Compiler:   4.2.1 Compatible Clang 3.8.0 (tags/RELEASE_380/final)
Build SHA-1:  8b865035e21aa4fa526ee017ba9dc685d7af649c
Build Type:   release

Since 1.1 is now supporting arrays, I decided to start using them in my project. I use basic arrays, mainly integer arrays which is what is causing the below issue.

Here is the error that Postgrex (Elixir postgresql driver) is reporting:

** (MatchError) no match of right hand side value: <<48, 110, 116, 101, 114, 118, 97, 108, 95, 105, 110, 116, 118, 110, 116, 118, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0>>
            (postgrex) lib/postgrex/type_module.ex:717: Postgrex.DefaultTypes."Elixir.Postgrex.Extensions.Array"/8
            (postgrex) lib/postgrex/protocol.ex:1982: Postgrex.Protocol.rows_recv/4
            (postgrex) lib/postgrex/protocol.ex:1329: Postgrex.Protocol.execute_recv/5
            (db_connection) lib/db_connection.ex:958: DBConnection.handle/4
            (db_connection) lib/db_connection.ex:1078: DBConnection.describe_run/5
            (db_connection) lib/db_connection.ex:1142: anonymous fn/4 in DBConnection.run_meter/5
            (db_connection) lib/db_connection.ex:1199: DBConnection.run_begin/3
            (db_connection) lib/db_connection.ex:584: DBConnection.prepare_execute/4
            (ecto) lib/ecto/adapters/postgres/connection.ex:68: Ecto.Adapters.Postgres.Connection.prepare_execute/5
            (ecto) lib/ecto/adapters/sql.ex:262: Ecto.Adapters.SQL.sql_call/6
            (ecto) lib/ecto/adapters/sql.ex:429: Ecto.Adapters.SQL.execute_and_cache/7
            (ecto) lib/ecto/repo/queryable.ex:133: Ecto.Repo.Queryable.execute/5
            (ecto) lib/ecto/repo/queryable.ex:37: Ecto.Repo.Queryable.all/4

Postgrex unlike many other drivers is leveraging the binary protocol and not the text protocol. We saw some errors like that in the past that @fishcakez identified and helped troubleshoot. I suspect something similar is happening with arrays.

@fishcakez, do you have any idea what is going on ?

@justinj justinj self-assigned this Oct 31, 2017
@justinj
Copy link
Contributor

justinj commented Oct 31, 2017

Hey @tlvenn, thanks for the bug report! It does look like there's an issue on our end regarding the binary protocol for arrays, I'll investigate and get back to you.

@tlvenn
Copy link
Contributor Author

tlvenn commented Oct 31, 2017

Thanks for the reply @justinj

The 'interesting' part is that sometimes it works...

@fishcakez
Copy link

@tlvenn the crash in postgrex is occurring at https://github.com/elixir-ecto/postgrex/blob/a6c1f1936851101b1fe6c340ab1e79c315b7388a/lib/postgrex/extensions/array.ex#L32. Its expecting some integers followed by the array data but seems to get what looks like an ascii string followed by zeros?

@tlvenn
Copy link
Contributor Author

tlvenn commented Nov 6, 2017

@justinj any update on this issue ? Thanks a lot in advance !

@justinj
Copy link
Contributor

justinj commented Nov 6, 2017

Hey @tlvenn, sorry, have had a lot on my plate but I'm looking into this now! Can you give me any info on what the queries you were using looked like?

@tlvenn
Copy link
Contributor Author

tlvenn commented Nov 7, 2017

Hi @justinj, it's a pretty trivial one:

SELECT a0."id", a0."ref_name", a0."parent_ids", a1."id", a1."lang", a1."name", a1."abbr", a1."activity_category_id" FROM "activity_categories" AS a0 INNER JOIN "activity_category_strings" AS a1 ON a1."activity_category_id" = a0."id"

With the following schema:

CREATE TABLE activity_categories (
	id INT NOT NULL DEFAULT unique_rowid(),
	ref_name STRING NOT NULL,
	parent_ids INT[] NULL,
	CONSTRAINT "primary" PRIMARY KEY (id ASC),
	FAMILY "primary" (id, ref_name, parent_ids)
)

CREATE TABLE activity_category_strings (
	id INT NOT NULL DEFAULT unique_rowid(),
	lang STRING NOT NULL,
	"name" STRING NOT NULL,
	abbr STRING NULL,
	activity_category_id INT NOT NULL,
	CONSTRAINT "primary" PRIMARY KEY (id ASC),
	CONSTRAINT activity_category_strings_activity_category_id_fkey FOREIGN KEY (activity_category_id) REFERENCES activity_categories (id),
	INDEX activity_category_strings_auto_index_activity_category_strings_activity_category_id_fkey (activity_category_id ASC),
	FAMILY "primary" (id, lang, "name", abbr, activity_category_id)
)

@justinj
Copy link
Contributor

justinj commented Nov 7, 2017

hey @tlvenn, I spent some time with some other drivers that support the binary format, I tried node-postgres, libpq, and pgx (here's a reconstruction of your schema in pgx) but they all seemed to work ok, can you give me any more info on

  • when the problem occurs? you mentioned it doesn't happen every time? and
  • if the problem goes away when you try against postgres?

@tlvenn
Copy link
Contributor Author

tlvenn commented Dec 4, 2017

Hi @justinj

I somehow missed your feedback, sorry about that. Right now my usage of arrays is minimal, I use it only in one table which happens to be loaded in full when the application starts. And yes it works 9 times out of 10.

I will try to check against Postgres and report back.

@justinj
Copy link
Contributor

justinj commented Dec 5, 2017

I believe this was caused by the same problem as #20372, and should be fixed by #20461. Sorry for the delayed fix @tlvenn!

@tlvenn
Copy link
Contributor Author

tlvenn commented Dec 5, 2017

Ha great to hear @justinj 🎉

justinj pushed a commit to justinj/cockroach that referenced this issue Dec 5, 2017
Release notes: fix issue with stale buffer data when using the binary
format for arrays.

Fixes cockroachdb#20372.
Fixes cockroachdb#19669.

This commit fixes an issue involving passing a bytes.Buffer by value
which would cause old buffered data for arrays to be re-used.

The bug here was somewhat subtle and had to do with copying a
bytes.Buffer by value whose slice header pointed to its fixed-size array
used for small allocations, and then *re-assigning* the original buffer,
causing the fixed-size array to be overwritten and the buffered value
changed. A reduced version of the issue can be seen here:

https://play.golang.org/p/4-v_AeqYtR
justinj pushed a commit to justinj/cockroach that referenced this issue Dec 5, 2017
Release notes: fix issue with stale buffer data when using the binary
format for arrays.

Fixes cockroachdb#20372.
Fixes cockroachdb#19669.

This commit fixes an issue involving passing a bytes.Buffer by value
which would cause old buffered data for arrays to be re-used.

The bug here was somewhat subtle and had to do with copying a
bytes.Buffer by value whose slice header pointed to its fixed-size array
used for small allocations, and then *re-assigning* the original buffer,
causing the fixed-size array to be overwritten and the buffered value
changed. A reduced version of the issue can be seen here:

https://play.golang.org/p/4-v_AeqYtR
justinj pushed a commit to justinj/cockroach that referenced this issue Jan 31, 2018
Release notes: fix issue with stale buffer data when using the binary
format for arrays.

Fixes cockroachdb#20372.
Fixes cockroachdb#19669.

This commit fixes an issue involving passing a bytes.Buffer by value
which would cause old buffered data for arrays to be re-used.

The bug here was somewhat subtle and had to do with copying a
bytes.Buffer by value whose slice header pointed to its fixed-size array
used for small allocations, and then *re-assigning* the original buffer,
causing the fixed-size array to be overwritten and the buffered value
changed. A reduced version of the issue can be seen here:

https://play.golang.org/p/4-v_AeqYtR
@jordanlewis jordanlewis added C-question A question rather than an issue. No code/spec/doc change needed. O-community Originated from the community and removed O-deprecated-community-questions labels Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-question A question rather than an issue. No code/spec/doc change needed. O-community Originated from the community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants