sql/pgwire: support decoding unknown (OID 705) in text/binary format

[nvanbenschoten]

Needed for #69010.

Postgres implements this here:
https://github.com/postgres/postgres/blob/2d689f2ee4615629867c4319a35533696cd16589/src/backend/utils/adt/varlena.c#L610-L662

They appear to parse as a cstring datum type, but from my limited testing, it seems to work fine to parse them as a text datum type.

Prototype:

diff --git a/pkg/sql/pgwire/pgwirebase/encoding.go b/pkg/sql/pgwire/pgwirebase/encoding.go
index 201ef9f272..b4eb2bb1b6 100644
--- a/pkg/sql/pgwire/pgwirebase/encoding.go
+++ b/pkg/sql/pgwire/pgwirebase/encoding.go
@@ -776,7 +776,7 @@ func DecodeDatum(
                return tree.MakeDEnumFromLogicalRepresentation(t, string(b))
        }
        switch id {
-       case oid.T_text, oid.T_varchar:
+       case oid.T_text, oid.T_varchar, oid.T_unknown:
                if err := validateStringBytes(b); err != nil {
                        return nil, err
                }

Epic CRDB-10300

[nvanbenschoten]

There's more to this story related to how T_unknown is handled when supplied as a prepared statement type hint. I haven't bottomed out on it yet or gained much of an understanding. However, I found that the following patch fixes some behavior in cases where PostgREST provides an unknown type in a prepared statement type hint:

diff --git a/pkg/sql/pgwire/conn.go b/pkg/sql/pgwire/conn.go
index 7ea89d5439..2b4e6e39f6 100644
--- a/pkg/sql/pgwire/conn.go
+++ b/pkg/sql/pgwire/conn.go
@@ -862,6 +862,10 @@ func (c *conn) handleParse(
                                sqlTypeHints[i] = nil
                                continue
                        }
+                       if t == oid.T_unknown {
+                               sqlTypeHints[i] = nil
+                               continue
+                       }
                        v, ok := types.OidToType[t]
                        if !ok {
                                err := pgwirebase.NewProtocolViolationErrorf("unknown oid type: %v", t)

I believe this change is allowing the placeholder to be interpreted as an untyped literal string constant, as opposed to a concrete data type. This would make some sense, given that Postgres defines the unknown psuedo-type as "identifying a not-yet-resolved type, e.g., of an undecorated string literal."

[rafiss]

Interesting finds. This is related to #60907 -- I wonder if the changes you're talking about are a better way to resolve that issue.

[otan]

Is there a somewhat more concrete way of how this is hit? It's don't understand how to test against / understand the problem without.

[nvanbenschoten]

I don't know of a more concrete way to hit this other than to run a query through PostgREST, unfortunately. PostgREST uses Hasql, so it's possible that all queries issued by Hasql hit this issue.

[otan]

do you know how the query is setup / run?

like what prepare commands etc. are provided?

[nvanbenschoten]

I don't quite know, but I went back to my prototype branch, rebased it on master to pick up some of the recent improvements, and tried to get this in a better state for you.

Here's the branch: https://github.com/nvanbenschoten/cockroach/commits/nvanbenschoten/postgrest. Notice that the second commit includes just the changes related to this issue. It's split out so that you can selectively remove that commit to isolate the need for this issue.

Here are instructions to get PostgREST up and running with that branch:

# in terminal 1
git checkout nvanbenschoten/postgrest
make buildshort
./cockroach demo --insecure

# in terminal 1's SQL shell
create role web_anon nologin;
grant select on all tables in schema public to web_anon;

create role authenticator login;
grant web_anon to authenticator;

# in terminal 2 (assuming macOS)
curl -LO https://github.com/PostgREST/postgrest/releases/download/v9.0.0-a1/postgrest-v9.0.0-a1-macos-x64.tar.gz
tar -xzf postgrest-v9.0.0-a1-macos-x64.tar.gz
chmod +x postgrest
echo 'db-uri = "postgres://authenticator@localhost:26257/movr"' >> tutorial.conf
echo 'db-schema = "public"'                                     >> tutorial.conf
echo 'db-anon-role = "web_anon"'                                >> tutorial.conf
./postgrest tutorial.conf

# in terminal 3
curl -s 'http://localhost:3000/users?limit=3' | jq

This cURL request returns the following with the patch:

[
  {
    "address": "88194 Angela Gardens Suite 94",
    "city": "amsterdam",
    "credit_card": "4443538758",
    "id": "ae147ae1-47ae-4800-8000-000000000022",
    "name": "Tyler Dalton"
  },
  {
    "address": "29590 Butler Plain Apt. 25",
    "city": "amsterdam",
    "credit_card": "3750897994",
    "id": "b3333333-3333-4000-8000-000000000023",
    "name": "Dillon Martin"
  },
  {
    "address": "32768 Eric Divide Suite 88",
    "city": "amsterdam",
    "credit_card": "8107478823",
    "id": "b851eb85-1eb8-4000-8000-000000000024",
    "name": "Deborah Carson"
  }
]

and the following without:

{
  "hint": "You have encountered an unexpected error.\n\nPlease check the public issue tracker to check whether this problem is\nalready tracked. If you cannot find it there, please report the error\nwith details by creating a new issue.\n\nIf you would rather not post publicly, please contact us directly\nusing the support form.\n\nWe appreciate your feedback.\n",
  "details": "stack trace:\ngithub.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirebase/encoding.go:817: DecodeDatum()\ngithub.com/cockroachdb/cockroach/pkg/sql/conn_executor_prepare.go:401: func2()\ngithub.com/cockroachdb/cockroach/pkg/sql/conn_executor_prepare.go:418: execBind()\ngithub.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1820: execCmd()\ngithub.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1645: run()\ngithub.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:664: ServeConn()\ngithub.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:639: func1()\nruntime/asm_amd64.s:1581: goexit()\n",
  "code": "XX000",
  "message": "internal error: error in argument for $1: unsupported OID 705 with format code FormatText"
}

Does that help?

[nvanbenschoten]

There's actually a bit more. This example so far hasn't needed the change to pkg/sql/pgwire/conn.go. But the following additional example does:

# in SQL shell
CREATE TABLE kv (k INT PRIMARY KEY, v INT);
INSERT INTO kv VALUES (1, 2);
GRANT SELECT ON kv TO web_anon;

With the change to pkg/sql/pgwire/conn.go, we get:

➜ curl -s 'http://localhost:3000/kv' | jq
[
  {
    "k": 1,
    "v": 2
  }
]
➜ curl -s 'http://localhost:3000/kv?k=eq.1' | jq
[
  {
    "k": 1,
    "v": 2
  }
]

Without the change to pkg/sql/pgwire/conn.go, we get:

➜ curl -s 'http://localhost:3000/kv' | jq
[
  {
    "k": 1,
    "v": 2
  }
]
➜ curl -s 'http://localhost:3000/kv?k=eq.1' | jq
[]

Notice that without the change, the request that uses a comparison is broken. I don't remember (or never knew) why this was, but it had something to do with an INT placeholder being interpreted as a STRING.

[rafiss]

As far as unknown decoding, here's a pgwire test we can use. The following expected output is what Postgres has:

send
Query {"String": "DROP TABLE IF EXISTS unknown_test"}
----

until ignore=NoticeResponse
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"DROP TABLE"}
{"Type":"ReadyForQuery","TxStatus":"I"}

send
Query {"String": "CREATE TABLE unknown_test (a INT8, b TEXT)"}
----

until
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"CREATE TABLE"}
{"Type":"ReadyForQuery","TxStatus":"I"}

