Fixes #38949 - Change logs_id_seq sequence to bigint

[nacoool]

No description provided.

[adamruzicka]

Please use two spaces per indentation level.

What would happen in the down direction if the current value was already outside the integer range?

[ekohl]

In addition to @adamruzicka's questions I'm looking at other places where a simlar change happend. When I look at db/migrate/20170110113824_change_id_value_range.rb then it changes 3 tables. Shouldn't we migrate all 3 to bigint? And in db/migrate/20200217110708_alter_session_sequence_to_cycle.rb it does the same for sessions so isn't that also affected?

What would happen in the down direction if the current value was already outside the integer range?

FYI: it's also possible to make the migration non-reversible, which is probably fine because I don't think we really reverse migrations.

[nacoool]

In addition to @adamruzicka's questions I'm looking at other places where a simlar change happend. When I look at db/migrate/20170110113824_change_id_value_range.rb then it changes 3 tables. Shouldn't we migrate all 3 to bigint? And in db/migrate/20200217110708_alter_session_sequence_to_cycle.rb it does the same for sessions so isn't that also affected?

Do we also want to make similar change for reports and fact_values tables as well?

[ekohl]

Do we also want to make similar change for reports and fact_values tables as well?

It makes sense to me. @adamruzicka?

In https://community.theforeman.org/t/pg-error-nextval-reached-maximum-value-of-sequence-logs-id-seq-2147483647/45095 there's some related discussion about setting the start of the sequence. Have you had a look at converting a table with existing entries?

[adamruzicka]

Do we also want to make similar change for reports and fact_values tables as well?

It makes sense to me. @adamruzicka?

Yup, those can get pretty big

[ekohl]

@nacoool have you done any investigation about the start of the sequence? Is that needed?

[nacoool]

@ekohl Yes, I looked into it. The start value is only relevant when a sequence is created or explicitly restarted. For existing sequences, PostgreSQL continues from the current last_value, so the start value itself isn’t needed here. What does matter is ensuring the current value is valid after converting to bigint, which is why resetting it to MAX(id) is the safe approach. I’ll update the migration accordingly.

[nacoool]

@ekohl
I have retested on local and after converting to bigint, It picks next value from current max value.
I think we should not change it.

[ekohl]

Thanks for checking that. A note for next time: please copy paste text instead of making a screenshot when possible. That makes it easier to read.

[ekohl]

I don't think there's a guarantee that the table name is the same as the sequence. In the ActiveRecord code I see you can at least use connection.default_sequence_name(table_name, primary_key) with the latter argument being optional. https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/DatabaseStatements.html#method-i-default_sequence_name doesn't have a good description, but for PostgreSQL it'll do the right thing: return the sequence name for a column.

So I'd suggest:

Suggested change

	def change_sequence(name, as:)
	connection.execute("ALTER SEQUENCE #{connection.quote_table_name(name)} AS #{as}")
	def change_sequence(table_name, as:)
	connection.execute("ALTER SEQUENCE #{connection.default_sequence_name(table_name)} AS #{as}")

[nacoool]

@ekohl
Table name can be different than sequence. then we can use below to fetch sequence

# pg_get_serial_sequence queries the database for the real sequence name
sequence_name = conn.select_value(
  "SELECT pg_get_serial_sequence(#{conn.quote('logs')}, 'id')"
)

# Remove schema prefix if present (e.g., "public.logs_id_seq" -> "logs_id_seq")
sequence_name = sequence_name.split('.').last if sequence_name&.include?('.')

doc: pg_get_serial_sequence
https://www.postgresql.org/docs/16/functions-info.html#FUNCTIONS-INFO-CATALOG-TABLE-PG-GET-SERIAL-SEQUENCE

default_sequence_name will add _id_seq to table name.

[3] pry(main)> conn.default_sequence_name(:logs)
=> "public.logs_id_seq"
[4] pry(main)> conn.default_sequence_name(:logs_id_seq)
=> "logs_id_seq_id_seq"

What do you think?

[ekohl]

I think it's better to refactor the migration to be based on table names instead of sequence names because all other migrations are also based on the table names. Under the hood default_sequence_name uses pg_get_serial_sequence (see https://github.com/rails/rails/blob/f4321f78fe567fd29dd0a0fb5cb17af68efd36bb/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L246-L257) so that should already be safe.

[nacoool]

Thank You for clarification. I have made changes as suggested.

[ekohl]

Code wise 👍 but I think it's good to mention somewhere why this is needed.

@adamruzicka would you mind reviewing since there were some changes I suggested.

[ekohl]

I'd leave out this empty line

Suggested change