-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
tables: Match column attributes to usage #6122
Conversation
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.
Reading through these, I see another one -- The rpm_packages.table
should probably set additional on version. release, epoch.
@@ -4,7 +4,7 @@ schema([ | |||
Column("type", TEXT, "Type of crash log"), | |||
Column("pid", BIGINT, "Process (or thread) ID of the crashed process"), | |||
Column("path", TEXT, "Path to the crashed process"), | |||
Column("crash_path", TEXT, "Location of log file"), | |||
Column("crash_path", TEXT, "Location of log file", index=True), |
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.
I don't see this in the c++ implementation, and looking at my machine, I think it's incorrect from a primary key perspective.
crash_path
could, maybe, be a primary key.
@@ -2,8 +2,8 @@ table_name("docker_container_processes") | |||
description("Docker container processes.") | |||
schema([ | |||
Column("id", TEXT, "Container ID", index=True, required=True), | |||
Column("pid", BIGINT, "Process ID"), | |||
Column("name", TEXT, "The process path or shorthand argv[0]", index=True), | |||
Column("pid", BIGINT, "Process ID", index=True), |
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.
I don't see this used as an index. Maybe additional?
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.
The combination of id
and pid
are the PRIMARY KEY
for this table.
Thanks for the review, I will make some follow up changes. Like you pointed out, I intentionally added several |
2734975
to
5e2e171
Compare
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.
lgtm
I did another audit of table implementations and how they use column constraints. These changes seem to match how I saw the constraints used and what makes sense from a primary-key == index perspective.