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 behavior for vector types is partially incorrect #13434

Closed
nvanbenschoten opened this issue Feb 6, 2017 · 3 comments
Closed

sql: pgwire behavior for vector types is partially incorrect #13434

nvanbenschoten opened this issue Feb 6, 2017 · 3 comments
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-pgwire pgwire protocol issues. A-sql-semantics C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Milestone

Comments

@nvanbenschoten
Copy link
Member

Sequelize queries various pg_catalog tables to assemble an array of indexes, using the following query:

SELECT i.relname                      AS name, 
       ix.indisprimary                AS PRIMARY, 
       ix.indisunique                 AS unique, 
       ix.indkey                      AS indkey, 
       Array_agg(a.attnum)            AS column_indexes, 
       Array_agg(a.attname)           AS column_names, 
       Pg_get_indexdef(ix.indexrelid) AS definition 
FROM   pg_class t, 
       pg_class i, 
       pg_index ix, 
       pg_attribute a 
WHERE  t.oid = ix.indrelid 
       AND i.oid = ix.indexrelid 
       AND a.attrelid = t.oid 
       AND t.relkind = 'r' 
       AND t.relname = 'user' 
GROUP  BY i.relname, 
          ix.indexrelid, 
          ix.indisprimary, 
          ix.indisunique, 
          ix.indkey 
ORDER  BY i.relname; 

Currently, the pg_index.indkey is being returned as an int[] instead of an int2vector, and these have different encodings. The former is an array, while the latter is a space-delimited string. As a result, Sequelize fails to parse the field. We should fix pg_index to return the correct type.

A straightforward approach to this would be to add a new OID wrapper type around DArray for this specific instance. We'd also need to allow OID Wrappers to use custom wire encodings when necessary.

On the other hand, it looks like "vector" types are common in pg_catalog tables. For instance, pg_index alone has 4 different fields with 2 different vector types. We may want to look into a more general approach to these types, instead of defaulting to arrays for each instance until we find that they break things.

@nvanbenschoten nvanbenschoten added the A-sql-pgcompat Semantic compatibility with PostgreSQL label Feb 6, 2017
@nvanbenschoten
Copy link
Member Author

First discussed in #12207.

@cuongdo cuongdo mentioned this issue Feb 6, 2017
14 tasks
@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-semantics labels Feb 10, 2017
@cuongdo
Copy link
Contributor

cuongdo commented Feb 22, 2017

Partly resolved by #13484, at least for pg_index.indkey. There are other vector columns that haven't been changed, so I'm leaving this open.

@cuongdo cuongdo added this to the Later milestone Feb 22, 2017
@knz knz added the A-sql-pgwire pgwire protocol issues. label Apr 27, 2018
@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. and removed C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels May 3, 2018
@jordanlewis
Copy link
Member

This is fixed.

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. A-sql-semantics C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

4 participants