SQL migrations don't do schema template substitution

[dennisgsmith]

Thanks for your work, this is a really cool project.

When following the steps to add sql migrations here, I noticed the output is invalid syntax, the /* TEMPLATE: schema */ substitution is not done. It' easy enough to find and replace, but maybe a cli flag to specify the schema would be nice, or even just having it default to public for the cli output. Seems like it was introduced in https://github.com/riverqueue/river/releases/tag/v0.21.0 with #848

[brandur]

Thanks.

I opened #903 to add a --schema option for river-migrate up. It also removes the template comment in case --schema isn't provided.

I noticed the output is invalid syntax

Even though not ideal, the SQL should all still be valid. The /* TEMPLATE: schema */ is a SQL comment, so it's ignored by Postgres when the migration is run.

[dennisgsmith]

You're right! For some reason the inline comments were causing a syntax error in my homegrown migration script, but I just checked the same river migrations in a fresh Postgres database and it runs fine. Sorry about that, there must be something going wrong in my script causing this.

[dennisgsmith]

Oops, in relation to my issue with inline comments I closed this prematurely and am unable to reopen from my phone, didn't see you had linked #903 against this.

[dennisgsmith]

@brandur Ok, had some time to look into this further and there is a bug related to the template change.

The river_migration table never gets created because to_regclass('/* TEMPLATE: schema */river_migration') either errors (<= Postgres 15) or evaluates to NULL (>= Postgres 16).

    IF (SELECT to_regclass('/* TEMPLATE: schema */river_migration') IS NOT NULL) THEN

I took a deeper dive and it seems that the internals of to_regclass changed from Postgres 15 to 16:

(screenshot since the diff is to large to link the specific lines):

It looks like the error is caught and turned to NULL instead of bubbling up. I checked and found that even though Postgres 16 and 17 do not error, the river_migration table never gets created because to_regclass('/* TEMPLATE: schema */river_migration') will evaluate to NULL in those versions.

Steps to reproduce:

river migrate-get --line main --all --exclude-version 1 --up > river_all.up.sql

# Error bubbles up
docker run -u postgres -e POSTGRES_PASSWORD=postgres -d --name pg15 postgres:15
cat ./river_all.up.sql | docker exec -i pg15 psql -U postgres -d postgres

CREATE TYPE
CREATE TABLE
CREATE INDEX
CREATE INDEX
CREATE INDEX
CREATE INDEX
CREATE INDEX
CREATE FUNCTION
CREATE TRIGGER
CREATE TABLE
ALTER TABLE
UPDATE 0
ALTER TABLE
ALTER TABLE
UPDATE 0
ALTER TABLE
ALTER TABLE
ALTER TABLE
UPDATE 0
ALTER TABLE
ALTER TYPE
ALTER TABLE
ALTER TABLE
DROP TRIGGER
DROP FUNCTION
CREATE TABLE
ALTER TABLE
ERROR:  invalid name syntax
CONTEXT:  SQL expression "(SELECT to_regclass('/* TEMPLATE: schema */river_migration') IS NOT NULL)"
PL/pgSQL function inline_code_block line 6 at IF
ALTER TABLE
CREATE INDEX
CREATE TABLE
CREATE TABLE
CREATE FUNCTION
ALTER TABLE
CREATE INDEX
DROP INDEX


# Silently fails, `river_migration` table doesn't get created, to_regclass('/* TEMPLATE: schema */river_migration') is NULL
docker run -u postgres -e POSTGRES_PASSWORD=postgres -d --name pg16 postgres:16
cat ./river_all.up.sql | docker exec -i pg16 psql -U postgres -d postgres

CREATE TYPE
CREATE TABLE
CREATE INDEX
CREATE INDEX
CREATE INDEX
CREATE INDEX
CREATE INDEX
CREATE FUNCTION
CREATE TRIGGER
CREATE TABLE
ALTER TABLE
UPDATE 0
ALTER TABLE
ALTER TABLE
UPDATE 0
ALTER TABLE
ALTER TABLE
ALTER TABLE
UPDATE 0
ALTER TABLE
ALTER TYPE
ALTER TABLE
ALTER TABLE
DROP TRIGGER
DROP FUNCTION
CREATE TABLE
ALTER TABLE
DO
ALTER TABLE
CREATE INDEX
CREATE TABLE
CREATE TABLE
CREATE FUNCTION
ALTER TABLE
CREATE INDEX
DROP INDEX

Still ,the easiest workaround has been to just find and replace (delete) /* TEMPLATE: schema */ so it just uses the default search_path. Possibly easiest resolution would be to just have that be the default behavior in the case that there is no schema explicitly provided.

[dennisgsmith]

I'll close this again since #903 was merged and fixes this, just wanted to add the above comment for posterity.

[brandur]

Ah you're right, that schema template is actually inside a string. It's always gone by the time it hits Postgres when running through the migration framework, but it's wrong when dumping raw SQL without going through sqlctemplate.

Yeah, good to leave this closed though. #903 will fix it definitively.