Draft
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Contributor
WalkthroughIntroduces comprehensive partition lifecycle management for PostgreSQL and database utilities for schema verification, data migration, and partition coverage validation. Integrates partition management into the prowloader workflow and provides extensive APIs with documentation and examples. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Member
|
/cc |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
pkg/dataloader/prowloader/prow.go (1)
361-374: Confirm the hard‑coded retention windows.The 90‑day detach + 100‑day drop directly controls data deletion. Please confirm this matches the desired retention policy; if it should vary by environment, consider making it configurable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dataloader/prowloader/prow.go` around lines 361 - 374, The function agePartitionsForDailyTestAnalysisByJob currently uses hard-coded retention windows (90 for DetachOldPartitions and 100 for DropOldDetachedPartitions) which control data deletion; make these retention values configurable instead of fixed literals: add configurable fields or constants (e.g., ProwLoader.detachRetentionDays and ProwLoader.dropRetentionDays or read from environment/config) and replace the literal 90 and 100 in the calls to partitions.DetachOldPartitions and partitions.DropOldDetachedPartitions, ensure the fields are initialized when ProwLoader is constructed (with sensible defaults matching current behavior), and update the log messages to include the configured retention values so behavior is clear per environment.pkg/db/partitions/examples.go (1)
1-760: Avoid exporting Example helpers in production builds.Because this file isn’t
*_test.go, allExample...functions become part of the public package API and are compiled into binaries. If these are documentation-only, move them to anexamples_test.go(or add a build tag) to prevent API bloat.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/partitions/examples.go` around lines 1 - 760, The file defines many Example... functions (e.g., ExampleListPartitionedTables, ExampleListPartitions, ExampleGetStats, ExampleComparePartitionStats, ExampleCheckRetentionPolicy, ExampleDryRunCleanup, ExampleDetachedPartitions, ExampleAttachedPartitions, ExampleDropOldDetachedPartitions, ExampleDetachWorkflow, ExampleReattachPartition, ExampleCreateMissingPartitions, ExampleCreatePartitionedTable, ExampleUpdatePartitionedTable, ExampleWorkflowForAnyTable, ExampleCompleteWorkflow) that are exported in production builds; move all these Example* helper functions out of the non-test package file by either (A) relocating them into a new examples_test.go (or examples_test.go in the same package) so they are only compiled for tests/documentation, or (B) add an appropriate build constraint (e.g., //go:build examples) at the top of this file and adjust CI to avoid building that tag by default—pick one approach and apply it consistently to all Example* functions to prevent API bloat.pkg/db/utils_example.go (1)
1-687: Avoid exporting Example helpers in production builds.Since this isn’t a
*_test.go, theseExample...functions become exported API and ship in production binaries. If they’re documentation-only, consider moving to anexamples_test.go(or using a build tag).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/utils_example.go` around lines 1 - 687, The Example* helper functions (e.g., ExampleVerifyTablesHaveSameColumns, ExampleVerifyPartitionMatchesParent, ExampleMigrateTableData, ExampleMigrateTableDataRange, ExampleIncrementalMigrationByMonth, etc.) are defined in a non-test file and will be included in production builds; move all Example... functions out of pkg/db/utils_example.go into a new test file (e.g., pkg/db/utils_example_test.go) so they are only compiled for tests/documentation, or alternatively add an appropriate build tag to exclude them from production. To fix: create the new *_test.go file and copy every function named Example... (referencing types/methods like DB.VerifyTablesHaveSameColumns, DB.MigrateTableData, DB.MigrateTableDataRange, DB.SyncIdentityColumn, DB.GetPartitionStrategy, DB.VerifyPartitionCoverage, etc.) into it (keeping package db or db_test as desired), then remove the Example... definitions from the production file so these helpers are not exported in production binaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/dataloader/prowloader/prow.go`:
- Around line 369-372: Log messages incorrectly reference "detaching" for
operations that drop partitions and read stats; update the Errorf calls that
follow partitions.DropOldDetachedPartitions (where it currently logs "error
detaching partitions for %s") and the similar block around lines 385-387 to use
accurate text such as "error dropping partitions for %s" and "error reading
detached partition stats for %s" respectively, keeping the same
log.WithError(err).Errorf pattern and using tableName to format the message.
In `@pkg/db/partitions/partitions.go`:
- Around line 600-642: The DetachPartition function is vulnerable because it
interpolates tableName and partitionName directly into the DDL; change it to
quote/escape identifiers the same way used in DropPartition (i.e., wrap
identifiers with the safe quoting function used elsewhere) before building the
ALTER TABLE ... DETACH PARTITION SQL string. Locate DetachPartition and replace
direct usage of tableName and partitionName in the query construction with the
sanitized/quoted versions (use the project’s identifier quoting helper that
DropPartition uses), ensuring you still validate the name with
isValidPartitionName and preserve dryRun/logging behavior.
- Around line 976-999: The CREATE and ALTER DDL strings in createTableQuery and
attachQuery use unquoted identifiers (partitionName, tableName) and should use
the same identifier quoting helper used by DropPartition to avoid SQL injection
and reserved-word issues; update those format strings to quote/escape
identifiers (tableName and partitionName) before interpolation and likewise
quote range literals if your helper supports it. Also wrap the create + attach
sequence in a single DB transaction (begin, Exec CREATE, Exec ATTACH,
commit/rollback) so the cleanup DROP TABLE fallback is redundant; if you keep
cleanup, ensure the rollback path drops the quoted partitionName. Locate
createTableQuery, attachQuery, the Exec calls on dbc.DB and the cleanup Exec
call to apply these changes.
- Around line 1252-1260: The dynamically-built DDL uses raw table and column
identifiers (tableName, columns, partitionClause) and needs SQL identifier
quoting; add a small helper (e.g., quoteIdentifier(name string) string) that
wraps identifiers in the proper quote (") and escapes embedded quotes, then
apply it when building createTableSQL and any other DDL construction sites
(e.g., where columns are joined and where tableName is interpolated, and the
other locations noted around 1299-1301). Ensure you only quote the identifier
parts (preserve types/constraints in column definitions) and use the helper for
every occurrence instead of raw variables so all table/column names are
consistently quoted.
- Around line 1874-1882: The ALTER TABLE string interpolates unquoted
identifiers (tableName, partitionName); update the DDL to use properly quoted
identifiers by wrapping tableName and partitionName with the project's
identifier-quoting helper (e.g., QuoteIdentifier) or by adding a local
quoteIdentifier that escapes and double-quotes names (handling schema-qualified
names), then build query with fmt.Sprintf("ALTER TABLE %s ATTACH PARTITION %s
FOR VALUES FROM ('%s') TO ('%s')", QuoteIdentifier(tableName),
QuoteIdentifier(partitionName), startDate, endDate) so identifiers are
consistently quoted.
- Around line 1536-1540: The code currently unconditionally appends DROP COLUMN
statements for any entries in currentColMap, which can cause accidental data
loss; update the function that builds alterStatements (e.g.,
UpdatePartitionedTable) to accept a boolean flag like allowDropColumns (and
optionally dryRun) and only generate/append "ALTER TABLE ... DROP COLUMN ..."
when allowDropColumns is true and any required confirmation is provided; if
allowDropColumns is false, do not append DROP statements and instead log a WARN
via the same logger used elsewhere indicating which columns would be dropped
(listing keys from currentColMap) so the user can review; ensure unit tests and
call sites are updated to pass the new parameter and that logging is used
instead of silently adding destructive SQL.
- Around line 556-598: The DROP uses an unquoted identifier—wrap the partition
identifier with pq.QuoteIdentifier to prevent SQL injection even after
validation: in DropPartition (which calls extractTableNameFromPartition and
isValidPartitionName) replace query := fmt.Sprintf("DROP TABLE IF EXISTS %s",
partitionName) with building the query using pq.QuoteIdentifier(partitionName).
Also ensure the pq package is imported in this file (github.com/lib/pq) so the
QuoteIdentifier call compiles; keep all existing validation and logging
unchanged.
In `@pkg/db/partitions/README.md`:
- Around line 199-205: Update the README to correct the inconsistent retention
value: change the "Minimum retention period enforcement (30 days)" text in the
Input Validation section to "Minimum retention period enforcement (90 days)" so
it matches the Safety Checks heading and the actual code enforcement (see the
retentionDays < 90 check and the "Minimum 90 days retention" line).
---
Duplicate comments:
In `@pkg/db/utils.go`:
- Around line 686-771: GetTableRowCount and SyncIdentityColumn build SQL by
interpolating table/column identifiers directly; use the identifier-quoting
helper (e.g., QuoteIdentifier or quoteIdentifier) introduced in the earlier
comment to safely quote tableName and columnName before using fmt.Sprintf, and
keep values parameterized where possible (e.g., use dbc.DB.Raw("SELECT COUNT(*)
FROM "+QuoteIdentifier(tableName)).Scan(&count) and build the ALTER TABLE using
quoted identifiers: fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s RESTART WITH
%d", QuoteIdentifier(tableName), QuoteIdentifier(columnName), nextValue));
update references in GetTableRowCount and SyncIdentityColumn (including the
query/alterSQL variables and the dbc.DB.Raw/Exec calls) so identifiers are
quoted and SQL injection is prevented.
---
Nitpick comments:
In `@pkg/dataloader/prowloader/prow.go`:
- Around line 361-374: The function agePartitionsForDailyTestAnalysisByJob
currently uses hard-coded retention windows (90 for DetachOldPartitions and 100
for DropOldDetachedPartitions) which control data deletion; make these retention
values configurable instead of fixed literals: add configurable fields or
constants (e.g., ProwLoader.detachRetentionDays and ProwLoader.dropRetentionDays
or read from environment/config) and replace the literal 90 and 100 in the calls
to partitions.DetachOldPartitions and partitions.DropOldDetachedPartitions,
ensure the fields are initialized when ProwLoader is constructed (with sensible
defaults matching current behavior), and update the log messages to include the
configured retention values so behavior is clear per environment.
In `@pkg/db/partitions/examples.go`:
- Around line 1-760: The file defines many Example... functions (e.g.,
ExampleListPartitionedTables, ExampleListPartitions, ExampleGetStats,
ExampleComparePartitionStats, ExampleCheckRetentionPolicy, ExampleDryRunCleanup,
ExampleDetachedPartitions, ExampleAttachedPartitions,
ExampleDropOldDetachedPartitions, ExampleDetachWorkflow,
ExampleReattachPartition, ExampleCreateMissingPartitions,
ExampleCreatePartitionedTable, ExampleUpdatePartitionedTable,
ExampleWorkflowForAnyTable, ExampleCompleteWorkflow) that are exported in
production builds; move all these Example* helper functions out of the non-test
package file by either (A) relocating them into a new examples_test.go (or
examples_test.go in the same package) so they are only compiled for
tests/documentation, or (B) add an appropriate build constraint (e.g.,
//go:build examples) at the top of this file and adjust CI to avoid building
that tag by default—pick one approach and apply it consistently to all Example*
functions to prevent API bloat.
In `@pkg/db/utils_example.go`:
- Around line 1-687: The Example* helper functions (e.g.,
ExampleVerifyTablesHaveSameColumns, ExampleVerifyPartitionMatchesParent,
ExampleMigrateTableData, ExampleMigrateTableDataRange,
ExampleIncrementalMigrationByMonth, etc.) are defined in a non-test file and
will be included in production builds; move all Example... functions out of
pkg/db/utils_example.go into a new test file (e.g.,
pkg/db/utils_example_test.go) so they are only compiled for tests/documentation,
or alternatively add an appropriate build tag to exclude them from production.
To fix: create the new *_test.go file and copy every function named Example...
(referencing types/methods like DB.VerifyTablesHaveSameColumns,
DB.MigrateTableData, DB.MigrateTableDataRange, DB.SyncIdentityColumn,
DB.GetPartitionStrategy, DB.VerifyPartitionCoverage, etc.) into it (keeping
package db or db_test as desired), then remove the Example... definitions from
the production file so these helpers are not exported in production binaries.
Comment on lines
+369
to
+372
| dropped, err := partitions.DropOldDetachedPartitions(pl.dbc, tableName, 100, false) | ||
| if err != nil { | ||
| log.WithError(err).Errorf("error detaching partitions for %s", tableName) | ||
| return err |
Contributor
There was a problem hiding this comment.
Fix copy/paste log messages for drop/stats errors.
These logs currently mention “detaching” in contexts where we’re dropping or reading stats, which is misleading.
✏️ Suggested log text updates
- log.WithError(err).Errorf("error detaching partitions for %s", tableName)
+ log.WithError(err).Errorf("error dropping detached partitions for %s", tableName)- log.WithError(err).Errorf("error detaching partitions for %s", tableName)
+ log.WithError(err).Errorf("error getting attached partition stats for %s", tableName)Also applies to: 385-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/dataloader/prowloader/prow.go` around lines 369 - 372, Log messages
incorrectly reference "detaching" for operations that drop partitions and read
stats; update the Errorf calls that follow partitions.DropOldDetachedPartitions
(where it currently logs "error detaching partitions for %s") and the similar
block around lines 385-387 to use accurate text such as "error dropping
partitions for %s" and "error reading detached partition stats for %s"
respectively, keeping the same log.WithError(err).Errorf pattern and using
tableName to format the message.
Comment on lines
+556
to
+598
| // DropPartition drops a single partition (DESTRUCTIVE - requires write access) | ||
| // This is a wrapper around DROP TABLE for safety and logging | ||
| func DropPartition(dbc *db.DB, partitionName string, dryRun bool) error { | ||
| start := time.Now() | ||
|
|
||
| // Extract table name from partition name | ||
| tableName, err := extractTableNameFromPartition(partitionName) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid partition name: %w", err) | ||
| } | ||
|
|
||
| // Validate partition name format for safety | ||
| if !isValidPartitionName(tableName, partitionName) { | ||
| return fmt.Errorf("invalid partition name: %s - must match %s_YYYY_MM_DD", partitionName, tableName) | ||
| } | ||
|
|
||
| if dryRun { | ||
| log.WithFields(log.Fields{ | ||
| "partition": partitionName, | ||
| "table": tableName, | ||
| }).Info("[DRY RUN] would drop partition") | ||
| return nil | ||
| } | ||
|
|
||
| query := fmt.Sprintf("DROP TABLE IF EXISTS %s", partitionName) | ||
| result := dbc.DB.Exec(query) | ||
| if result.Error != nil { | ||
| log.WithError(result.Error).WithFields(log.Fields{ | ||
| "partition": partitionName, | ||
| "table": tableName, | ||
| }).Error("failed to drop partition") | ||
| return result.Error | ||
| } | ||
|
|
||
| elapsed := time.Since(start) | ||
| log.WithFields(log.Fields{ | ||
| "partition": partitionName, | ||
| "table": tableName, | ||
| "elapsed": elapsed, | ||
| }).Info("dropped partition") | ||
|
|
||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for identifier quoting utility in codebase
rg -n "quote.*ident|QuoteIdent" --type=goRepository: openshift/sippy
Length of output: 1958
🏁 Script executed:
#!/bin/bash
# Check if pq library is available
rg -n '"github.com/lib/pq"' --type=goRepository: openshift/sippy
Length of output: 916
🏁 Script executed:
#!/bin/bash
# Find and examine isValidPartitionName function
rg -n "func isValidPartitionName" --type=go -A 10Repository: openshift/sippy
Length of output: 766
🏁 Script executed:
#!/bin/bash
# Find and examine extractTableNameFromPartition function
rg -n "func extractTableNameFromPartition" --type=go -A 10Repository: openshift/sippy
Length of output: 903
Use pq.QuoteIdentifier for identifier quoting.
While isValidPartitionName provides robust validation (exact length check, prefix validation, and date format parsing via time.Parse), identifier quoting should still be used as a defense-in-depth best practice. The github.com/lib/pq library is already imported elsewhere in the project (e.g., pkg/filter/filterable.go).
Recommended fix
- query := fmt.Sprintf("DROP TABLE IF EXISTS %s", partitionName)
+ query := "DROP TABLE IF EXISTS " + pq.QuoteIdentifier(partitionName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/partitions/partitions.go` around lines 556 - 598, The DROP uses an
unquoted identifier—wrap the partition identifier with pq.QuoteIdentifier to
prevent SQL injection even after validation: in DropPartition (which calls
extractTableNameFromPartition and isValidPartitionName) replace query :=
fmt.Sprintf("DROP TABLE IF EXISTS %s", partitionName) with building the query
using pq.QuoteIdentifier(partitionName). Also ensure the pq package is imported
in this file (github.com/lib/pq) so the QuoteIdentifier call compiles; keep all
existing validation and logging unchanged.
Comment on lines
+600
to
+642
| // DetachPartition detaches a partition from the parent table (safer alternative to DROP) | ||
| // The detached table can be archived or dropped later | ||
| func DetachPartition(dbc *db.DB, partitionName string, dryRun bool) error { | ||
| start := time.Now() | ||
|
|
||
| // Extract table name from partition name | ||
| tableName, err := extractTableNameFromPartition(partitionName) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid partition name: %w", err) | ||
| } | ||
|
|
||
| // Validate partition name format for safety | ||
| if !isValidPartitionName(tableName, partitionName) { | ||
| return fmt.Errorf("invalid partition name: %s - must match %s_YYYY_MM_DD", partitionName, tableName) | ||
| } | ||
|
|
||
| if dryRun { | ||
| log.WithFields(log.Fields{ | ||
| "partition": partitionName, | ||
| "table": tableName, | ||
| }).Info("[DRY RUN] would detach partition") | ||
| return nil | ||
| } | ||
|
|
||
| query := fmt.Sprintf("ALTER TABLE %s DETACH PARTITION %s", tableName, partitionName) | ||
| result := dbc.DB.Exec(query) | ||
| if result.Error != nil { | ||
| log.WithError(result.Error).WithFields(log.Fields{ | ||
| "partition": partitionName, | ||
| "table": tableName, | ||
| }).Error("failed to detach partition") | ||
| return result.Error | ||
| } | ||
|
|
||
| elapsed := time.Since(start) | ||
| log.WithFields(log.Fields{ | ||
| "partition": partitionName, | ||
| "table": tableName, | ||
| "elapsed": elapsed, | ||
| }).Info("detached partition") | ||
|
|
||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Same identifier quoting concern as DropPartition.
The tableName and partitionName are interpolated directly into the DDL statement at Line 624. Apply the same quoting approach recommended above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/partitions/partitions.go` around lines 600 - 642, The DetachPartition
function is vulnerable because it interpolates tableName and partitionName
directly into the DDL; change it to quote/escape identifiers the same way used
in DropPartition (i.e., wrap identifiers with the safe quoting function used
elsewhere) before building the ALTER TABLE ... DETACH PARTITION SQL string.
Locate DetachPartition and replace direct usage of tableName and partitionName
in the query construction with the sanitized/quoted versions (use the project’s
identifier quoting helper that DropPartition uses), ensuring you still validate
the name with isValidPartitionName and preserve dryRun/logging behavior.
Comment on lines
+976
to
+999
|
|
||
| // Create the partition table with same structure as parent | ||
| createTableQuery := fmt.Sprintf("CREATE TABLE IF NOT EXISTS %s (LIKE %s INCLUDING ALL)", partitionName, tableName) | ||
| result := dbc.DB.Exec(createTableQuery) | ||
| if result.Error != nil { | ||
| log.WithError(result.Error).WithField("partition", partitionName).Error("failed to create partition table") | ||
| continue | ||
| } | ||
|
|
||
| // Attach the partition to the parent table | ||
| attachQuery := fmt.Sprintf( | ||
| "ALTER TABLE %s ATTACH PARTITION %s FOR VALUES FROM ('%s') TO ('%s')", | ||
| tableName, | ||
| partitionName, | ||
| rangeStart, | ||
| rangeEnd, | ||
| ) | ||
| result = dbc.DB.Exec(attachQuery) | ||
| if result.Error != nil { | ||
| // If attach fails, try to clean up the created table | ||
| log.WithError(result.Error).WithField("partition", partitionName).Error("failed to attach partition") | ||
| dbc.DB.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %s", partitionName)) | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Identifier quoting needed in DDL statements.
Lines 978, 986-992, and 997 construct DDL statements with unquoted identifiers. Apply the same identifier quoting approach as recommended for DropPartition.
Additionally, the cleanup logic at Line 997 is good defensive coding, but consider wrapping the create+attach in a transaction for atomicity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/partitions/partitions.go` around lines 976 - 999, The CREATE and ALTER
DDL strings in createTableQuery and attachQuery use unquoted identifiers
(partitionName, tableName) and should use the same identifier quoting helper
used by DropPartition to avoid SQL injection and reserved-word issues; update
those format strings to quote/escape identifiers (tableName and partitionName)
before interpolation and likewise quote range literals if your helper supports
it. Also wrap the create + attach sequence in a single DB transaction (begin,
Exec CREATE, Exec ATTACH, commit/rollback) so the cleanup DROP TABLE fallback is
redundant; if you keep cleanup, ensure the rollback path drops the quoted
partitionName. Locate createTableQuery, attachQuery, the Exec calls on dbc.DB
and the cleanup Exec call to apply these changes.
Comment on lines
+1252
to
+1260
| // Build the CREATE TABLE statement with partition strategy | ||
| partitionClause := config.ToSQL() | ||
| createTableSQL := fmt.Sprintf( | ||
| "CREATE TABLE IF NOT EXISTS %s (\n %s\n) %s", | ||
| tableName, | ||
| strings.Join(columns, ",\n "), | ||
| partitionClause, | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Apply identifier quoting to dynamically constructed DDL.
The table name and column names at Lines 1255, 1299-1301, and elsewhere should be quoted. While table/column names from GORM models are typically safe, applying consistent quoting is a defensive best practice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/partitions/partitions.go` around lines 1252 - 1260, The
dynamically-built DDL uses raw table and column identifiers (tableName, columns,
partitionClause) and needs SQL identifier quoting; add a small helper (e.g.,
quoteIdentifier(name string) string) that wraps identifiers in the proper quote
(") and escapes embedded quotes, then apply it when building createTableSQL and
any other DDL construction sites (e.g., where columns are joined and where
tableName is interpolated, and the other locations noted around 1299-1301).
Ensure you only quote the identifier parts (preserve types/constraints in column
definitions) and use the helper for every occurrence instead of raw variables so
all table/column names are consistently quoted.
Comment on lines
+1536
to
+1540
| // Remaining columns in map should be dropped | ||
| for colName := range currentColMap { | ||
| alterStatements = append(alterStatements, | ||
| fmt.Sprintf("ALTER TABLE %s DROP COLUMN %s", tableName, colName)) | ||
| } |
Contributor
There was a problem hiding this comment.
Dropping columns without confirmation could be dangerous.
Line 1537-1539 automatically generates DROP COLUMN statements for columns present in the database but not in the model. This could unintentionally cause data loss if a model field is accidentally removed or renamed.
Consider:
- Adding a separate flag to enable column drops (disabled by default)
- Requiring explicit confirmation for destructive operations
- At minimum, logging at WARN level instead of just including in alterStatements
🛡️ Proposed safer approach
// Remaining columns in map should be dropped
+ // WARNING: Only drop columns if explicitly enabled to prevent accidental data loss
for colName := range currentColMap {
- alterStatements = append(alterStatements,
- fmt.Sprintf("ALTER TABLE %s DROP COLUMN %s", tableName, colName))
+ log.WithFields(log.Fields{
+ "table": tableName,
+ "column": colName,
+ }).Warn("column exists in database but not in model - consider manual removal if intended")
}Or add a parameter to control this behavior:
func UpdatePartitionedTable(dbc *db.DB, model interface{}, tableName string, allowDropColumns bool, dryRun bool) (string, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/partitions/partitions.go` around lines 1536 - 1540, The code currently
unconditionally appends DROP COLUMN statements for any entries in currentColMap,
which can cause accidental data loss; update the function that builds
alterStatements (e.g., UpdatePartitionedTable) to accept a boolean flag like
allowDropColumns (and optionally dryRun) and only generate/append "ALTER TABLE
... DROP COLUMN ..." when allowDropColumns is true and any required confirmation
is provided; if allowDropColumns is false, do not append DROP statements and
instead log a WARN via the same logger used elsewhere indicating which columns
would be dropped (listing keys from currentColMap) so the user can review;
ensure unit tests and call sites are updated to pass the new parameter and that
logging is used instead of silently adding destructive SQL.
Comment on lines
+1874
to
+1882
|
|
||
| // Reattach the partition with FOR VALUES clause | ||
| query := fmt.Sprintf( | ||
| "ALTER TABLE %s ATTACH PARTITION %s FOR VALUES FROM ('%s') TO ('%s')", | ||
| tableName, | ||
| partitionName, | ||
| startDate, | ||
| endDate, | ||
| ) |
Contributor
There was a problem hiding this comment.
Apply identifier quoting for DDL statement.
Line 1876-1882 constructs the ALTER TABLE ATTACH PARTITION statement with unquoted identifiers. Apply consistent identifier quoting as recommended earlier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/partitions/partitions.go` around lines 1874 - 1882, The ALTER TABLE
string interpolates unquoted identifiers (tableName, partitionName); update the
DDL to use properly quoted identifiers by wrapping tableName and partitionName
with the project's identifier-quoting helper (e.g., QuoteIdentifier) or by
adding a local quoteIdentifier that escapes and double-quotes names (handling
schema-qualified names), then build query with fmt.Sprintf("ALTER TABLE %s
ATTACH PARTITION %s FOR VALUES FROM ('%s') TO ('%s')",
QuoteIdentifier(tableName), QuoteIdentifier(partitionName), startDate, endDate)
so identifiers are consistently quoted.
Comment on lines
+199
to
+205
| **Safety Checks**: | ||
| - Minimum 90 days retention | ||
| - Maximum 75% of attached partitions deleted | ||
| - Maximum 80% of attached storage deleted | ||
|
|
||
| **Important**: Only considers **attached partitions** when validating thresholds. Detached partitions are excluded from calculations to ensure the policy is safe for active data. | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the README file in the partitions directory
fd -t f README.md -p "*/partitions/*"Repository: openshift/sippy
Length of output: 160
🏁 Script executed:
# Read the README file around the cited lines
wc -l pkg/db/partitions/README.mdRepository: openshift/sippy
Length of output: 91
🏁 Script executed:
# Check the specific lines mentioned in the review
sed -n '195,210p' pkg/db/partitions/README.mdRepository: openshift/sippy
Length of output: 583
🏁 Script executed:
# Check lines 1442-1445
sed -n '1438,1450p' pkg/db/partitions/README.mdRepository: openshift/sippy
Length of output: 412
🏁 Script executed:
# Search for retention period enforcement in the partitions package
rg -A 3 -B 3 "retention|RetentionDays" pkg/db/partitions/ --type-list | head -30Repository: openshift/sippy
Length of output: 448
🏁 Script executed:
# Look for any constants or validation that enforces minimum retention
rg "30|90" pkg/db/partitions/ -A 2 -B 2 --type goRepository: openshift/sippy
Length of output: 1413
🏁 Script executed:
# Search more broadly for retention validation logic
rg "Minimum.*retention|retention.*minimum" pkg/db/partitions/ -iRepository: openshift/sippy
Length of output: 392
Update documentation to correct retention period inconsistency.
The README states "Minimum 90 days retention" in the Safety Checks section, but "Minimum retention period enforcement (30 days)" in the Input Validation section. The actual code enforces a 90-day minimum (if retentionDays < 90), so the 30-day value must be corrected to 90 days to align with both the stated policy and implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/partitions/README.md` around lines 199 - 205, Update the README to
correct the inconsistent retention value: change the "Minimum retention period
enforcement (30 days)" text in the Input Validation section to "Minimum
retention period enforcement (90 days)" so it matches the Safety Checks heading
and the actual code enforcement (see the retentionDays < 90 check and the
"Minimum 90 days retention" line).
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Initial work managing tables and partitions within sippy
Summary by CodeRabbit
New Features
Documentation
Tests