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: support Hibernate's use of _pg_expandarray #16971

Closed
11 tasks done
justinj opened this issue Jul 10, 2017 · 21 comments · Fixed by #26628
Closed
11 tasks done

sql: support Hibernate's use of _pg_expandarray #16971

justinj opened this issue Jul 10, 2017 · 21 comments · Fixed by #26628
Assignees
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-semantics C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs
Milestone

Comments

@justinj
Copy link
Contributor

justinj commented Jul 10, 2017

Adding an index to a table in Hibernate issues a query which makes use of this. Note that supporting this also means that we would need to support record types. The query below should work, if this is implemented:

SELECT (i.keys).n FROM (SELECT information_schema._pg_expandarray(indkey) AS keys FROM pg_index) AS i;

This function takes an array a and returns a set of records that looks like the following:

 x   |  n
----------
a[1] |  1
a[2] |  2
a[3] |  3
a[4] |  4
...

The work is to be decomposed as follows:

@awoods187 awoods187 added the A-sql-pgcompat Semantic compatibility with PostgreSQL label Oct 17, 2017
@petermattis
Copy link
Collaborator

@justinj Supporting record types looks to be a sizable project. Is that accurate? Or can this somehow fallout of the existing system easily?

Cc @knz

@justinj
Copy link
Contributor Author

justinj commented Oct 17, 2017

I haven't investigated too deeply, but if we only support the result type then I suspect it's not too bad. My guess would be that the most difficult part would probably be handling the . operator in a way that was insulated from name resolution.

@knz
Copy link
Contributor

knz commented Oct 17, 2017

It's definitely a mid-size project, and somewhat larger than implementing arrays as result-value type.

@jordanlewis
Copy link
Member

I just realized that this is more difficult than we thought. Not only do we need to support (or at least pretend to support) record types, we also need to support generator functions in render positions, which we currently don't support at all.

See #13156 for more context.

@jordanlewis
Copy link
Member

To be more clear, this is in all reality blocked on correlated subquery support as well!

@knz
Copy link
Contributor

knz commented Feb 28, 2018

Looking at this briefly on @awoods187's request. Resolution here requires support for record types in in-memory values (datums or whatever will replace them), not db encoding for them (i.e. we don't need to store them). Analogous to the same distinction we used when array support was first implemented.

@petermattis
Copy link
Collaborator

@jordanlewis Is the query that hibernate issues identical to the use of _pg_expandarray on other ORMs? Or are there minor syntactic differences. I'm wondering if we can do something even simpler (re: more hacky) than in-memory support for the record type.

@jordanlewis
Copy link
Member

They're not all identical. Sometimes they occur in the context of larger queries. Here's another example:

        SELECT
            indexrelid,
            (information_schema._pg_expandarray(indclass)).x AS operator_argument_type_oid,
            (information_schema._pg_expandarray(indclass)).n AS operator_argument_position
        FROM
            pg_index

The only thing we really need to support for this "indexing into" the _pg_expandarray function with the .x syntax, for the x and n fields.

@knz
Copy link
Contributor

knz commented Mar 31, 2018

