Skip to content

reference schema name when creating index #3

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kgeis
Copy link

@kgeis kgeis commented Dec 9, 2020

fixes #2

Copy link

@brylie brylie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the README to document this feature.

@kgeis
Copy link
Author

kgeis commented Apr 13, 2021

This is a bug fix, not a feature. I don't understand how it would fit in the README.

@brylie
Copy link

brylie commented Apr 27, 2021

@drewbanin can we please get a review/approval for this bug fix?

@drewbanin
Copy link
Contributor

hey @jtcohen6 - do you think you could take a look at this one?

@brylie
Copy link

brylie commented Apr 27, 2021

It's also worth considering the impact native dbt Postgres index creation support will have on this package.

dbt-labs/dbt-core#804

@jtcohen6
Copy link

Right on — I just merged dbt-labs/dbt-core#3106, which adds native support for Postgres indexes in dbt-postgres. The tact we took there was, rather than namespacing indexes to specific schemas, creating a globally unique (hashed) name for each index based on the relation, properties, and creation timestamp.

If either of you think that's a mistake—that indexes should be namespaced to the same schema as the relation they're defined on—we can still change that before releasing v0.20.0.

In any case, I'm happy to approve and merge this PR as is, while also acknowledging that we should announce the deprecation of this package.

@brylie
Copy link

brylie commented Apr 27, 2021

I think the approach you took is fine, as long as the index names will still be human-readable and relatively concise. Agreed on deprecating this package unless it may serve as a foundation for other Postgres-specific functionality.

@jtcohen6
Copy link

@brylie The approach over in dbt-labs/dbt-core#3106 prioritizes global uniqueness and conciseness over human readability. (The index name is just a MD5 hash.) Could you say a bit more about your desire for human readability? It makes sense intuitively, and I want to make sure we're taking a balanced approach to these trade-offs.

From PostgreSQL docs: "No schema name can be included here; the index is always created in the same schema as its parent table."
@kgeis
Copy link
Author

kgeis commented Apr 27, 2021

It looks like you have a different approach, but I still updated my PR for correctness. It was not appropriate for me to reference a schema in the index name.

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 this pull request may close these issues.

index macro does not honor schema
4 participants