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

Calling model#id is broken for Rails 7.2+ for composite primary keys #89

Open
andrepiske opened this issue Dec 19, 2024 · 1 comment · May be fixed by #88
Open

Calling model#id is broken for Rails 7.2+ for composite primary keys #89

andrepiske opened this issue Dec 19, 2024 · 1 comment · May be fixed by #88

Comments

@andrepiske
Copy link

Hello!
I found the #id method is broken in some scenarios when a model has include PgParty::Model. The way it breaks is that model_instance.id always returns nil. The scenario seems to be when the partitioned table has a composite primary key AND rails version is >= 7.2 (in my experiments, it breaks in 7.2 and 8.0 but works fine in 7.1).

A partitioned table like the one below will have this issue:

CREATE TABLE bigint_timestamp_ranges (
  id BIGSERIAL NOT NULL,
  updated_at TIMESTAMP WITHOUT TIME ZONE NOT NULL,
  created_at TIMESTAMP WITHOUT TIME ZONE NOT NULL,
  PRIMARY KEY (id, created_at)
) PARTITION BY RANGE (created_at);

CREATE TABLE bigint_timestamp_ranges_a
  PARTITION OF bigint_timestamp_ranges
  FOR VALUES FROM ('2024-12-19T00:00:00Z') TO ('2024-12-20T00:00:00Z');

CREATE TABLE bigint_timestamp_ranges_b
  PARTITION OF bigint_timestamp_ranges
  FOR VALUES FROM ('2024-12-20T00:00:00Z') TO ('2024-12-21T00:00:00Z');

(these exact partitions aren't important but I've included them for the sake of completeness)

I'll try to show the issue by having two PRs opened in a fork of pg_party off 1.9.0.

One PR is this one: andrepiske#1 -- I've added a BigintTimestampRange model and I'm creating its table a little differently than other specs. In the spec/dummy/db/schema.rb file, I'm dropping and re-creating indexes so that the table becomes just like the sql above. In this PR, the issue is fixed and the fix is in the lib/pg_party/model/shared_methods.rb file, by using self.primary_key = ... instead of @primary_key = .... I made a spec out of spec/integration/model/bigint_date_range_spec.rb (mostly copied and adjusted) to show the issue and the github action runs that spec file only. With the fix in, the spec passes: https://github.com/andrepiske/pg_party/actions/runs/12397946007

Then here is another PR removing the fix (it stems from that first PR above): andrepiske#2 -- the broken code is what's in pg_party main branch now. Here is a github action running it and failing for rails >= 7.2 but passing for 7.1: https://github.com/andrepiske/pg_party/actions/runs/12397977681

I opened a draft pull request with the fix in #88 but when I went for writing a spec for it, I wasn't sure really how to. It seems pg_party doesn't oficially support composite primary keys, so it'd be a larger overhaul of the gem to make it work, no?

It also seems that what introduced this issue is rails/rails#49808.

@marcoroth pinging you in case you have interest in this one, given your discussion in the PR above and your contribution in #83 -- which hopefully I'm not breaking with the changes I'm proposing here.


Sidenote: For what it's worth, I have a table pretty much like that in a gem I'm working on at https://github.com/trusted/iron_trail/blob/3b3fff4c/lib/generators/iron_trail/templates/create_irontrail_changes.rb.erb#L3-L17

I'm now thinking whether or not I should have that composite primary key at all. The reason it's composite is because postgres requires the partitioned column to be part of the index. Another option would be to have the primary key in the partitions only, that'd allow me to have a non-composite primary key and it'd make things easier -- especially since I'm not worried about duplicate ids given its values are generated from a sequence.

@andrepiske andrepiske linked a pull request Dec 19, 2024 that will close this issue
@rkrage
Copy link
Owner

rkrage commented Jan 4, 2025

I haven't actually tried this, but I assume you can get around the composite primary key thing by manually setting the primary key in your model (self.primary_key = :id)? As you pointed out, most of the time you simply have a composite primary key because Postgres requires the partitioned column to be a part of the key (like id and created_at), but from a Rails perspective you probably just need to use id as the PK.

As for a longterm fix, I'll take a look at your PR this week and will try to write some test cases.

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.

2 participants