Uh oh!
There was an error while loading. Please reload this page.
Add basic community files - #1
Merged
Merged
Conversation
tuhaihe
requested review from
my-ship-it and oracleloyall
and removed request for
oracleloyallNovember 17, 2023 03:49
liang8283 added a commit
to liang8283/cloudberry-backup
that referenced
this pull request
May 11, 2026
Follow-up to e24bc69. Two real bugs and two minor cleanups found in code review of that commit: 1. (data loss) gp_toolkit.gpbackup_file_info hand-built the tablespace path as 'pg_tblspc/<tsp>/<dboid>/<rfn>', missing the mandatory 'PG_<major>_<catver>' directory that PostgreSQL/Cloudberry put in the middle. pg_stat_file would fail for every heap table living in a custom tablespace; the EXCEPTION branch swallowed the error and returned the literal string '|0' (not empty). Because '|0' is not filtered by 'WHERE info <> '''', that constant became the table's hash on every backup, so any custom-tablespace heap table would appear unchanged forever and be silently skipped from every incremental backup -- losing any new rows. 2. (SQL injection / correctness) getTableFileHash interpolated FQN parts straight into the SQL via fmt.Sprintf('%s', '%s'). splitFQN stripped double quotes but left single quotes intact, so a table name with an embedded single quote (a legal pg identifier, e.g. "o'reilly") would break out of the string literal. 3. (clarity) getAOSegContentHash's default case said "GP7+ AOCS" but actually fires for GP6+. Comment fixed. 4. (consistency) HeapEntry.FileHashMD5 missing 'omitempty' that AOEntry.FileHashMD5 already had. Fix for apache#1 and apache#2 is the same change: pass the table's OID through to the plpgsql function, and let pg_relation_filepath() compute the on-disk path. The built-in already handles all tablespace layouts correctly across PG/Cloudberry versions, and oid interpolation is just an integer literal so the SQL-string-escaping concern disappears. The EXCEPTION block now returns true empty-string, so a hash failure on a single table falls through to "include in incremental" instead of poisoning the hash with a constant. Implementation notes: backup/queries_incremental.go - gp_toolkit.gpbackup_file_info now takes (p_oid oid) and uses pg_relation_filepath(p_oid). The setup path drops any older (text, text) signature first so an in-place upgrade against a previous gpbackup installation cleans up cleanly. The duplicate- check query gained 'pronargs = 1' to distinguish from the old signature. - getHeapTableFQNs -> getHeapTables, returning []heapTable{Oid,FQN}. - getTableFileHash(hashConn, oid, fqn) -- oid interpolated as integer literal, fqn used only for log messages. - getFileHashesForTables takes []heapTable. - splitFQN removed. backup/wrappers.go - One-line callsite update. toc/toc.go - HeapEntry.FileHashMD5 gets omitempty for symmetry with AOEntry. Verification: - make build / make unit on the server: green. - Targeted live cluster e2e (heap + ao_row + ao_column + partitioned AO, full + mutate + incremental + restore): every expected inclusion/exclusion matches, restored row counts match source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liang8283 added a commit
to liang8283/cloudberry-backup
that referenced
this pull request
May 12, 2026
Follow-up to e24bc69. Two real bugs and two minor cleanups found in code review of that commit: 1. (data loss) gp_toolkit.gpbackup_file_info hand-built the tablespace path as 'pg_tblspc/<tsp>/<dboid>/<rfn>', missing the mandatory 'PG_<major>_<catver>' directory that PostgreSQL/Cloudberry put in the middle. pg_stat_file would fail for every heap table living in a custom tablespace; the EXCEPTION branch swallowed the error and returned the literal string '|0' (not empty). Because '|0' is not filtered by 'WHERE info <> '''', that constant became the table's hash on every backup, so any custom-tablespace heap table would appear unchanged forever and be silently skipped from every incremental backup -- losing any new rows. 2. (SQL injection / correctness) getTableFileHash interpolated FQN parts straight into the SQL via fmt.Sprintf('%s', '%s'). splitFQN stripped double quotes but left single quotes intact, so a table name with an embedded single quote (a legal pg identifier, e.g. "o'reilly") would break out of the string literal. 3. (clarity) getAOSegContentHash's default case said "GP7+ AOCS" but actually fires for GP6+. Comment fixed. 4. (consistency) HeapEntry.FileHashMD5 missing 'omitempty' that AOEntry.FileHashMD5 already had. Fix for apache#1 and apache#2 is the same change: pass the table's OID through to the plpgsql function, and let pg_relation_filepath() compute the on-disk path. The built-in already handles all tablespace layouts correctly across PG/Cloudberry versions, and oid interpolation is just an integer literal so the SQL-string-escaping concern disappears. The EXCEPTION block now returns true empty-string, so a hash failure on a single table falls through to "include in incremental" instead of poisoning the hash with a constant. Implementation notes: backup/queries_incremental.go - gp_toolkit.gpbackup_file_info now takes (p_oid oid) and uses pg_relation_filepath(p_oid). The setup path drops any older (text, text) signature first so an in-place upgrade against a previous gpbackup installation cleans up cleanly. The duplicate- check query gained 'pronargs = 1' to distinguish from the old signature. - getHeapTableFQNs -> getHeapTables, returning []heapTable{Oid,FQN}. - getTableFileHash(hashConn, oid, fqn) -- oid interpolated as integer literal, fqn used only for log messages. - getFileHashesForTables takes []heapTable. - splitFQN removed. backup/wrappers.go - One-line callsite update. toc/toc.go - HeapEntry.FileHashMD5 gets omitempty for symmetry with AOEntry. Verification: - make build / make unit on the server: green. - Targeted live cluster e2e (heap + ao_row + ao_column + partitioned AO, full + mutate + incremental + restore): every expected inclusion/exclusion matches, restored row counts match source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
No description provided.