sql: current_schema should skip over invalid schemas

[wzrdtales]

BUG REPORT

For cockroachdb 2.0.0 the node driver fails to create a table for unknown reasons when using it with db-migrate:

[ERROR] error: invalid target name: "migrations"
    at Connection.parseE (/home/node/node_modules/pg/lib/connection.js:545:11)
    at Connection.parseMessage (/home/node/node_modules/pg/lib/connection.js:370:19)
    at Socket.<anonymous> (/home/node/node_modules/pg/lib/connection.js:113:22)
    at emitOne (events.js:116:13)
    at Socket.emit (events.js:211:7)
    at addChunk (_stream_readable.js:263:12)
    at readableAddChunk (_stream_readable.js:250:11)
    at Socket.Readable.push (_stream_readable.js:208:10)
    at TCP.onread (net.js:607:20)

If you execute the same query by hand, it does work for unknown reasons:

CREATE TABLE IF NOT EXISTS "migrations" ("id"   SERIAL PRIMARY KEY NOT NULL, "name" VARCHAR (255) NOT NULL, "run_on" TIMESTAMP  NOT NULL) 

On my side everything is patched now:

db-migrate/cockroachdb@ce29dca

[wzrdtales]

To reproduce you could simply do

npm i -g db-migrate
npm i -g db-migrate-cockroachdb@^2.0.2

and just run

DATABASE_URL=cockroachdb://root@yourip:26257/yourdb db-migrate up --verbose

[wzrdtales]

Seems to be an issue with the query

SET search_path TO "user","public";

After that the create queries fail.

[wzrdtales]

Have published a patch for my cockroachdb driver db-migrate/cockroachdb@ce29dca

You have to use version 2.0.2 explicitly to reproduce the issue properly.

[knz]

@wzrdtales you mention your client sends SET search_path TO "user","public";.
CockroachDB does not support the user schema. Can you clarify what your intent is here? The search_path in CockroachDB should always have public in the first position.

[knz]

Even in PostgreSQL the same search path would cause the same error if the user schema does not exist.

[knz]

@wzrdtales looking at your code, I understand this SET statement is issued by the baseline library that your code is overloading.

Can you share with us the exact text of the SET statement sent by the client / received by the server? You can use the execution log for this (set sql.trace.log_statement_execute = true then pull the cockroach-sql-exec log file from your log directory).

[wzrdtales]

@knz I see, I guess I need to explain a bit more.

you mention your client sends SET search_path TO "user","public";.
CockroachDB does not support the user schema. Can you clarify what your intent is here? The search_path in CockroachDB should always have public in the first position.

What you changed from version 1 to version 2 was, that table_schema was the column in information_schema.tables before. What the driver does is just taking the db and setting it to schema and use the pg driver underneath. Which worked for version 1. However now in version 2 you moved this from table_schema to table_catalog instead. So "user" here is not wrong, it is the database in this case which the driver connects to. And public is the default schema. So this query is actually not really wrong and should work normally, this is executed as you can see here https://github.com/db-migrate/pg/blob/111586d8461c4ab90949db0d14a4a126b4b3f1a8/index.js#L233 exactly the same for postgres databases. So this seems to be still a bug in that case.

So to provide you the exact statements,you actually can reproduce those with the instruction above. However here you go:

[INFO] Using dev settings: { user: 'root',
  driver: 'cockroachdb',
  database: 'user',
  host: 'localhost',
  port: 26257 }
[INFO] require: db-migrate-cockroachdb
[INFO] connecting
[INFO] connected
[SQL] select version() as version
[SQL] SHOW search_path
[SQL] SET search_path TO "user","public"
[SQL] SELECT table_name FROM information_schema.tables WHERE table_name = 'migrations' AND table_schema = 'user'
[INFO] creating table: migrations
[SQL] CREATE TABLE  "migrations" ("id"   SERIAL PRIMARY KEY NOT NULL, "name" VARCHAR (255) NOT NULL, "run_on" TIMESTAMP  NOT NULL) 

[jordanlewis]

Even in PostgreSQL the same search path would cause the same error if the user schema does not exist.

That's not true, empirically - I just tried it. The core incompatibility here is that Postgres permits search paths that contain nonexistent schemas, and CockroachDB doesn't. We should just fix that.

The first element specifies that a schema with the same name as the current user is to be searched. If no such schema exists, the entry is ignored.

See https://www.postgresql.org/docs/9.1/static/ddl-schemas.html and https://www.postgresql.org/docs/9.1/static/runtime-config-client.html.

@wzrdtales you will need to change the SELECT as well, though - as you noticed, the database name now lives in table_catalog and not table_schema. This is fully compatible with Postgres and the information_schema specification.

[wzrdtales]

There is already a patch online that fixes that issue for my driver, just staying here to figure out with you what needs to be done on your side :) And your mentioning of table_catalog is exactly what the patch I posted above is about and what I described in the last answer though :)

So the assumption made here is, that the db name is equivalent to schema which would correctly resolve to schemaname,public. Which has been the case previously, but is wrong now.

[wzrdtales]

So I would suggest at least documenting this changes from v1 to v2 and making clear that the db name is not anymore equal to the schema name.

[knz]

@jordanlewis please check again:

The core incompatibility here is that Postgres permits search paths that contain nonexistent schemas,

CockroachDB does the same, to search for existing objects.

The issue here is the attempt to create a table. Both CockroachDB and PostgreSQL create objects in the "current schema" (= the first item in search_path). If that schema does not exist, object creation fails. It is not valid to skip over invalid schemas in that case, for otherwise all hell breaks loose: if none of the schemas in search_path exist, both engines would then try to create the table in pg_catalog, which in turns would trigger very weird and unexpected error messages.

[jordanlewis]

@knz, no, unfortunately that's false (with regards to creating new tables). I'll demonstrate:

jordan=# set search_path="nonexistant","public";
SET
jordan=# show search_path;
     search_path
---------------------
 nonexistant, public
(1 row)

jordan=# create table lives_in_public(a int primary key);
CREATE TABLE
jordan=# select table_catalog, table_schema, table_name from information_schema.tables where table_name='lives_in_public';
 table_catalog | table_schema |   table_name
---------------+--------------+-----------------
 jordan        | public       | lives_in_public
(1 row)

[knz]

Here's what I'm referring to:

https://www.postgresql.org/docs/10/static/sql-createtable.html

If a schema name is given (for example, CREATE TABLE myschema.mytable ...) then the table is created in the specified schema. Otherwise it is created in the current schema.

[knz]

Separately, in https://www.postgresql.org/docs/10/static/functions-info.html

current_schema returns the name of the schema that is first in the search path (or a null value if the search path is empty). This is the schema that will be used for any tables or other named objects that are created without specifying a target schema.

[jordanlewis]

Ah, apparently the docs are lying - it seems current_schema is set to the first existing schema in the search path! From my example above:

jordan=# select current_schema;
 current_schema
----------------
 public
(1 row)

[knz]

Ok thanks for figuring that out. Changing the definition of the current schema in CockroachDB to align with pg is much easier than changing the name resolution rules.