Fix has_been_used? for Postgres table names near the identifier length limit - #133
Open
dduugg wants to merge 4 commits into
Open
Fix has_been_used? for Postgres table names near the identifier length limit#133dduugg wants to merge 4 commits into
dduugg wants to merge 4 commits into
Conversation
…h limit
pre_count's has_been_used? guessed a table's sequence name as
"#{table}_id_seq" and looked it up with a naive pg_class match. Once that
guess exceeds Postgres' 63-byte NAMEDATALEN limit, Postgres silently
truncates it, so the guess can collide with the table's own name (or some
other relation) instead of the real, differently-truncated sequence name
Postgres actually generated. has_sequence? then false-positives on that
wrong relation, and the follow-up "SELECT last_value from ..." blows up
with `PG::UndefinedColumn: column "last_value" does not exist` since it's
querying a table, not a sequence.
Use pg_get_serial_sequence(table, 'id') instead of guessing, which is what
it's for. Falls back to has_rows? when the table has no "id" column at all
(pg_get_serial_sequence raises rather than returning nil for that case).
rescue ::ActiveRecord::StatementInvalid was catching any statement failure, not just the "no id column" case it's meant to handle, silently falling back to has_rows? for genuine SQL/connection errors too. Check the underlying PG::UndefinedColumn cause and re-raise anything else.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Truncation's Postgres adapter guesses a table's auto-increment sequence name as"#{table}_id_seq"and looks it up with a naivepg_classstring match. Once that guess exceeds Postgres' 63-byteNAMEDATALENlimit, Postgres silently truncates the identifier, so the guess can land on the table's own name (or some unrelated relation) instead of the real, differently-truncated sequence name Postgres actually generated for it.Concretely, for a table whose name is already at or near 63 characters:
has_sequence?then false-positives against the table's ownpg_classrow, andhas_been_used?'s follow-up query blows up:This is a regression surfaced by #103 (2.2.0): the old
currval(...)call was wrapped in a barerescuethat happened to swallow this case too; thelast_valuerewrite dropped that safety net.Fix
Use
pg_get_serial_sequence(table, 'id')— the Postgres builtin purpose-built for this — instead of string-guessing. It correctly resolves the real (possibly truncated) sequence name regardless of identifier length. Falls back tohas_rows?when the table has noidcolumn at all, sincepg_get_serial_sequenceraises rather than returningnilfor that case.Testing
Added a regression test under
truncation_spec.rb(Postgres-only) that creates a table with a name at the exact 63-byte limit and exercisespre_count'shas_been_used?path against it, both for a never-touched table and one with real activity. Ran the full spec suite locally against Postgres and SQLite (no regressions); didn't have MySQL/Trilogy available locally, but the change is scoped entirely toPostgreSQLAdapterso those adapters are untouched.Happy to adjust style/approach if you'd prefer a different fix shape.