-
-
Notifications
You must be signed in to change notification settings - Fork 758
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
fix drizzle-kit push
crash on compound indexes with expression members
#3888
base: main
Are you sure you want to change the base?
Conversation
drizzle-kit push
on schemas with compound indexes with expression membersdrizzle-kit push
crash on pulling db schema with compound indexes that have expression members
also fixes #3062 |
CASE | ||
WHEN i.indkey[k.i-1] != 0 THEN a.attname | ||
ELSE pg_get_indexdef(i.indexrelid, k.i, true) | ||
END AS column_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any cases when a.attname
and the returned value from pg_get_indexref
are different for simple columns? pg_get_indexref
returns the same values that the index was created with, couldn't this be simplified to just pg_get_indexdef(i.indexrelid, k.i, true) as column_name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly— I have no idea!
This SQL was generated by Claude and seems to work for my use case. I can happily debug through the Typescript but my knowledge of PG introspection/internals is weak.
I'm hoping a reviewer with knowledge of pg details can make a proper assessment of the Claude-generated SQL changes— entirely possible your simplification is valid.
for simple columns
this is a fix for compound indexes where at least one component is an expression (ie not a simple column). Not sure if that affects your answer or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "simple columns" I meant references to columns (without expressions), such as source_language
in your example. Both the original and your refactored solution work for those cases, I was just wondering if we could make it cleaner and easier to read without the conditional and array indexing 😄 Other than that, it's essentially the same patch as I made on our project and hadn't had any issues so far, hope a maintainer can review it soon!
…on (non-column) members fix drizzle-team#3745
drizzle-kit push
crash on pulling db schema with compound indexes that have expression membersdrizzle-kit push
crash on compound indexes with expression members
Related: SQL generated on introspect: But on push it wants to do: I think there's a bug in how |
fix #3745
See my comment #3745 (comment)
drizzle-kit push
crashes with confusing Zod errorExpected string, received null
when making incremental updates to DBs that have tables likewhere there's an index like
(col, expression(col))
such that one of the index components isexpression(col)
, ie. not just a plain column.The crash will not happen when the
translations
table is first added to the schema anddrizzle-kit push
ed. The crash will only happen on subsequentdrizzle-kit push
es, whentranslation
's index is already in the db.This happens because
drizzle-kit push
first reads the existing schema of the db, diffs it against the schema in typescript, and makes the updates necessary. The bug is in reading from the db. If drizzle-kit encounters a compound index with an expression (non-column) component, the db schema introspection code returns null for that component.Later, the schema introspected out of the running db is passed to Zod for validation, which is surprised at a
null
where it expects a sql string defining the index's nth component.The inappropriate null is constructed in
fromDatabase
, where:The
dbIndexes
it pulls from postgres has acolumn_name=NULL
for the row withindexname='tnsltn', index_order=2
, where it ought to havecolumn_name='md5((source_text)::text)'
.I confirmed this in my project, which has a table with similar index to that of the original bug reporter. I worked with Claude to generate this new SQL query. I confirmed that this changed SQL query produced by Claude works for my project.
However, I'm personally not great at SQL, so do very much take this with a grain of salt. I can't meaningfully read the original query, nor the one Claude wrote.
Hopefully someone with better SQL can read and give feedback!