sql: more Postgres compatibility measures for ActiveRecord #13429

jordanlewis · 2017-02-06T16:47:22Z

A few more compatibility features for ActiveRecord.

OID casts unwrap outer quotes from their inputs - '"table"'::regclass is equivalent to 'table'::regclass.
pg_type's entries for string types are now named the same as in Postgres: text and varchar.
pg_type's entry for decimal types are now named the same as in Postgres: numeric.
pg_namespace now has one entry per database.
Add the (empty) pg_extension virtual table.
Add (no-op) implementations of some advisory lock builtins.
Add the current_database builtin.
Add a new overload to obj_description.

Resolves #12153.
Resolves #12148.
Resolves #13436.
Resolves #13486.
Resolves #13485.
Resolves #13498.

This change is

cuongdo · 2017-02-06T18:38:48Z

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

pkg/sql/parser/builtins.go, line 1406 at r3 (raw file):

				return NewDString(schemaForDb(ctx.Database)), nil
			},
			Info: "Returns the current database.",

This description is now outdated

pkg/sql/parser/builtins.go, line 1436 at r3 (raw file):

				return schemas, nil
			},
			Info: "Returns the current database; optionally include implicit schemas (e.g. " +

This is now outdated

pkg/sql/testdata/builtin_function, line 1147 at r2 (raw file):

SELECT CURRENT_SCHEMAS(true)
----
{pg_catalog,public}

These tests aren't part of this commit

pkg/sql/testdata/pgoidtype, line 12 at r1 (raw file):


query O
SELECT '"pg_constraint"'::REGCLASS

could you also test with some whitespace before and after pg_constraint?

Comments from Reviewable

jordanlewis · 2017-02-06T19:39:40Z

TFTR!

Review status: 1 of 6 files reviewed at latest revision, 4 unresolved discussions.

pkg/sql/parser/builtins.go, line 1406 at r3 (raw file):

Previously, cuongdo (Cuong Do) wrote…

This description is now outdated

Done.

pkg/sql/parser/builtins.go, line 1436 at r3 (raw file):

Previously, cuongdo (Cuong Do) wrote…

This is now outdated

Done.

pkg/sql/testdata/builtin_function, line 1147 at r2 (raw file):

Previously, cuongdo (Cuong Do) wrote…

These tests aren't part of this commit

Done.

pkg/sql/testdata/pgoidtype, line 12 at r1 (raw file):

Previously, cuongdo (Cuong Do) wrote…

could you also test with some whitespace before and after pg_constraint?

Done.

Comments from Reviewable

nvanbenschoten · 2017-02-06T19:46:49Z

Review status: 1 of 6 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

pkg/sql/parser/builtins.go, line 1410 at r4 (raw file):

	},

	// For now, schemas are the same as databases. So, current_schemas returns the

This change should coordinate with #13404.

pkg/sql/testdata/pgoidtype, line 12 at r4 (raw file):


query OO
SELECT '"pg_constraint"'::REGCLASS, '  "pg_constraint" '::REGCLASS

Should something like ' pg_constraint '::REGCLASS (whitespace but no quotes) work?

Comments from Reviewable

jordanlewis · 2017-02-06T23:11:16Z

Review status: 0 of 7 files reviewed at latest revision, 6 unresolved discussions.

pkg/sql/testdata/pgoidtype, line 12 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should something like ' pg_constraint '::REGCLASS (whitespace but no quotes) work?

Good point. Yes. That works now.

Comments from Reviewable

nvanbenschoten · 2017-02-07T09:10:52Z

Review status: 0 of 7 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

pkg/sql/parser/eval.go, line 2220 at r8 (raw file):

			// Trim whitespace and unwrap outer quotes if necessary.
			// This is required to mimic postgres.
			s = strings.Trim(s, " ")

It looks like newlines and tabs should also be stripped for whatever reason. strings.TrimSpaces is probably what we're looking for.

pkg/sql/parser/eval.go, line 2221 at r8 (raw file):

			// This is required to mimic postgres.
			s = strings.Trim(s, " ")
			s = unwrapQuotesRegexp.ReplaceAllString(s, "$1")

We don't really need a regexp anymore then:

if len(s) > 1 && s[0] == `"` && s[len(s)-1] == `"` {
  s = s[1:len(s)-1]
}

Comments from Reviewable

jordanlewis · 2017-02-07T15:25:25Z

