Skip to content
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(memory-store): Remove bad indices #1039

Merged
merged 1 commit into from
Jan 11, 2025
Merged

fix(memory-store): Remove bad indices #1039

merged 1 commit into from
Jan 11, 2025

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Jan 11, 2025

User description

Signed-off-by: Diwank Singh Tomer [email protected]


PR Type

Bug fix


Description

  • Removed creation of redundant and problematic indices in transitions table.

  • Retained necessary index creation for metadata using GIN.

  • Simplified SQL migration script for better maintainability.


Changes walkthrough 📝

Relevant files
Bug fix
000012_transitions.up.sql
Removed problematic indices and simplified migration script

memory-store/migrations/000012_transitions.up.sql

  • Removed creation of three problematic indices.
  • Retained creation of idx_transitions_metadata index.
  • Simplified SQL script by removing unnecessary checks.
  • +0/-14   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information


    Important

    Remove specific indices from 000012_transitions.up.sql related to current_step, next_step, and step_label.

    • Indices Removal:
      • Removed idx_transitions_current, idx_transitions_next, and idx_transitions_label indices from 000012_transitions.up.sql.
      • These indices were on execution_id, current_step, next_step, and step_label columns.
    • Misc:
      • No other changes were made to the file or functionality.

    This description was created by Ellipsis for 6d65d21. It will automatically update as commits are pushed.

    Signed-off-by: Diwank Singh Tomer <[email protected]>
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Index Removal Impact

    Removing unique indices on current_step, next_step and step_label could impact query performance and data integrity constraints. Verify that these indices were indeed problematic and their removal won't negatively affect application functionality.

    IF NOT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = 'idx_transitions_metadata') THEN
        CREATE INDEX idx_transitions_metadata ON transitions USING GIN (metadata);
    END IF;

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    👍 Looks good to me! Reviewed everything up to 6d65d21 in 8 seconds

    More details
    • Looked at 25 lines of code in 1 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. memory-store/migrations/000012_transitions.up.sql:70
    • Draft comment:
      The removal of indices idx_transitions_current, idx_transitions_next, and idx_transitions_label may impact query performance if these columns are frequently queried. Ensure that the remaining indices and constraints meet the application's performance requirements.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The removal of the indices seems intentional, but it's important to ensure that the remaining indices and constraints are sufficient for the application's needs.

    Workflow ID: wflow_pIhwbwoZW1j4CNn4


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add a basic index to improve query performance on a frequently used column

    Consider adding a non-unique index on execution_id to optimize queries filtering or
    joining on this column, as it appears to be a frequently used field in the
    transitions table.

    memory-store/migrations/000012_transitions.up.sql [71-76]

     DO $$
     BEGIN
         IF NOT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = 'idx_transitions_metadata') THEN
             CREATE INDEX idx_transitions_metadata ON transitions USING GIN (metadata);
         END IF;
    +
    +    IF NOT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = 'idx_transitions_execution_id') THEN
    +        CREATE INDEX idx_transitions_execution_id ON transitions(execution_id);
    +    END IF;
     END $$;
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While adding an index on execution_id could improve query performance, the PR explicitly removes several execution_id-based indices, suggesting a deliberate decision to reduce index overhead. The suggestion contradicts the PR's intent of simplifying the indexing strategy.

    3

    @creatorrr creatorrr merged commit 0509485 into dev Jan 11, 2025
    5 checks passed
    @creatorrr creatorrr deleted the x/remove-bad-indices branch January 11, 2025 10:01
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant