Fix bug where already-quoted search path components could get double-quoted when creating the migrations table.

[GriffinSchneider]

Postgres's SHOW search_path returns the search path as a comma-separated string with spaces, like "$user", public, "another-quoted-schema", so checking whether the first character of a search path component is " is not sufficient. Instead, just check whether the search path component contains quotes at all, and don't add any quotes if it does.

To reproduce the bug:

• Run:

  ALTER ROLE <username> SET search_path TO "$user",public,"something-with-quotes";

• Apply migrations
• db-migrate-pg tries to run something like this (note the ""):

  SET search_path TO "$user","public",""something-with-quotes""

• Get an error like:

  [ERROR] error: zero-length delimited identifier at or near """"
      at Connection.parseE (/Users/griffinschneider/dev/core/node_modules/pg/lib/connection.js:604:13)
      at Connection.parseMessage (/Users/griffinschneider/dev/core/node_modules/pg/lib/connection.js:403:19)
      at Socket.<anonymous> (/Users/griffinschneider/dev/core/node_modules/pg/lib/connection.js:123:22)
      at Socket.emit (events.js:305:20)
      at Socket.EventEmitter.emit (domain.js:483:12)
      at addChunk (_stream_readable.js:341:12)
      at readableAddChunk (_stream_readable.js:316:11)
      at Socket.Readable.push (_stream_readable.js:250:10)
      at TCP.onStreamRead (internal/stream_base_commons.js:186:23)

[commitlint-wzrdtales[bot]]

There were the following issues with this Pull Request

• Commit: bba89f40d8ef21437bf8b64772cce0e426d61651
  • ✖ message may not be empty , - ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.

---

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

[mgabeler-lee-6rs]

Any outlook on when this can merge? This bug is giving me hell right now :wink:

[wzrdtales]

Sry went under the radar.

This is missing a test, otherwise that might be fine to suggest. I doubt that anyone names their tables with an escaped quote, nor it should be allowed by most db systems using the pg wire protocol.

[mgabeler-lee-6rs]

My failure for this is far more simple.

1. Using pgBouncer for connection pooling
2. Using a non-default schema for one app
3. The default postgresql search_path of "$user", public

Result: First use of a connection does: SET search_path TO "myschema","$user","public" Second use of a connection from the bouncer does: SET search_path TO "myschema","myschema",""$user"","$public" Which fails with a syntax error

---

Side thought: if the schema name didn't need to be quoted in the output of SHOW search_path, why bother trying to add quotes to it for SET search_path? It seems only the path element being added may need quoting? Is there a corner case I'm missing?

[mgabeler-lee-6rs]

Side Side note: The SET search_path this is doing isn't even effective for my use case, because it may run against a different "real" connection on the far side of the bouncer from the actual migrations. To be safe against such situations, it would need to be done as SET LOCAL search_path TO ... and happen inside a / each transaction, not at the global connection level.

... which also means it ends up unwittingly putting the migrations table in the wrong schema...

[wzrdtales]

this change causes the exact error message the creator was getting. zero-length delimited identifier at or near """"

test suite is finally running again now, just merged for testing and reverted

[mgabeler-lee-6rs]

The CI failure is sadly very difficult to interpret, it's hard to tell where the error is coming from at all :(

Given that PG seems to return the search_path value always already quoted, I retain my earlier suggestion that this entire chunk of code can probably get deleted and just prepend the desired search path to the current one unconditionally. Having a schema listed multiple times in the search path doesn't seem to be an error, so this seems like it should be safe even in cases where it is redundant.

[wzrdtales]

Actually not really.

The last commands executed with your attempted fix are

[SQL] show server_version_num
[SQL] SHOW search_path
[SQL] SET search_path TO ""$user"", public
xx[SQL] SET search_path TO test_schema2
[SQL] SHOW search_path

and without

[SQL] show server_version_num
[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 = 'public'

The $user search path returned by SHOW search_path by postgres is

postgres=# SHOW search_path;
   search_path   
-----------------
 "$user", public
(1 Zeile)

So the check for a starting quote, does its job as it should.

If you want to try get this fix done, the test suite is working now again and is rewritten on lab. You can add global.verbose = true to the tests to temporarily see all the executed queries.

[wzrdtales]

I am not a fan of this search path logic anyways, so if there is a better way to handle, I would be glad to discuss :sweat_smile:

[wzrdtales]

Ultimately:

https://github.com/db-migrate/pg/pull/47/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R222

Your change here caused the check if "first char is not a quote" to become, there is a quote anywhere in the string. Causing the quoting to be triggered. If you actually wanted to avoid quoting this was doing the inverse of it.

[mgabeler-lee-6rs]

So the check for a starting quote, does its job as it should.

This only works for the first element of the search path as written:

postgres=# set search_path = public,"$user";
SET
postgres=# show search_path ;
   search_path   
-----------------
 public, "$user"
(1 row)

oops, the first char of the second element is ... a space due to not trimming the paths after splitting on commas.

We can get more fun if we e.g. start with set search_path = "$user", public, "bobby, tables"; where now the split is just completely wrong.

Your change

I'm not the PR author ;)

caused the check if "first char is not a quote" to become, there is a quote anywhere in the string. Causing the quoting to be triggered. If you actually wanted to avoid quoting this was doing the inverse of it

Yep ... though as we can see with the formatting issue above, fixing this logic inversion would also have caused "$user", public to become "$user", " public" which is also wrong.

My proposal is to replace this entire block: https://github.com/db-migrate/pg/blob/master/index.js#L252-L272

With:

return this.all(`SET search_path TO "${this.schema}", ${result[0].search_path}`);

[wzrdtales]

I'm not the PR author ;)

I didn't exactly scroll through and check :)

[wzrdtales]

My proposal is to replace this entire block

The thing is, those things were added for a reason. I don't know them anymore, but I am certainly breaking someones setup if I'm going to remove this. Generally I would say it should be save to assume, the db returns a usable string that itself understands though.

[wzrdtales]

It is probably all this tango before, trying to get a version from various amounts of different postgres versions and variations. So I guess, we really can't remove this safely, just trying to actually make it work.

[wzrdtales]

The search path actually gets trimmed here. https://github.com/db-migrate/pg/blob/00aa56bb21a149b10fa1703f10cadbe51b1bd180/index.js#L257

This is probably just too late.

[wzrdtales]

yes its too late. added a test reproducing this, and putting trim before actually fixes this issue.

[wzrdtales]

fixed in https://github.com/db-migrate/pg/commit/13e956bf619b1ec283aa26e32377d2d48c96e074 fixed in db-migrate-pg@1.5.1