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

Add support for identity columns #163

Closed
bheemvennapureddy opened this issue Aug 29, 2024 · 18 comments · Fixed by #164
Closed

Add support for identity columns #163

bheemvennapureddy opened this issue Aug 29, 2024 · 18 comments · Fixed by #164

Comments

@bheemvennapureddy
Copy link

-- Table: dbo.Url

-- DROP TABLE IF EXISTS dbo.Url;

CREATE TABLE dbo."Url"
(
    "URL_KEY" BIGINT PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY,
    "DOMAIN" VARCHAR(100) NOT NULL,
    "PATH" VARCHAR(1000) NOT NULL,
    "HASH" VARCHAR(50) UNIQUE
);

CREATE INDEX ix_url_ix_hash
    ON dbo."Url" USING btree ("HASH");

creates

Screenshot 2024-08-29 at 1 20 34 AM

where as it should create

-- Table: dbo.Url

-- DROP TABLE IF EXISTS dbo."Url";

CREATE TABLE IF NOT EXISTS dbo."Url"
(
    "URL_KEY" bigint NOT NULL GENERATED BY DEFAULT AS IDENTITY ( INCREMENT 1 START 1 MINVALUE 1 MAXVALUE 9223372036854775807 CACHE 1 ),
    "DOMAIN" character varying(100) COLLATE pg_catalog."default" NOT NULL,
    "PATH" character varying(1000) COLLATE pg_catalog."default" NOT NULL,
    "HASH" character varying(50) COLLATE pg_catalog."default",
    CONSTRAINT "Url_pkey" PRIMARY KEY ("URL_KEY"),
    CONSTRAINT "Url_HASH_key" UNIQUE ("HASH")
)

TABLESPACE pg_default;

ALTER TABLE IF EXISTS dbo."Url"
    OWNER to postgres;
-- Index: ix_url_ix_hash

-- DROP INDEX IF EXISTS dbo.ix_url_ix_hash;

CREATE INDEX IF NOT EXISTS ix_url_ix_hash
    ON dbo."Url" USING btree
    ("HASH" COLLATE pg_catalog."default" ASC NULLS LAST)
    TABLESPACE pg_default;
@Navbryce
Copy link

Generated isn't supported yet but serials and sequences are. I recommend using one of those instead.

@bheemvennapureddy
Copy link
Author

bheemvennapureddy commented Aug 29, 2024

serials are not recommended by Postgres itself https://wiki.postgresql.org/wiki/Don%27t_Do_This#Don.27t_use_serial. postgres recommends using Identity for new ones. so i am not sure everyone else will be happy using others.

@Navbryce
Copy link

Many of the reasons listed there are schema management, which this tool papers over for you.

@bheemvennapureddy
Copy link
Author

So you recommend using serial over generated identity?

@bplunkett-stripe
Copy link
Collaborator

No, I'm just saying more to unblock yourself, it might make sense to use serials instead of generated values until support is added.

@bplunkett-stripe bplunkett-stripe changed the title GENERATED BY DEFAULT AS IDENTITY doesn't work Add support for generated columns Aug 29, 2024
@bheemvennapureddy
Copy link
Author

We will be rolling this app soon so we might start writing data soon as well so changing types again will be a problem

@bplunkett-stripe
Copy link
Collaborator

A serial column is probably naively coercible to a generated value (worth checking). You would want to start the new generated column at an offset higher than the current value (with a significant buffer to account for a race).

Alternatively, you could try to wait for me to implement this. Unfortunately, I have pretty limited time and implementing new features is not a top priority.

@bheemvennapureddy
Copy link
Author

Can you throw some insights for me so I can give it a shot if you are open to accept pull requests ?

@bplunkett-stripe
Copy link
Collaborator

bplunkett-stripe commented Aug 30, 2024

I can probably knock this out today. Let me try to throw a PR up! (for identity columns).

@bplunkett-stripe bplunkett-stripe changed the title Add support for generated columns Add support for identity columns Aug 30, 2024
@bplunkett-stripe
Copy link
Collaborator

PR is ready to go! Probably will get a review on Monday and in some time that day. In in the interim, you can try it out.

@bheemvennapureddy
Copy link
Author

PR is ready to go! Probably will get a review on Monday and in some time that day. In in the interim, you can try it out.

that worked

Screenshot 2024-08-31 at 6 32 05 AM

@bheemvennapureddy
Copy link
Author

@Navbryce for some reason the identity columns are missing nextval() from information schema rather they take a random id for insert, any insights on this ?

@bplunkett-stripe
Copy link
Collaborator

I mean is the column created correctly by pg-schema-diff, or does this appear to be a Postgres issue?

@bheemvennapureddy
Copy link
Author

I don't know whats going wrong - i have created the table and column by pg-schema-diff and the information schema is missing the nextval for that

Screenshot 2024-10-01 at 9 57 07 PM Screenshot 2024-10-01 at 9 57 30 PM

column default is null for this table

@bheemvennapureddy
Copy link
Author

Screenshot 2024-10-01 at 10 02 06 PM Screenshot 2024-10-01 at 10 02 14 PM Screenshot 2024-10-01 at 10 02 29 PM

@bheemvennapureddy
Copy link
Author

Probably looks like the bulk inserts are causing issue with seq falling behind

@bheemvennapureddy
Copy link
Author

Sorry my bad it was the bulk copy which was causing the issue.

@bplunkett-stripe
Copy link
Collaborator

No worries! If the generated SQL is correct (schema), then the problem is within the application or Postgres. pg-schema-diff doesn't (directly) affect the actual "data plane" of postgres.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants