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}")