Bram and I had an offline discussion and Bram discovered this item has two aspects to it:

  • implementing _pg_expandarray. Surprisingly (to me) this is merely a SRF, like the others we already support.
  • implementing the notation E.x when E is a SRF call; this is surprisingly simply a notation to access the column x in the table produced by migrating E to the FROM position. This is a natural extension of the x.y column notation and has nothing to do with record types. (I had a "mind blown" moment when I realized the consequences of Bram's discovery)

@knz
Copy link
Contributor

knz commented Mar 31, 2018

@awoods187 this does mean this item is much easier to implement than what we initially thought, it downgrades it from M-L to S.

@awoods187
Copy link
Contributor

I'll update to a S and am excited to see the results.

@BramGruneir
Copy link
Member

BramGruneir commented Apr 2, 2018

I just wanted to add a bit here, there are 3 parts that are required to support the statement above and
be able to close this issue.

  • sql: Add the builtin function _pg_expandarray() #24422 Implement the information_schema._pg_expandarray as a Set Returning Function (SRF).
  • Add the ability to use the . notation to access a table produced from the migration of a SRF to the FROM position. See @knz's comment above for more details.
  • Fix the name resolution to handle SELECT unnest(indkey) FROM pg_index;.

@jordanlewis
Copy link
Member

pg_index.indkey is already implemented - was there something else you needed from it?

@BramGruneir
Copy link
Member

BramGruneir commented Apr 2, 2018

Hmm...

This works:

+---------+
| indkey  |
+---------+
| 1 2     |
|       1 |
|       1 |
|       1 |
|       1 |
| 1 2 4 3 |
| 1 6     |
| 1 7     |
|       1 |
|       1 |
| 2 3     |
|       1 |
|       5 |
|       4 |
| 1 2     |
| 1 2     |
| 1 2     |
|       1 |
|       2 |
+---------+
(19 rows)

But this doesn't (in my branch with _pg_expandarray working):

SELECT information_schema._pg_expandarray(indkey) FROM pg_index;
pq: column name "indkey" not found

And even this doesn't work:

SELECT unnest(indkey) FROM pg_index;
pq: column name "indkey" not found

@jordanlewis
Copy link
Member

Yeah, our name resolution can't handle this case at the moment.

@BramGruneir
Copy link
Member

Ok, I'll update that checklist for this case.

BramGruneir added a commit to BramGruneir/cockroach that referenced this issue Apr 2, 2018
This adds the information_schema._pg_expandarray() function. It is needed for
compatibility with a number of ORMs. See cockroachdb#16971.

information_schema._pg_expandarray takes an array and returns a set of the
value and an index into the array. This is a very old function and from what I
can disern, was designed for internal use only, but was picked up by a few
ORMs. There is no real supporting documentation for the feature. The code for
it can be seen here:
https://sourcegraph.com/github.com/postgres/postgres/-/blob/src/backend/catalog/information_schema.sql#L43:17

Furthermore, if the function is a Set Retruning Function, it returns tuples
when evaluated in a scaler context:

In Postgres:

```sql
SELECT information_schema._pg_expandarray(array['i','i','o','t','b']);

 _pg_expandarray
-----------------
 (i,1)
 (i,2)
 (o,3)
 (t,4)
 (b,5)
(5 rows)
```

With this patch Cockroach now supports this directly as well:

```sql
SELECT information_schema._pg_expandarray(array['i','i','o','t','b']);

+------------------------------------+
| information_schema._pg_expandarray |
+------------------------------------+
| ('i',1)                            |
| ('i',2)                            |
| ('o',3)                            |
| ('t',4)                            |
| ('b',5)                            |
+------------------------------------+
(5 rows)
```

However, when evaluating an SRF in the `FROM` context, it should return
columns. Luckily, we already do exactly that. This is true for `unnest()` as
well as the new `_pg_expandarray`. The difference is a little subtle and I
couldn't find any good documentation on it, but this answer seems to cover it
quite well:
https://dba.stackexchange.com/questions/172463/understanding-set-returning-function-srf-in-the-select-list

In Postgres:

```sql
SELECT * FROM information_schema._pg_expandarray(array['i','i','o','t','b']);

 x | n
---+---
 i | 1
 i | 2
 o | 3
 t | 4
 b | 5
(5 rows)

```

In Cockroach:

```sql
SELECT * from information_schema._pg_expandarray(array['i','i','o','t','b']);

+---+---+
| x | n |
+---+---+
| i | 1 |
| i | 2 |
| o | 3 |
| t | 4 |
| b | 5 |
+---+---+
(5 rows)
```

Note that after froming the resulting set to a table, this function is
essentially the same as:

```sql
SELECT * from unnest(array['i','1','3','r']) with ordinality as c(x,n);

+---+---+
| x | n |
+---+---+
| i | 1 |
| 1 | 2 |
| 3 | 3 |
| r | 4 |
+---+---+
(4 rows)
```

But to retain direct compatibility with Postgres, the original SET response
needs to be maintained as well.

Part of cockroachdb#16971.

Release note (sql change): Added support for the
information_schema._pg_expandarray() function.
@knz
Copy link
Contributor

knz commented Jun 12, 2018

Another test query from metabase/metabase#5050

SELECT NULL AS TABLE_CAT, 
n.nspname AS TABLE_SCHEM, 
ct.relname AS TABLE_NAME, 
a.attname AS COLUMN_NAME, 
(i.keys).n AS KEY_SEQ, 
ci.relname AS PK_NAME 
FROM pg_catalog.pg_class ct 
JOIN pg_catalog.pg_attribute a ON (ct.oid = a.attrelid) 
JOIN pg_catalog.pg_namespace n ON (ct.relnamespace = n.oid)
 JOIN (SELECT i.indexrelid, i.indrelid, i.indisprimary, information_schema._pg_expandarray(i.indkey) AS keys FROM pg_catalog.pg_index i) i ON (a.attnum = (i.keys).x AND a.attrelid = i.indrelid) 
JOIN pg_catalog.pg_class ci ON (ci.oid = i.indexrelid) 
WHERE true AND ct.relname = 'sometablename' AND i.indisprimary
 ORDER BY table_name, pk_name, key_seq

knz added a commit to knz/cockroach that referenced this issue Jun 12, 2018
This work is yet another step towards cockroachdb#16971.

The labeled tuples introduced in cockroachdb#25283 can now be accessed using their labels.

Star expansion is still not allowed for tuples with fewer or more than
1 columns.

Release note (sql change): Labeled tuples can now be accessed using
their labels (e.g. `SELECT (x).word FROM (SELECT pg_expand_keywords() AS
x)`.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
knz added a commit to knz/cockroach that referenced this issue Jun 13, 2018
This work is yet another step towards cockroachdb#16971.

The labeled tuples introduced in cockroachdb#25283 can now be accessed using their labels.

Star expansion is still not allowed for tuples with fewer or more than
1 columns.

Release note (sql change): Labeled tuples can now be accessed using
their labels (e.g. `SELECT (x).word FROM (SELECT pg_expand_keywords() AS
x)`.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
knz added a commit to knz/cockroach that referenced this issue Jun 13, 2018
This work is yet another step towards cockroachdb#16971.

The labeled tuples introduced in cockroachdb#25283 can now be accessed using
their labels or using a star, e.g. `(E).*`.

Release note (sql change): Labeled tuples can now be accessed using
their labels (e.g. `SELECT (x).word FROM (SELECT pg_expand_keywords()
AS x)` or a star (e.g. `SELECT (x).* FROM (SELECT pg_expand_keywords()
AS x)`).

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
knz added a commit to knz/cockroach that referenced this issue Jun 13, 2018
This work is yet another step towards cockroachdb#16971.

The labeled tuples introduced in cockroachdb#25283 can now be accessed using
their labels or using a star, e.g. `(E).*`.

Release note (sql change): Labeled tuples can now be accessed using
their labels (e.g. `SELECT (x).word FROM (SELECT pg_expand_keywords()
AS x)` or a star (e.g. `SELECT (x).* FROM (SELECT pg_expand_keywords()
AS x)`).

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
knz added a commit to knz/cockroach that referenced this issue Jun 13, 2018
This work is yet another step towards cockroachdb#16971.

The labeled tuples introduced in cockroachdb#25283 can now be accessed using
their labels or using a star, e.g. `(E).*`.

Release note (sql change): Labeled tuples can now be accessed using
their labels (e.g. `SELECT (x).word FROM (SELECT pg_expand_keywords()
AS x)` or a star (e.g. `SELECT (x).* FROM (SELECT pg_expand_keywords()
AS x)`).

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
knz added a commit to knz/cockroach that referenced this issue Jun 14, 2018
This work is yet another step towards cockroachdb#16971.

The labeled tuples introduced in cockroachdb#25283 can now be accessed using
their labels or using a star, e.g. `(E).*`.

Release note (sql change): Labeled tuples can now be accessed using
their labels (e.g. `SELECT (x).word FROM (SELECT pg_expand_keywords()
AS x)` or a star (e.g. `SELECT (x).* FROM (SELECT pg_expand_keywords()
AS x)`).

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Jun 14, 2018
26628: sql: Support tuple column access and tuple stars r=knz a=knz

First commits from #26621.
Completes the fix to #24866 by re-activating disabled tests.
This work is yet another step towards #16971. It would actually fix #16971 if it were not for #26627, #26624 and #26629.

This work is yet another step towards #16971.

The labeled tuples introduced in #25283 can now be accessed using
their labels or using a star, e.g. `(E).*`.

Release note (sql change): Labeled tuples can now be accessed using
their labels (e.g. `SELECT (x).word FROM (SELECT pg_expand_keywords()
AS x)` or a star (e.g. `SELECT (x).* FROM (SELECT pg_expand_keywords()
AS x)`).

Fixes #26720.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Co-authored-by: Bram Gruneir <bram@cockroachlabs.com>
@craig craig bot closed this as completed in #26628 Jun 14, 2018
@knz
Copy link
Contributor

knz commented Jun 14, 2018

Oops, no this is not done yet

@knz knz reopened this Jun 14, 2018
@knz
Copy link
Contributor

knz commented Aug 30, 2018

This is complete now.

@knz knz closed this as completed Aug 30, 2018
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-semantics C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants