Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve dir schema source errors #172

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

bplunkett-stripe
Copy link
Collaborator

@bplunkett-stripe bplunkett-stripe commented Sep 8, 2024

Description

Improve schema source errors to include file path that errored. Including line number is not really viable right now because it would require splitting a string blob into individual SQL statements. Parsing SQL is pretty fragile, and it's something pg-schema-diff tries to avoid. We can revisit adding that functionality later.

cc @aleclarson

Motivation

Partially fixes #167

Testing

Error with file name:

go run ./cmd/pg-schema-diff plan --schema-dir ./internal/migration_acceptance_tests/testdata/dirsrc_invalid_sql --dsn 'host=localhost user=bplunkett dbname=postgres'
Error: generating plan: getting new schema: running DDL (from internal/migration_acceptance_tests/testdata/dirsrc_invalid_sql/schema_0/1.sql): ERROR: relation "non_existent_table" does not exist (SQLSTATE 42P01)
exit status 1

Happy path:

 go run ./cmd/pg-schema-diff plan --schema-dir ./internal/migration_acceptance_tests/testdata/dirsrc_happy_path --dsn 'host=localhost user=bplunkett dbname=postgres'

################################ Generated plan ################################
1. CREATE TYPE "public"."color" AS ENUM ('red', 'green', 'blue');
        -- Statement Timeout: 3s

2. CREATE TABLE "public"."fizzbuzz" (
        "primary_color" color,
        "other_color" color,
        "id" text COLLATE "pg_catalog"."default"
);
        -- Statement Timeout: 3s

3. ALTER TABLE "public"."foobar" ADD COLUMN "color" color;
        -- Statement Timeout: 3s

4. ALTER TABLE "public"."foobar" ADD COLUMN "id" character varying(255) COLLATE "pg_catalog"."default" NOT NULL;
        -- Statement Timeout: 3s

5. CREATE UNIQUE INDEX CONCURRENTLY foobar_pkey ON public.foobar USING btree (id);
        -- Statement Timeout: 20m0s
        -- Lock Timeout: 3s
        -- Hazard INDEX_BUILD: This might affect database performance. Concurrent index builds require a non-trivial amount of CPU, potentially affecting database performance. They also can take a while but do not lock out writes.

6. ALTER TABLE "public"."foobar" ADD CONSTRAINT "foobar_pkey" PRIMARY KEY USING INDEX "foobar_pkey";
        -- Statement Timeout: 3s

7. CREATE TABLE "public"."foobar_fk" (
        "id" text COLLATE "pg_catalog"."default"
);
        -- Statement Timeout: 3s

8. ALTER TABLE "public"."foobar_fk" ADD CONSTRAINT "foobar_fk_id_fkey" FOREIGN KEY (id) REFERENCES foobar(id) NOT VALID;
        -- Statement Timeout: 3s

9. ALTER TABLE "public"."foobar_fk" VALIDATE CONSTRAINT "foobar_fk_id_fkey";
        -- Statement Timeout: 3s

@aleclarson
Copy link

aleclarson commented Sep 9, 2024

Nice! 👍

I think it'd be worth integrating pg_query_go, which you're probably aware of already. Especially useful for line numbers and better dependency tracking (#129).

@bplunkett-stripe bplunkett-stripe merged commit 5fe8259 into main Sep 9, 2024
9 checks passed
@bplunkett-stripe
Copy link
Collaborator Author

I think it'd be worth integrating pg_query_go, which you're probably aware of already. Especially useful for line numbers and better dependency tracking (#129).

Yep, solid call out. My biggest FUD around parsing queries is if the parser doesn't support some common format of query. The current implementation of pg-schema-diff has 0 SQL parsing; we rely entirely on Postgres to do that. I think it's definitely worth integrating with some form of query parsing in the future, but we should make sure there's an escape hatch for users in the event it doesn't work as expected.

@bplunkett-stripe bplunkett-stripe deleted the bplunkett/improve-dir-schema-source-errors branch September 9, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include failed statement's file and line in stderr
3 participants