Add the missing Columns in Elasticsearch table#112
Conversation
|
📝 WalkthroughWalkthroughA new database migration adds columns ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/resources/db/migration/dbidentity/V12__Elasticsearch_Columns.sql`:
- Around line 46-52: The ALTER statement incorrectly narrows current_offset to
INT and drops NOT NULL on counters; update the ALTER for table
db_identity.t_elasticsearch_sync_job so that current_offset is BIGINT NOT NULL
DEFAULT 0 and the counter columns total_records, processed_records,
success_count, and failure_count are defined as BIGINT NOT NULL DEFAULT 0 (leave
status as VARCHAR(50) NOT NULL DEFAULT 'PENDING') to restore the original types
and NOT NULL constraints.
- Around line 19-30: The migration currently adds created_date as NOT NULL with
DEFAULT CURRENT_TIMESTAMP which stamps existing rows with the migration time;
instead, first add created_date as a nullable column (use the same
`@preparedStatement` / PREPARE stmt pattern to execute an ALTER TABLE ... ADD
COLUMN created_date TIMESTAMP NULL AFTER triggered_by), then run an UPDATE to
backfill created_date from started_at (e.g., UPDATE <table> SET created_date =
started_at WHERE created_date IS NULL), and finally ALTER TABLE to make
created_date NOT NULL with DEFAULT CURRENT_TIMESTAMP and then create
idx_created_date; keep using the same `@preparedStatement/PREPARE` stmt/EXECUTE
stmt/DEALLOCATE PREPARE pattern around each DDL/UPDATE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a0aab65-1f7d-43c0-addc-b84975ced6c1
📒 Files selected for processing (1)
src/main/resources/db/migration/dbidentity/V12__Elasticsearch_Columns.sql
| -- Add created_date | ||
| SET @columnname = 'created_date'; | ||
| SET @preparedStatement = (SELECT IF( | ||
| (SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS | ||
| WHERE TABLE_SCHEMA = @dbname AND TABLE_NAME = @tablename AND COLUMN_NAME = @columnname) > 0, | ||
| 'SELECT 1', | ||
| CONCAT('ALTER TABLE ', @dbname, '.', @tablename, | ||
| ' ADD COLUMN created_date TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP AFTER triggered_by;') | ||
| )); | ||
| PREPARE stmt FROM @preparedStatement; | ||
| EXECUTE stmt; | ||
| DEALLOCATE PREPARE stmt; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full context of both migration files
cd src/main/resources/db/migration/dbidentity && ls -la V*.sql | head -20Repository: PSMRI/AMRIT-DB
Length of output: 1026
🏁 Script executed:
# Check the V7 migration to understand the existing schema
cat src/main/resources/db/migration/dbidentity/V7__ELASTICSEARCH.sqlRepository: PSMRI/AMRIT-DB
Length of output: 952
🏁 Script executed:
# Check the V12 migration file in full
cat src/main/resources/db/migration/dbidentity/V12__Elasticsearch_Columns.sqlRepository: PSMRI/AMRIT-DB
Length of output: 4695
🌐 Web query:
MySQL ADD COLUMN NOT NULL DEFAULT CURRENT_TIMESTAMP existing rows behavior
💡 Result:
When adding a column to an existing MySQL table using ALTER TABLE ADD COLUMN with NOT NULL and DEFAULT CURRENT_TIMESTAMP, the existing rows are populated with the default value, which is the current timestamp at the time the ALTER statement executes.
Citations:
- 1: https://stackoverflow.com/questions/22868345/mysql-add-a-not-null-column
- 2: https://dev.mysql.com/doc/refman/en/data-type-defaults.html
- 3: https://dev.mysql.com/doc/refman/en/alter-table.html
- 4: https://dev.mysql.com/doc/en/timestamp-initialization.html
Backfill created_date from started_at before making it non-null.
Line 26 will stamp every pre-existing job record with the migration execution time, not the row's original start time. When MySQL executes ADD COLUMN created_date TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, all existing rows are populated with the current timestamp at that moment—not the historical started_at value. This causes two problems: (1) historical chronology is lost, and (2) the idx_created_date index will be built on incorrect timestamps.
Add created_date as nullable first, backfill from started_at, then alter it to NOT NULL:
Suggested fix
- ' ADD COLUMN created_date TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP AFTER triggered_by;')
+ ' ADD COLUMN created_date TIMESTAMP NULL AFTER triggered_by;')
));
PREPARE stmt FROM `@preparedStatement`;
EXECUTE stmt;
DEALLOCATE PREPARE stmt;
+
+UPDATE db_identity.t_elasticsearch_sync_job
+SET created_date = COALESCE(started_at, CURRENT_TIMESTAMP)
+WHERE created_date IS NULL;
+
+ALTER TABLE db_identity.t_elasticsearch_sync_job
+MODIFY COLUMN created_date TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- Add created_date | |
| SET @columnname = 'created_date'; | |
| SET @preparedStatement = (SELECT IF( | |
| (SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS | |
| WHERE TABLE_SCHEMA = @dbname AND TABLE_NAME = @tablename AND COLUMN_NAME = @columnname) > 0, | |
| 'SELECT 1', | |
| CONCAT('ALTER TABLE ', @dbname, '.', @tablename, | |
| ' ADD COLUMN created_date TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP AFTER triggered_by;') | |
| )); | |
| PREPARE stmt FROM @preparedStatement; | |
| EXECUTE stmt; | |
| DEALLOCATE PREPARE stmt; | |
| -- Add created_date | |
| SET `@columnname` = 'created_date'; | |
| SET `@preparedStatement` = (SELECT IF( | |
| (SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS | |
| WHERE TABLE_SCHEMA = `@dbname` AND TABLE_NAME = `@tablename` AND COLUMN_NAME = `@columnname`) > 0, | |
| 'SELECT 1', | |
| CONCAT('ALTER TABLE ', `@dbname`, '.', `@tablename`, | |
| ' ADD COLUMN created_date TIMESTAMP NULL AFTER triggered_by;') | |
| )); | |
| PREPARE stmt FROM `@preparedStatement`; | |
| EXECUTE stmt; | |
| DEALLOCATE PREPARE stmt; | |
| UPDATE db_identity.t_elasticsearch_sync_job | |
| SET created_date = COALESCE(started_at, CURRENT_TIMESTAMP) | |
| WHERE created_date IS NULL; | |
| ALTER TABLE db_identity.t_elasticsearch_sync_job | |
| MODIFY COLUMN created_date TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/db/migration/dbidentity/V12__Elasticsearch_Columns.sql`
around lines 19 - 30, The migration currently adds created_date as NOT NULL with
DEFAULT CURRENT_TIMESTAMP which stamps existing rows with the migration time;
instead, first add created_date as a nullable column (use the same
`@preparedStatement` / PREPARE stmt pattern to execute an ALTER TABLE ... ADD
COLUMN created_date TIMESTAMP NULL AFTER triggered_by), then run an UPDATE to
backfill created_date from started_at (e.g., UPDATE <table> SET created_date =
started_at WHERE created_date IS NULL), and finally ALTER TABLE to make
created_date NOT NULL with DEFAULT CURRENT_TIMESTAMP and then create
idx_created_date; keep using the same `@preparedStatement/PREPARE` stmt/EXECUTE
stmt/DEALLOCATE PREPARE pattern around each DDL/UPDATE.
| ALTER TABLE db_identity.t_elasticsearch_sync_job | ||
| MODIFY COLUMN status VARCHAR(50) NOT NULL DEFAULT 'PENDING', | ||
| MODIFY COLUMN total_records BIGINT DEFAULT 0, | ||
| MODIFY COLUMN processed_records BIGINT DEFAULT 0, | ||
| MODIFY COLUMN success_count BIGINT DEFAULT 0, | ||
| MODIFY COLUMN failure_count BIGINT DEFAULT 0, | ||
| MODIFY COLUMN current_offset INT DEFAULT 0, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "V7__ELASTICSEARCH.sql" src/Repository: PSMRI/AMRIT-DB
Length of output: 122
🏁 Script executed:
fd "V12__Elasticsearch_Columns.sql" src/Repository: PSMRI/AMRIT-DB
Length of output: 131
🏁 Script executed:
cat -n src/main/resources/db/migration/dbidentity/V7__ELASTICSEARCH.sqlRepository: PSMRI/AMRIT-DB
Length of output: 1092
🏁 Script executed:
cat -n src/main/resources/db/migration/dbidentity/V12__Elasticsearch_Columns.sqlRepository: PSMRI/AMRIT-DB
Length of output: 5563
Fix the counter column types and constraints in this ALTER statement.
Line 52 narrows current_offset from BIGINT to INT; the original V7 schema explicitly defined it as BIGINT NOT NULL DEFAULT 0 to prevent offset overflow on large jobs. The same block also removes NOT NULL from all five counter columns (lines 48–51), which violates the original schema design. On large sync jobs, the INT offset can overflow, and missing NOT NULL constraints allow unexpected NULL values in counter fields.
🔧 Restore original types and NOT NULL constraints
ALTER TABLE db_identity.t_elasticsearch_sync_job
MODIFY COLUMN status VARCHAR(50) NOT NULL DEFAULT 'PENDING',
- MODIFY COLUMN total_records BIGINT DEFAULT 0,
- MODIFY COLUMN processed_records BIGINT DEFAULT 0,
- MODIFY COLUMN success_count BIGINT DEFAULT 0,
- MODIFY COLUMN failure_count BIGINT DEFAULT 0,
- MODIFY COLUMN current_offset INT DEFAULT 0,
+ MODIFY COLUMN total_records BIGINT NOT NULL DEFAULT 0,
+ MODIFY COLUMN processed_records BIGINT NOT NULL DEFAULT 0,
+ MODIFY COLUMN success_count BIGINT NOT NULL DEFAULT 0,
+ MODIFY COLUMN failure_count BIGINT NOT NULL DEFAULT 0,
+ MODIFY COLUMN current_offset BIGINT NOT NULL DEFAULT 0,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ALTER TABLE db_identity.t_elasticsearch_sync_job | |
| MODIFY COLUMN status VARCHAR(50) NOT NULL DEFAULT 'PENDING', | |
| MODIFY COLUMN total_records BIGINT DEFAULT 0, | |
| MODIFY COLUMN processed_records BIGINT DEFAULT 0, | |
| MODIFY COLUMN success_count BIGINT DEFAULT 0, | |
| MODIFY COLUMN failure_count BIGINT DEFAULT 0, | |
| MODIFY COLUMN current_offset INT DEFAULT 0, | |
| ALTER TABLE db_identity.t_elasticsearch_sync_job | |
| MODIFY COLUMN status VARCHAR(50) NOT NULL DEFAULT 'PENDING', | |
| MODIFY COLUMN total_records BIGINT NOT NULL DEFAULT 0, | |
| MODIFY COLUMN processed_records BIGINT NOT NULL DEFAULT 0, | |
| MODIFY COLUMN success_count BIGINT NOT NULL DEFAULT 0, | |
| MODIFY COLUMN failure_count BIGINT NOT NULL DEFAULT 0, | |
| MODIFY COLUMN current_offset BIGINT NOT NULL DEFAULT 0, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/db/migration/dbidentity/V12__Elasticsearch_Columns.sql`
around lines 46 - 52, The ALTER statement incorrectly narrows current_offset to
INT and drops NOT NULL on counters; update the ALTER for table
db_identity.t_elasticsearch_sync_job so that current_offset is BIGINT NOT NULL
DEFAULT 0 and the counter columns total_records, processed_records,
success_count, and failure_count are defined as BIGINT NOT NULL DEFAULT 0 (leave
status as VARCHAR(50) NOT NULL DEFAULT 'PENDING') to restore the original types
and NOT NULL constraints.
1 participant
📋 Description
JIRA ID:
NA
✅ Type of Change
Summary by CodeRabbit
Performance
Chores