Skip to content

Conversation

@siqueiraa
Copy link

Summary

  • Fixed the --restore-table-mapping workflow so that schema and data restores both target the mapped table names, preventing accidental drops of the original table and the subsequent "… is not created" failure.
  • Normalised CLI-provided table mappings (e.g. transaction_archive:transaction_archive_v4 and db.table:newdb.newtable) so they are stored in a consistent form and applied everywhere in the restore pipeline.
  • Hardened the data-restore phase to query ClickHouse with both original and mapped patterns, ensuring it locates newly created tables and surfaces actionable diagnostics if a mapping is misconfigured.
  • Enhanced the missing-table check to consider mapped/original combinations and to re-query ClickHouse before emitting an error, reducing false negatives and clarifying real issues.
  • Added focused unit coverage to lock the new mapping behaviour in place.

Context

Restoring m_views_tables.transaction_archive with --restore-table-mapping transaction_archive:transaction_archive_v4 previously dropped the target table (transaction_archive_v4), created it during schema restore, but the data phase still probed ClickHouse for the original name. That caused the restore to abort with 'm_views_tables.transaction_archive' is not created, leaving data un-restored.

Root Cause

restoreSchemaRegular operated on metadata that already carried the mapped name, while restoreDataRegular continued to query ClickHouse (and validate presence) using the original identifier. Because GetTables never saw transaction_archive_v4 under the old pattern, the data phase declared the table missing even though schema creation succeeded.

Solution Details

  • Normalised table mapping ingestion (prepareRestoreMapping) so both CLI formats (bare table and db.table) collapse to the canonical table key.
  • Updated restoreDataRegular to:
    • Log both metadata tables and the system tables returned by GetTables.
    • Query ClickHouse with a merged pattern containing original and mapped names, ensuring visibility of the newly created table.
    • Fall back to targeted lookups inside checkMissingTables before erroring out.
  • Adjusted checkMissingTables to consider original, mapped, and reverse-mapped combinations and to re-populate its cache from ClickHouse on demand.
  • Kept restoreSchemaRegular intact per upstream guidance; all fixes are isolated to mapping ingestion and data-phase validation.
  • Added unit tests covering the mapping normalisation and pattern rewriting paths to guard against regressions.

Testing

  • GOCACHE=$(pwd)/.gocache go test ./pkg/backup -run TestRestore
  • Manual restore via API:
    restore --table=m_views_tables.transaction_archive \
            --restore-table-mapping transaction_archive:transaction_archive_v4 \
            chi-ch-cluster-ch-deployment-0-0-full-2025-10-20-10-41-51
    
    Observed data restore completing against m_views_tables.transaction_archive_v4 without missing-table errors.

Rollout Notes

  • Requires rebuilding/publishing the container image (make build-docker + docker build … --target image_short).
  • After deployment, existing restore jobs using --restore-table-mapping will pick up the fix automatically; no config updates needed.
  • Optional: log verbosity can be reduced (or gated behind DEBUG) once operators finish validating the new behaviour.

…TE operations

- Table mapping was not applied during schema restoration
- DROP/DETACH operations were targeting original table names instead of mapped names
- Added logic to apply RestoreTableMapping before schema operations
- Fixes issue where --restore-table-mapping parameter was ignored during --schema restore

Issue: When using --restore-table-mapping with --schema flag, the tool would
execute DROP/DETACH on the original table name from backup instead of the
mapped target table name, causing data loss.

The RestoreData function correctly applies mapping, but restoreSchemaRegular
did not. This fix ensures consistency across both functions.
@Slach
Copy link
Collaborator

Slach commented Oct 20, 2025

please stop using vibe-coding if you can't programming and don't understand what exactly was generated

@Slach
Copy link
Collaborator

Slach commented Oct 20, 2025

thanks for catching bug anyway, will try to fix it

@siqueiraa
Copy link
Author

Are you talking about the treatment in pkg/backup/restore.go being redundant? (408:507) @Slach
If so, you could have just asked me to change it instead of saying I’m only “vibecoding.” I’ve been coding in Go for at least two years, and I do have a basic understanding of how the language works

@Slach
Copy link
Collaborator

Slach commented Oct 20, 2025

My apologies, please excuse my rude words. I just in stress.
I will explain later

Was I right about vibe-coded PR?

@siqueiraa
Copy link
Author

No, I only used AI to generate the PR description because I couldn’t find any template, and I wanted to provide the correct context.
As for the code, I’ve been looking into it since Friday generating tests and learning your code before suggesting the solution.

@Slach
Copy link
Collaborator

Slach commented Oct 20, 2025

Ok, yep, too much words for PR description than usual
sorry again for my rude words, will provide more details later

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.

2 participants