sql: Sequelize fixes

[cuongdo]

These fixes make basic Sequelize CRUD operations work, except for upsert.

---

This change is 

[jordanlewis]

Reviewed 2 of 2 files at r1.
Review status: 2 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/sql/pg_catalog.go, line 817 at r2 (raw file):

    indislive BOOL,
    indisreplident BOOL,
    indkey STRING,

Nice idea! I... don't think that we should merge this, though. It may fix things for Sequelize, but int2vectors in Postgres are arrays - and because they are arrays, there's a lot of external code that uses them as arrays. I don't think we should do a quick fix for Sequelize and break a ton of other postgres compatible code.

For example, here's code within PGJDBC that assumes that indkey is an array: https://github.com/pgjdbc/pgjdbc/search?utf8=%E2%9C%93&q=indkey

And Django: https://github.com/django/django/blob/90db4bb0d72e4731052bd33500840fff00834768/django/db/backends/postgresql/introspection.py#L42

And some other kind of crazy looking PHP/wordpress thing: https://github.com/mfukano/wp-heroku/blob/a057cbeaf54297871c7cea7da73578fb3c6a4f12/wp-content/pg4wp/driver_pgsql_install.php#L99

---

Comments from Reviewable

[cuongdo]

Review status: 2 of 5 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/pg_catalog.go, line 817 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Nice idea! I... don't think that we should merge this, though. It may fix things for Sequelize, but int2vectors in Postgres are arrays - and because they are arrays, there's a lot of external code that uses them as arrays. I don't think we should do a quick fix for Sequelize and break a ton of other postgres compatible code.

For example, here's code within PGJDBC that assumes that indkey is an array: https://github.com/pgjdbc/pgjdbc/search?utf8=%E2%9C%93&q=indkey

And Django: https://github.com/django/django/blob/90db4bb0d72e4731052bd33500840fff00834768/django/db/backends/postgresql/introspection.py#L42

And some other kind of crazy looking PHP/wordpress thing: https://github.com/mfukano/wp-heroku/blob/a057cbeaf54297871c7cea7da73578fb3c6a4f12/wp-content/pg4wp/driver_pgsql_install.php#L99

I believe there are other, harder issues blocking Django, and I'm not clear how that specific code in PGJDBC gets executed (my tests haven't hit that code yet). That said, I'll see what it takes to support this in a better way.

---

Comments from Reviewable

[jordanlewis]

Review status: 2 of 5 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/pg_catalog.go, line 817 at r2 (raw file):

Previously, cuongdo (Cuong Do) wrote…

I believe there are other, harder issues blocking Django, and I'm not clear how that specific code in PGJDBC gets executed (my tests haven't hit that code yet). That said, I'll see what it takes to support this in a better way.

Agreed that this isn't the first failure in either PGJDBC or Django, but my point was just that there's a lot of code that already relies on indkey being an array out there on GitHub - and it would be a shame to break all of that now. I could be convinced that that's the right move, if it's too hard to do otherwise - it just wouldn't be my first choice.

---

Comments from Reviewable

[cuongdo]

Rebased and introduced a new INT2VECTOR column type only usable by virtual tables. It seems that OIDs for result rows are tied to the column type.

@nvanbenschoten Are there simpler ways to introduce a different serialization format for Datums that use DOidWrapper? Or is this the best way to do this?

[cuongdo]

To confirm my understanding, it looks like pgwire.(*writeBuffer).writeTextDatum serializes the result. It's passed a sql.ResultColumn, which is created by sql.makeResultColumns. makeResultColumns takes a []ColumnDescriptor as its only parameter, so unless we make deeper changes, the serialization of all outgoing Datums is tied to the ColumnDescriptor. Unless I'm missing something?

[jordanlewis]

From what I recall when I was testing Sequelize, I believe that Sequelize will be happy as long as the output column in question is sent with an int2vector OID. In other words, I don't think we need to worry about the data appearing in the int2vector format in ./cockroach sql.

You can test my belief by temporarily changing the switch in tArray.Oid() in type.go to return T_int2vector for TypeInt instead of T__int8.

---

Review status: 2 of 19 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[cuongdo]

Regardless, I don't believe I can affect the OID of the outgoing column without changing the ColumnDescriptor.

…

