Skip to content

added new index after changing order#907

Open
astelmashenko wants to merge 11 commits into
conductor-oss:mainfrom
astelmashenko:feature/poll-msgs-index
Open

added new index after changing order#907
astelmashenko wants to merge 11 commits into
conductor-oss:mainfrom
astelmashenko:feature/poll-msgs-index

Conversation

@astelmashenko
Copy link
Copy Markdown
Contributor

@astelmashenko astelmashenko commented Mar 23, 2026

Pull Request type

  • Bugfix

Changes in this PR

fix index after fixing shceduler polling query order #515
Before a query was:

                        + "    ORDER BY priority DESC, deliver_on, created_on "

and there was an index for that (check postgres-persistence/src/main/resources/db/migration_postgres/V7__new_qm_index_desc_priority.sql and probably index was incorrect also):

CREATE INDEX combo_queue_message ON queue_message USING btree (queue_name , priority desc, popped, deliver_on, created_on)

after query changes:

                        + "    ORDER BY deliver_on, priority DESC, created_on "

Please notice order of order by changed, so in this PR old index is deleted and new is created specifically to make poll query efficient:

CREATE INDEX combo_queue_message
    ON queue_message (queue_name, deliver_on ASC, priority DESC, created_on ASC)
    INCLUDE (message_id)
    WHERE popped = false;

Copy link
Copy Markdown
Contributor

@nthmost-orkes nthmost-orkes left a comment

Choose a reason for hiding this comment

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

Index design looks correct — the column order matches the current ORDER BY in PostgresQueueDAO (deliver_on, priority DESC, created_on), the partial index on popped = false is a real improvement over V7's approach of including popped as a regular column, and the INCLUDE (message_id) makes it a covering index for the CTE so no heap fetch needed. Good stuff.

One concern: the migration uses a plain CREATE INDEX, which takes a ShareLock on queue_message for the duration of the build and blocks all writes (queue pops and pushes) until it finishes. V9 uses DROP INDEX CONCURRENTLY / CREATE INDEX CONCURRENTLY specifically to avoid this on busy production systems. This migration should do the same.

The catch is that CONCURRENTLY can't run inside a transaction block, so you'd also need a -- flyway:nonTransactional comment at the top of the file (V9 uses CONCURRENTLY but is missing this header, which is actually a latent bug there). So the migration would look like:

-- flyway:nonTransactional
DROP INDEX CONCURRENTLY IF EXISTS combo_queue_message;

CREATE INDEX CONCURRENTLY combo_queue_message
ON queue_message (queue_name, deliver_on ASC, priority DESC, created_on ASC)
INCLUDE (message_id)
WHERE popped = false;

Otherwise this LGTM.

@nthmost-orkes
Copy link
Copy Markdown
Contributor

Index design looks correct — the column order matches the current ORDER BY in PostgresQueueDAO (deliver_on, priority DESC, created_on), the partial index on popped = false is a real improvement over V7's approach of including popped as a regular column, and the INCLUDE (message_id) makes it a covering index for the CTE so no heap fetch needed. Good stuff.

One concern: the migration uses a plain CREATE INDEX, which takes a ShareLock on queue_message for the duration of the build and blocks all writes (queue pops and pushes) until it finishes. V9 uses DROP INDEX CONCURRENTLY / CREATE INDEX CONCURRENTLY specifically to avoid this on busy production systems. This migration should do the same.

The catch is that CONCURRENTLY can't run inside a transaction block, so you'd also need a -- flyway:nonTransactional comment at the top of the file (V9 uses CONCURRENTLY but is missing this header, which is actually a latent bug there). So the migration would look like:

-- flyway:nonTransactional
DROP INDEX CONCURRENTLY IF EXISTS combo_queue_message;

CREATE INDEX CONCURRENTLY combo_queue_message
    ON queue_message (queue_name, deliver_on ASC, priority DESC, created_on ASC)
    INCLUDE (message_id)
    WHERE popped = false;

Otherwise this LGTM.

@astelmashenko
Copy link
Copy Markdown
Contributor Author

@nthmost-orkes , I added the header line, can we merge it now?

@nthmost-orkes
Copy link
Copy Markdown
Contributor

@nthmost-orkes , I added the header line, can we merge it now?

Yes -- sorry to overlook this, just need to run tests and i'll pull this in.

@nthmost-orkes nthmost-orkes self-requested a review April 7, 2026 22:45
Copy link
Copy Markdown
Contributor

@nthmost-orkes nthmost-orkes left a comment

Choose a reason for hiding this comment

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

LGTM

@astelmashenko
Copy link
Copy Markdown
Contributor Author

@nthmost-orkes , it's all green now, how do I merge? should it merge automatically?

@nthmost-orkes
Copy link
Copy Markdown
Contributor

@nthmost-orkes , it's all green now, how do I merge? should it merge automatically?

I think we have a flaky test blocking this from passing

@nthmost-orkes
Copy link
Copy Markdown
Contributor

The build CI failure is caused by a Flyway migration version conflict, not a flake.

Root cause: Found more than one migration with version 14

This PR adds V14__optimize_queue_message_pop_index.sql, but V14__parent_workflow_id.sql already exists on main. Flyway requires unique version numbers across all migrations, so the Spring context fails to start — taking down every Postgres test class (PostgresIndexDAOTest, PostgresExecutionDAOTest, PostgresConfigurationDataMigrationTest, etc.).

Fix: Rename the new migration to the next available version:

V15__optimize_queue_message_pop_index.sql

(Current highest is V14__parent_workflow_id.sql; there is no V15 yet.)

@astelmashenko
Copy link
Copy Markdown
Contributor Author

@nthmost-orkes , fixed name, CI build is ok now.

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.

4 participants