stripe / pg-schema-diff

Go library for diffing Postgres schemas and generating SQL migrations
MIT License
270 stars 20 forks source link

Generated SQL for new table reorders columns #118

Closed burke1791 closed 3 months ago

burke1791 commented 3 months ago

When generating a plan that includes a new table, the generated CREATE TABLE ... statement has the columns in a different order than the .sql file. For example, given this sql file:

CREATE TABLE public.user (
    user_id INT NOT NULL,
    username TEXT NOT NULL,
    user_smallint SMALLINT NOT NULL,
    created_date TIMESTAMP NOT NULL,
    test_col Interval NULL
);

Running pg-schema-diff plan ... results in the following generated SQL statement (tested on both pg14 and pg15):

################################ Generated plan ################################
1. CREATE TABLE "public"."user" (
        "test_col" interval,
        "created_date" timestamp without time zone NOT NULL,
        "user_id" integer NOT NULL,
        "user_smallint" smallint NOT NULL,
        "username" text COLLATE "pg_catalog"."default" NOT NULL
);
        -- Statement Timeout: 3s

The columns appear to be sorted in descending order based on their data type size.

bplunkett-stripe commented 3 months ago

Correct! This is designed to minimize the padding that Postgres uses.

This is actually a toggle-able behavior that we have just not plumbed through the CLI. I put up a PR to plumb it through.

burke1791 commented 3 months ago

Awesome, thanks for the info. I'd argue column re-ordering should be off by default because storage details like this should be up to the DBA instead of the source control tool. For example, if a nullable column is expected to be sparse, then force re-ordering might actually be worse than the written order.

bplunkett-stripe commented 3 months ago

I see! That makes sense. For now, I'll leave it as enabled by-default, but if enough folks take issue with it, then I will switch it to disabled. I'm honestly too sure how much folks think about data-packing tables. I was of the opinion that most folks might not be aware of it, but maybe they are?