On Thu, Feb 9, 2017 at 12:11 PM Jordan Lewis ***@***.***> wrote: From what I recall when I was testing Sequelize, I believe that Sequelize will be happy as long as the output column in question is sent with an int2vector OID. In other words, I don't think we need to worry about the data appearing in the int2vector format in ./cockroach sql. You can test my belief by temporarily changing the switch in tArray.Oid() in type.go to return T_int2vector for TypeInt instead of T__int8. ------------------------------ Review status: 2 of 19 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/13484#-:-KcYpKnd1zLFdmP7O8qJ:b-8wgc4h>* — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13484 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABffpjUBz3YkaCewTXFeaCbprTt6WuqXks5ra0ixgaJpZM4L7EH3> .

[cuongdo]

@jordanlewis You are right that if I return T_int2vector from tArray.Oid(), Sequelize seems to do the right thing (though I'm not clear how it's doing that). So, that would allow me to get rid of the pgwire changes. That still doesn't avoid the need to make some kind of change to the ColumnDescriptor for indkey to modify the OID.

---

Review status: 2 of 19 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[cuongdo]

Also, I'm going to leave the serialization changes in. This line from Sequelize depends on the fact that indkey has spaces rather than commas:

https://github.com/sequelize/sequelize/blob/v3/lib/dialects/postgres/query.js#L150

While it doesn't complain when it receives a string like {2,3}, it's still doing the wrong thing silently.

---

Review status: 2 of 19 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[nvanbenschoten]

Nice job tackling this!

---

Reviewed 2 of 2 files at r1, 3 of 3 files at r3, 17 of 17 files at r4.
Review status: all files reviewed at latest revision, 13 unresolved discussions, all commit checks successful.

---

pkg/sql/create.go, line 1160 at r4 (raw file):

			if !desc.IsVirtualTable() {
				if _, ok := d.Type.(*parser.ArrayColType); ok {
					return desc, util.UnimplementedWithIssueErrorf(2115, "ARRAY and INT2VECTOR column types are unsupported")

Throughout: "array" should correspond to "vector", because we're referring to a generic data type. So s/INT2VECTOR/VECTOR/

---

pkg/sql/pg_catalog.go, line 828 at r4 (raw file):

						}
					}
					indkeyArr, err := colIDArrayToDatum(index.ColumnIDs)

Let's change this to colIDArrayToArray and change the signature to return a *parser.DArray. Then we can create a new function right below it called colIDArrayToVector that performs this wrapping.

---

pkg/sql/show.go, line 171 at r3 (raw file):

		}
	default:
		if _, ok := varGen[name]; !ok {

Why not just change the names in varGen to be lower case and switch name := strings.ToUpper(n.Name) to name := strings.ToLower(n.Name). Will that break anything other than forcing a few more test changes?

---

pkg/sql/parser/col_types.go, line 328 at r4 (raw file):

}