Review status: 0 of 7 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

pkg/sql/parser/eval.go, line 2220 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It looks like newlines and tabs should also be stripped for whatever reason. strings.TrimSpaces is probably what we're looking for.

Done.

pkg/sql/parser/eval.go, line 2221 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We don't really need a regexp anymore then:
if len(s) > 1 && s[0] == `"` && s[len(s)-1] == `"` {
  s = s[1:len(s)-1]
}

Done.

Comments from Reviewable

tamird · 2017-02-07T16:24:53Z

Reviewed 1 of 2 files at r5, 6 of 6 files at r9, 3 of 3 files at r10, 3 of 3 files at r11, 3 of 3 files at r12.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks pending.

pkg/sql/parser/builtins.go, line 1441 at r11 (raw file):

			},
			Info: "Returns the schema of the current current database; optionally " +
				"include implicit schemas (e.g. `pg_catalog`). CockroachDB " +

includes?

pkg/sql/parser/builtins.go, line 1664 at r11 (raw file):

const (
	// InformationSchemaName is the name of the information_schema table.

s/table/database/ on all three constants

pkg/sql/parser/builtins.go, line 1674 at r11 (raw file):

// schemaForDb maps a DatabaseDescriptor to its corresponding pgNamespace.
// See the comment above pgNamespace for more details.
func schemaForDb(db string) string {

how come not 'DB'?

pkg/sql/parser/builtins.go, line 1675 at r11 (raw file):

// See the comment above pgNamespace for more details.
func schemaForDb(db string) string {
	if db == InformationSchemaName || db == PgCatalogName || db == SystemName {

might read better as a switch-case

Comments from Reviewable

jordanlewis · 2017-02-07T21:59:45Z

Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

pkg/sql/parser/builtins.go, line 1441 at r11 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

includes?

Done.

pkg/sql/parser/builtins.go, line 1664 at r11 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/table/database/ on all three constants

Done.

pkg/sql/parser/builtins.go, line 1674 at r11 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

how come not 'DB'?

Done.

pkg/sql/parser/builtins.go, line 1675 at r11 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

might read better as a switch-case

Done.

Comments from Reviewable

tamird · 2017-02-07T22:35:10Z

Reviewed 1 of 4 files at r13, 3 of 3 files at r14.
Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

pkg/sql/parser/builtins.go, line 1676 at r14 (raw file):

func schemaForDB(db string) string {
	switch db {
	case InformationSchemaName:

you can make this one single case

switch db {
case InformationSchemaName, PgCatalogName, SystemName:
  return db
default:
  return "public"
}

Comments from Reviewable

jordanlewis · 2017-02-07T22:37:46Z

Review status: 3 of 7 files reviewed at latest revision, 8 unresolved discussions.

pkg/sql/parser/builtins.go, line 1676 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

you can make this one single case

switch db {
case InformationSchemaName, PgCatalogName, SystemName:
  return db
default:
  return "public"
}

Done.

Comments from Reviewable

tamird · 2017-02-07T22:54:57Z

Reviewed 1 of 4 files at r15, 3 of 3 files at r16.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

Comments from Reviewable

jordanlewis · 2017-02-08T21:28:40Z

Reworked the current_schema commit after discussing with @knz about compatibility with his upcoming namespaces change. Now the current_schema behavior is left unchanged - but pg_namespace now reports a namespace per database instead of a single public namespace for all user-created databases.

Also added a few more commits with some new fixes for active record and updated the PR description to match.

knz · 2017-02-09T23:30:58Z

Awesome, thanks. One request about a builtin description, though. Looks otherwise fine to me.

Reviewed 1 of 3 files at r16, 1 of 4 files at r18, 3 of 3 files at r21, 1 of 1 files at r22.
Review status: 7 of 8 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

pkg/sql/parser/builtins.go, line 1395 at r22 (raw file):

	},

	"current_database": {

Side note: The existence of both current_database and current_schema is bound to introduce confusion.
All the more reason to do something about #13319.

pkg/sql/parser/builtins.go, line 1421 at r22 (raw file):

				return NewDString(ctx.Database), nil
			},
			Info: "Returns the current database.",

Once current_database becomes available, please change the Info field of current_schema to say "This function is provided for compatibility with PostgreSQL. For a new CockroachDB application, consider using current_database() instead."

Comments from Reviewable

jordanlewis · 2017-02-10T00:09:41Z

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

pkg/sql/parser/builtins.go, line 1421 at r22 (raw file):

Previously, knz (kena) wrote…

Once current_database becomes available, please change the Info field of current_schema to say "This function is provided for compatibility with PostgreSQL. For a new CockroachDB application, consider using current_database() instead."

Done.

Comments from Reviewable

Postgres unwraps inner quotes when doing oid casts, and some ORMs rely on this behavior. For example, `'table'::REGCLASS` and `'"table"'::REGCLASS` are treated the same.

For postgres compatibility. Strings are called either text or varchar depending on their OID, despite their identical disk and syntax representation in CockroachDB.

CockroachDB doesn't support extensions, so this is an empty table. It's needed for ORM support.

pg_namespace now contains a single namespace for every database, including system ones, named as the database is. This corrects the broken link between what `current_schema` reports and the `pg_namespace` virtual table - this link is required for ORM compatibility. Previously, we implemented pg_namespace by exposing a single `public` namespace to which every user-created database was linked. This is inconsistent with Postgres as well as our future plans for schemas/namespaces. We no longer expose any namespaces with the name `public`.

These are required for ActiveRecord ORM support. We implement them by returning true no matter what.

For ORM compatibility.

jordanlewis · 2017-02-10T16:36:04Z

RFAL - is this good to go?

knz · 2017-02-10T20:13:46Z

It is for me! But I didn't feel I was primary reviewer :-) Jordan Lewis <notifications@github.com> schreef op 10 februari 2017 17:36:07 CET:

…

RFAL - is this good to go? -- You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: #13429 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

jordanlewis · 2017-02-10T20:16:08Z

I mainly didn't set you as a primary reviewer because you have been awfully busy - but I'm more than happy to take your LGTM if you're offering 😄

bdarnell · 2017-02-12T18:13:22Z

Review status: 0 of 8 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

pkg/sql/parser/builtins.go, line 1680 at r30 (raw file):

			ReturnType: fixedReturnType(TypeBool),
			fn: func(_ *EvalContext, _ Datums) (Datum, error) {
				return DBoolTrue, nil

I'm a little late to this PR, but it seems bad to lie about locking like this. Even if ActiveRecord is (currently) OK with us stubbing out this lock, implementing this function in a silently broken way could cause problems for other frameworks in the future.

Comments from Reviewable

jordanlewis · 2017-02-12T19:32:44Z

Review status: 0 of 8 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

pkg/sql/parser/builtins.go, line 1680 at r30 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I'm a little late to this PR, but it seems bad to lie about locking like this. Even if ActiveRecord is (currently) OK with us stubbing out this lock, implementing this function in a silently broken way could cause problems for other frameworks in the future.

Good point. Opened #13546.

Comments from Reviewable

sandstrom · 2020-08-14T08:53:28Z

You should mention in your docs somewhere, that there are known limitations with your ActiveRecord compatibility and explain that these things are (mentioning this one).

/~https://github.com/rails/rails/blob/8de10f9bb6edaf10bde21cea73db3da2aac6f6de/activerecord/lib/active_record/migration.rb#L151

brandondrew · 2023-03-26T22:33:38Z

@jordanlewis

TFTR!

Tokamak Fusion Test Reactor?

jordanlewis · 2023-03-26T22:46:21Z

It stands for "thanks for the review"

jordanlewis requested review from cuongdo and nvanbenschoten February 6, 2017 16:47

jordanlewis force-pushed the activerecord-fixes branch from a32f1c8 to 049f0b3 Compare February 6, 2017 16:53

jordanlewis force-pushed the activerecord-fixes branch 2 times, most recently from 1a96562 to b852805 Compare February 6, 2017 19:39

jordanlewis force-pushed the activerecord-fixes branch 3 times, most recently from 99b326f to 2520cb0 Compare February 6, 2017 23:10

jordanlewis force-pushed the activerecord-fixes branch 2 times, most recently from 1cb8586 to f39d118 Compare February 7, 2017 15:31

jordanlewis force-pushed the activerecord-fixes branch from f39d118 to eb4916b Compare February 7, 2017 21:59

jordanlewis force-pushed the activerecord-fixes branch from eb4916b to 3a1bae1 Compare February 7, 2017 22:37

jordanlewis mentioned this pull request Feb 7, 2017

sql: support ActiveRecord through postgresql adapter #12792

Closed

13 tasks

jordanlewis force-pushed the activerecord-fixes branch from 3a1bae1 to 660e82b Compare February 8, 2017 21:24

jordanlewis force-pushed the activerecord-fixes branch 3 times, most recently from 553e530 to 83bd928 Compare February 9, 2017 22:49

jordanlewis assigned knz Feb 9, 2017

jordanlewis force-pushed the activerecord-fixes branch from 83bd928 to 64e0c1e Compare February 10, 2017 00:09

jordanlewis added 8 commits February 10, 2017 10:43

sql: unwrap inner quotes in oid casts

ba87ee7

Postgres unwraps inner quotes when doing oid casts, and some ORMs rely on this behavior. For example, `'table'::REGCLASS` and `'"table"'::REGCLASS` are treated the same.

sql: strings are named correctly in pg_type

0b1596f

For postgres compatibility. Strings are called either text or varchar depending on their OID, despite their identical disk and syntax representation in CockroachDB.

sql: implement pg_extension virtual table

2835d15

CockroachDB doesn't support extensions, so this is an empty table. It's needed for ORM support.

sql: add current_database() builtin

de98aa3

sql: no-op pg_advisory_lock functions

49036d2

These are required for ActiveRecord ORM support. We implement them by returning true no matter what.

sql: decimals are called 'numeric' in pg_type

07efe74

For ORM compatibility.

sql: add obj_description(int, string) overload

96e8bce

For ORM compatibility.

jordanlewis force-pushed the activerecord-fixes branch from 64e0c1e to 96e8bce Compare February 10, 2017 16:19

jordanlewis merged commit 71e7475 into cockroachdb:master Feb 10, 2017

jordanlewis deleted the activerecord-fixes branch February 10, 2017 20:33

jordanlewis mentioned this pull request Feb 12, 2017

sql: fill out pg_advisory_lock stubs #13546

Open

jseldess mentioned this pull request Feb 13, 2017

sql: more Postgres compatibility measures for ActiveRecord cockroachdb/docs#1091

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: more Postgres compatibility measures for ActiveRecord #13429

sql: more Postgres compatibility measures for ActiveRecord #13429

jordanlewis commented Feb 6, 2017 •

edited

Loading

cuongdo commented Feb 6, 2017

jordanlewis commented Feb 6, 2017

nvanbenschoten commented Feb 6, 2017

jordanlewis commented Feb 6, 2017

nvanbenschoten commented Feb 7, 2017

jordanlewis commented Feb 7, 2017

tamird commented Feb 7, 2017

jordanlewis commented Feb 7, 2017

tamird commented Feb 7, 2017

jordanlewis commented Feb 7, 2017

tamird commented Feb 7, 2017

jordanlewis commented Feb 8, 2017

knz commented Feb 9, 2017

jordanlewis commented Feb 10, 2017

jordanlewis commented Feb 10, 2017

knz commented Feb 10, 2017 via email

jordanlewis commented Feb 10, 2017

bdarnell commented Feb 12, 2017

jordanlewis commented Feb 12, 2017

sandstrom commented Aug 14, 2020

brandondrew commented Mar 26, 2023

jordanlewis commented Mar 26, 2023

sql: more Postgres compatibility measures for ActiveRecord #13429

sql: more Postgres compatibility measures for ActiveRecord #13429

Conversation

jordanlewis commented Feb 6, 2017 • edited Loading

cuongdo commented Feb 6, 2017

jordanlewis commented Feb 6, 2017

nvanbenschoten commented Feb 6, 2017

jordanlewis commented Feb 6, 2017

nvanbenschoten commented Feb 7, 2017

jordanlewis commented Feb 7, 2017

tamird commented Feb 7, 2017

jordanlewis commented Feb 7, 2017

tamird commented Feb 7, 2017

jordanlewis commented Feb 7, 2017

tamird commented Feb 7, 2017

jordanlewis commented Feb 8, 2017

knz commented Feb 9, 2017

jordanlewis commented Feb 10, 2017

jordanlewis commented Feb 10, 2017

knz commented Feb 10, 2017 via email

jordanlewis commented Feb 10, 2017

bdarnell commented Feb 12, 2017

jordanlewis commented Feb 12, 2017

sandstrom commented Aug 14, 2020

brandondrew commented Mar 26, 2023

jordanlewis commented Mar 26, 2023

jordanlewis commented Feb 6, 2017 •

edited

Loading