electric-sql / electric

Sync little subsets of your Postgres data into local apps and services.
https://electric-sql.com
Apache License 2.0
6.19k stars 143 forks source link

Unsupported DO...END block in migration #651

Closed gorbak25 closed 1 month ago

gorbak25 commented 10 months ago
    (electric 0.7.1) lib/electric/postgres/proxy/injector/operation.ex:825: Electric.Postgres.Proxy.Injector.Operation.Electric.Postgres.Proxy.Injector.Operation.SyntaxError.error_response("Unsupported DO...END block in migration", %PgProtocol.Message.Query{query: "DO $$ \nDECLARE \n    schema_exists BOOLEAN;\nBEGIN\n    SELECT EXISTS (\n        SELECT 1\n        FROM information_schema.schemata\n        WHERE schema_name = 'electric'\n    ) INTO schema_exists;\n\n    IF schema_exists THEN\n        CALL electric.electrify('public.\"DataTable\"');\n        CALL electric.electrify('public.\"Column\"');\n        CALL electric.electrify('public.\"IntegrationInstance\"');\n        CALL electric.electrify('public.\"ColumnReference\"');\n        CALL electric.electrify('public.\"Row\"');\n        CALL electric.electrify('public.\"CellValue\"');\n        \n        -- Hotfix the electric schema\n        ALTER TABLE electric.ddl_commands\n        DROP CONSTRAINT ddl_table_unique_migrations;\n        CREATE EXTENSION IF NOT EXISTS pgcrypto;  \n        ALTER TABLE electric.ddl_commands \n        ADD COLUMN query_digest text GENERATED ALWAYS AS (digest(query, 'shake128')) STORED;\n        ALTER TABLE electric.ddl_commands ADD CONSTRAINT ddl_table_unique_migrations UNIQUE (txid, txts, version, query_digest);\n    END IF;\nEND $$;\n"})
magnetised commented 10 months ago

@gorbak25 thanks for the bug report. could you include the sql of the migration please

the error message mentions the use of do blocks... the proxy disallows these because the sql parser doesn't read the body of the do and so we can't figure out what's being done in order to capture the operations.

is there any driving need to use a do block for your migration? The above seems to be checking for the existence of the electric schema before applying the operations. could you skip this check?

gorbak25 commented 10 months ago

@magnetised Thanks for getting back to me! I've got 2 places where I'm using DO $$ ... END $$; blocks.

The first one was used in 0.6.3 to electrify the tables and this bug report refers to it

DO $$ 
DECLARE 
    schema_exists BOOLEAN;
BEGIN
    SELECT EXISTS (
        SELECT 1
        FROM information_schema.schemata
        WHERE schema_name = 'electric'
    ) INTO schema_exists;

    IF schema_exists THEN
        CALL electric.electrify('public."DataTable"');
        CALL electric.electrify('public."Column"');
        CALL electric.electrify('public."IntegrationInstance"');
        CALL electric.electrify('public."ColumnReference"');
        CALL electric.electrify('public."Row"');
        CALL electric.electrify('public."CellValue"');

        -- Hotfix the electric schema
        ALTER TABLE electric.ddl_commands
        DROP CONSTRAINT ddl_table_unique_migrations;
        CREATE EXTENSION IF NOT EXISTS pgcrypto;  
        ALTER TABLE electric.ddl_commands 
        ADD COLUMN query_digest text GENERATED ALWAYS AS (digest(query, 'shake128')) STORED;
        ALTER TABLE electric.ddl_commands ADD CONSTRAINT ddl_table_unique_migrations UNIQUE (txid, txts, version, query_digest);
    END IF;
END $$;

Fortunately after converting it to

ALTER TABLE public."DataTable" ENABLE ELECTRIC;
ALTER TABLE public."Column" ENABLE ELECTRIC;
ALTER TABLE public."IntegrationInstance" ENABLE ELECTRIC;
ALTER TABLE public."ColumnReference" ENABLE ELECTRIC;
ALTER TABLE public."Row" ENABLE ELECTRIC;
ALTER TABLE public."CellValue" ENABLE ELECTRIC;

it just works.

The second occurrence unfortunately is not easy to get rid off. I've got some helper tables maintained using triggers and i want for ON DELETE CASCADE triggers to work on them during logical replication. To achieve that I'm dynamically generating sql which will enable the triggers:

DO $$ 
DECLARE
    rec RECORD;
    v_sql TEXT;
BEGIN
    -- Enables ON DELETE CASCADE ON UPDATE CASCADE triggers during logical replication
    FOR rec IN (
        SELECT 'ALTER TABLE "' || s1.t || '" ENABLE ALWAYS TRIGGER "' || s1.tgname || '";' AS sql
        FROM 
        (
            SELECT 'CellValue' AS t, tgname FROM pg_trigger WHERE tgrelid='"CellValue"'::regclass AND tgconstrrelid='"CellValueForEvaluation"'::regclass
            UNION
            SELECT 'DataTable' AS t, tgname FROM pg_trigger WHERE tgrelid='"DataTable"'::regclass AND tgconstrrelid='"DataTableTopoColumnOrder"'::regclass
        ) AS s1
    )
    LOOP
        v_sql := rec.sql;
        EXECUTE v_sql;
    END LOOP;
END $$;

Is there an alternative to this?

PS. I've managed to get my app running on 7.1 by splitting the schema into 2 parts - one for the proxy, the second one for postgres.

magnetised commented 10 months ago

@gorbak25 glad you got it to work. The problem with do blocks is that, because they allow for full plpgsql programs, not just sql statements, it's nigh on impossible for us to extract the actual ddl statements out of them, which means we can't do our syntax re-writing or various validations.

however, your use case above is valid. I'm thinking perhaps some CALL electric.passthrough($$ body of migration$$) or CALL electric.safe(); -- body of migration follows statement that we could add that would allow for these kinds of opaque commands to come through, with clear guidelines to the developer that Things Will Break(tm) if they touch any electrified tables inside it.

Will think about it and come back to you. You shouldn't have to split the migrations like that.

gorbak25 commented 10 months ago

CALL electric.i_want_to_break_electric($$ ... $$) would work best :) Having an escape hatch for non standard stuff would be beneficial for me!

justindotpub commented 7 months ago

I just ran into this problem as well, while running Oban migrations. I initially thought it was coming from functions I'm writing in plpgsql, but I see now DO END blocks in Oban's migrations. Are DO END blocks not allowed? Are there any other limitations I should be aware of related to the use of plpgsql? Thank you.

justindotpub commented 7 months ago

For reference, here is what the Oban migration is trying to execute.

06:28:11.360 [info] execute "DO $$\nBEGIN\nIF NOT EXISTS (SELECT 1 FROM pg_type\n               WHERE typname = 'oban_job_state'\n                 AND typnamespace = 'public'::regnamespace::oid) THEN\n    CREATE TYPE \"public\".oban_job_state AS ENUM (\n      'available',\n      'scheduled',\n      'executing',\n      'retryable',\n      'completed',\n      'discarded'\n    );\n  END IF;\nEND$$;\n"

Also regarding overall plpgsql support, we make pretty extensive use of plpgsql in our project, but if I temporarily comment out all Oban migration code, I get through all of our migrations, so from what I'm seeing so far it seems DO END blocks are the only issue.

alco commented 7 months ago

@justindotpub We will support passthrough for DO...END blocks one way or another. We just haven't come up with a satisfactory design for it yet.

You can work around this limitation in the proxy by applying some migrations directly to Postgres while still using the Proxy for migrations that touch electrified schema.

Nick-Lucas commented 4 months ago

Migrations are also generated with this syntax by DrizzleORM for enum types

DO $$ BEGIN
  CREATE TYPE "objective_state" AS ENUM('open', 'closed');
EXCEPTION
  WHEN duplicate_object THEN null;
END $$;
jljorgenson18 commented 2 months ago

Yep, I just ran into this as well. I was hoping Drizzle would be less problematic than Prisma. I had no clue Electric wouldn't be handle DO blocks.

It seems like a major use case is that they have exception blocks, but I'm not sure.

I don't see a great way to make Electric work with Drizzle without this.

KyleAMathews commented 1 month ago

👋 we've been working the last month on a rebuild of the Electric server over at a temporary repo https://github.com/electric-sql/electric-next/

You can read more about why we made the decision at https://next.electric-sql.com/about

We're really excited about all the new possibilities the new server brings and we hope you'll check it out soon and give us your feedback.

We're now moving the temporary repo back here. As part of that migration we're closing all the old issues and PRs. We really appreciate you taking the time to investigate and report this issue!