var int2vectorColType = &ArrayColType{

This was the approach I initially took with NameColType and OidColType. I would strongly suggest we create a new ColumnType implementation instead. The if ct.Name == "INT2VECTOR" { special cases are both ugly and fragile.

---

pkg/sql/parser/datum.go, line 2012 at r4 (raw file):

		panic(fmt.Errorf("cannot wrap %T with an Oid", v))
	default:
		// Currently only *DInt and *DString are hooked up to work with *DOidWrapper.

For DOidWrapper support of *DArray, we're going to need to honor this comment. That means replacing all .(*DArray) type assertions with corresponding function calls implemented in the style of AsDInt and MustBeDInt.

---

pkg/sql/parser/datum.go, line 2159 at r4 (raw file):

}

// NewDIntVectorFromDArray is a helper routine to create a *DIntVector

let's move this up below NewDOid

---

pkg/sql/pgwire/types.go, line 181 at r4 (raw file):

	case *parser.DArray:
		if w, ok := d.(*parser.DOidWrapper); ok {

I'd turn this into a switch statement based on the OID of d (d.ResolvedType().OID()) directly so that we don't need the checked type assertion.

---

pkg/sql/pgwire/types.go, line 185 at r4 (raw file):

			case oid.T_int2vector:
				// int2vectors are serialized as a string of space-separated values.
				a := w.Wrapped.(*parser.DArray)

v is already this array

---

pkg/sql/sqlbase/structured.go, line 1670 at r4 (raw file):

	case parser.TypeInterval:
		ctyp.Kind = ColumnType_INTERVAL
	case parser.TypeIntVector:

nit: move to below parser.TypeIntArray.

---

pkg/sql/sqlbase/structured.go, line 1720 at r4 (raw file):

	case ColumnType_OID:
		return parser.TypeOid
	case ColumnType_INT2VECTOR:

same

---

pkg/sql/sqlbase/structured.proto, line 44 at r4 (raw file):

    NAME = 11;
    OID = 12;
    INT2VECTOR = 13;

Let's do something similar to below. Make this = 120 or something so that if/when we decide on a more general approach to this, we won't have baggage.

---

pkg/sql/sqlbase/testutils.go, line 136 at r4 (raw file):

		return parser.NewDOid(parser.DInt(rng.Int63()))
	case ColumnType_INT_ARRAY, ColumnType_INT2VECTOR:
		// TODO(cuongdo): we don't support for persistence of arrays yet

arrays or vectors

---

Comments from Reviewable

[cuongdo]

Review status: 1 of 23 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

pkg/sql/create.go, line 1160 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Throughout: "array" should correspond to "vector", because we're referring to a generic data type. So s/INT2VECTOR/VECTOR/

Done.

---

pkg/sql/pg_catalog.go, line 828 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's change this to colIDArrayToArray and change the signature to return a *parser.DArray. Then we can create a new function right below it called colIDArrayToVector that performs this wrapping.

How do I return a NULL when len(index.ColumnIDs) == 0? That's what Postgres seems to do with various ARRAY columns, such as pg_constraint.conkey

---

pkg/sql/show.go, line 171 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why not just change the names in varGen to be lower case and switch name := strings.ToUpper(n.Name) to name := strings.ToLower(n.Name). Will that break anything other than forcing a few more test changes?

Done.

---

pkg/sql/parser/col_types.go, line 328 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This was the approach I initially took with NameColType and OidColType. I would strongly suggest we create a new ColumnType implementation instead. The if ct.Name == "INT2VECTOR" { special cases are both ugly and fragile.

Done.

---

pkg/sql/parser/datum.go, line 2012 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

For DOidWrapper support of *DArray, we're going to need to honor this comment. That means replacing all .(*DArray) type assertions with corresponding function calls implemented in the style of AsDInt and MustBeDInt.

Done.

---

pkg/sql/parser/datum.go, line 2159 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

let's move this up below NewDOid

Done.

---

pkg/sql/pgwire/types.go, line 181 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'd turn this into a switch statement based on the OID of d (d.ResolvedType().OID()) directly so that we don't need the checked type assertion.

Much cleaner! Done

---

pkg/sql/pgwire/types.go, line 185 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

v is already this array

Done.

---

pkg/sql/sqlbase/structured.go, line 1670 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: move to below parser.TypeIntArray.

Done.

---

pkg/sql/sqlbase/structured.go, line 1720 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

same

Done.

---

pkg/sql/sqlbase/structured.proto, line 44 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's do something similar to below. Make this = 120 or something so that if/when we decide on a more general approach to this, we won't have baggage.

Done.

---

pkg/sql/sqlbase/testutils.go, line 136 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

arrays or vectors

Done.

---

Comments from Reviewable

[cuongdo]

Thanks for the thorough review, as always!

Rebased and addressed comments

---

Review status: 0 of 23 files reviewed at latest revision, 13 unresolved discussions.

---

Comments from Reviewable

[cuongdo]

@nvanbenschoten Any comments? There have been many recent changes to the type system, so I'd love to get further feedback.

[nvanbenschoten]

with two small changes

---

Reviewed 2 of 19 files at r5, 21 of 21 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/sql/pg_catalog.go, line 828 at r4 (raw file):

Previously, cuongdo (Cuong Do) wrote…

How do I return a NULL when len(index.ColumnIDs) == 0? That's what Postgres seems to do with various ARRAY columns, such as pg_constraint.conkey

Good point, colIDArrayToArray will still return a parser.Datum. But you can handle the NULL case in colIDArrayToVector. Right now that won't even work because you blindly type assert below.

---

pkg/sql/pgwire/types.go, line 191 at r6 (raw file):

			}
			b.writeLengthPrefixedVariablePutbuf()
		case oid.T__int8, oid.T__text, oid.T__name:

Let's just put a default case instead of enumerating all of these.

---

Comments from Reviewable

[cuongdo]

Review status: 2 of 23 files reviewed at latest revision, 3 unresolved discussions.

---

pkg/sql/pg_catalog.go, line 828 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Good point, colIDArrayToArray will still return a parser.Datum. But you can handle the NULL case in colIDArrayToVector. Right now that won't even work because you blindly type assert below.

Done.

---

pkg/sql/pgwire/types.go, line 191 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's just put a default case instead of enumerating all of these.

Done.

---

Comments from Reviewable