# 'S' for Statement
# ParameterFormatCodes = [0] for text format
send
Parse {"Name": "s1", "Query": "INSERT INTO unknown_test VALUES($1, $2)", "ParameterOIDs":[705, 705]}
Describe {"ObjectType": "S", "Name": "s1"}
Bind {"DestinationPortal": "p1", "PreparedStatement": "s1", "ParameterFormatCodes": [0], "Parameters": [{"text":"1"}, {"text":"one"}]}
Execute {"Portal": "p1"}
Sync
----

until
ReadyForQuery
----
{"Type":"ParseComplete"}
{"Type":"ParameterDescription","ParameterOIDs":[20,25]}
{"Type":"NoData"}
{"Type":"BindComplete"}
{"Type":"CommandComplete","CommandTag":"INSERT 0 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

send
Query {"String": "SELECT a, b FROM unknown_test"}
----

until
ReadyForQuery
----
{"Type":"RowDescription","Fields":[{"Name":"a","TableOID":16530,"TableAttributeNumber":1,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0},{"Name":"b","TableOID":16530,"TableAttributeNumber":2,"DataTypeOID":25,"DataTypeSize":-1,"TypeModifier":-1,"Format":0}]}
{"Type":"DataRow","Values":[{"text":"1"},{"text":"one"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

I added the above to pkg/sql/pgwire/testdata/pgtest/postgrest. Running with make test PKG=./pkg/sql/pgwire TESTS='TestPGTest/postgrest' triggers the error.

[otan]

hah your changes seem perfectly sane to me, so I shall just add tests and create a PR.