Remap filtered rows to relation descriptor for dropped columns#540
Remap filtered rows to relation descriptor for dropped columns#540SAY-5 wants to merge 1 commit into
Conversation
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
📝 WalkthroughWalkthroughChangesFiltered tuple descriptor conversion
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/spock_functions.c`:
- Around line 3040-3048: In the SPI result loop, update the tuple handling
around execute_attr_map_tuple so each newly remapped HeapTuple is freed after
tuplestore_puttuple stores it, while leaving original SPI tuples untouched when
no mapping occurs. Ensure cleanup happens only for tuples allocated by
execute_attr_map_tuple.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d4567e6-f8fa-457e-b352-946e5e4de88a
📒 Files selected for processing (1)
src/spock_functions.c
| for (i = 0; i < SPI_processed; i++) | ||
| { | ||
| HeapTuple tup = SPI_tuptable->vals[i]; | ||
|
|
||
| if (map != NULL) | ||
| tup = execute_attr_map_tuple(tup, map); | ||
|
|
||
| tuplestore_puttuple(tupstore, tup); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Free the remapped tuple to prevent severe memory leaks.
execute_attr_map_tuple allocates a new HeapTuple for each row. Since SPI_execute already materializes the entire filtered dataset in memory, accumulating newly allocated tuples without freeing them will double the memory usage. For large tables, this resource leak can quickly lead to an Out-Of-Memory (OOM) error and crash the backend during initial synchronization.
Free the remapped tuple after storing it, as tuplestore_puttuple safely copies the tuple into the tuplestore's memory or disk context.
🐛 Proposed fix
for (i = 0; i < SPI_processed; i++)
{
HeapTuple tup = SPI_tuptable->vals[i];
if (map != NULL)
- tup = execute_attr_map_tuple(tup, map);
-
- tuplestore_puttuple(tupstore, tup);
+ {
+ HeapTuple mapped_tup = execute_attr_map_tuple(tup, map);
+ tuplestore_puttuple(tupstore, mapped_tup);
+ heap_freetuple(mapped_tup);
+ }
+ else
+ {
+ tuplestore_puttuple(tupstore, tup);
+ }
}📝 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.
| for (i = 0; i < SPI_processed; i++) | |
| { | |
| HeapTuple tup = SPI_tuptable->vals[i]; | |
| if (map != NULL) | |
| tup = execute_attr_map_tuple(tup, map); | |
| tuplestore_puttuple(tupstore, tup); | |
| } | |
| for (i = 0; i < SPI_processed; i++) | |
| { | |
| HeapTuple tup = SPI_tuptable->vals[i]; | |
| if (map != NULL) | |
| { | |
| HeapTuple mapped_tup = execute_attr_map_tuple(tup, map); | |
| tuplestore_puttuple(tupstore, mapped_tup); | |
| heap_freetuple(mapped_tup); | |
| } | |
| else | |
| { | |
| tuplestore_puttuple(tupstore, tup); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/spock_functions.c` around lines 3040 - 3048, In the SPI result loop,
update the tuple handling around execute_attr_map_tuple so each newly remapped
HeapTuple is freed after tuplestore_puttuple stores it, while leaving original
SPI tuples untouched when no mapping occurs. Ensure cleanup happens only for
tuples allocated by execute_attr_map_tuple.
1 participant
spock_table_data_filtered()runsSELECT * FROM <rel>through SPI and puts the result tuples straight into the tuplestore, whose descriptor is the full relation descriptor.SELECT *omits dropped columns, so those tuples only carry the live attributes and every attribute past a dropped position is then read at the wrong offset, giving corrupt/NULL data on the subscriber or a provider crash when a misread varlena is detoasted (#528).This remaps each SPI tuple to the relation descriptor with
convert_tuples_by_position()/execute_attr_map_tuple(), which inserts NULLs at the dropped positions; when the descriptors already match,convert_tuples_by_position()returns NULL and the tuples are stored unchanged.Reproducing and adding the row-filter regression case both need a running provider/subscriber cluster, which I could not spin up locally, so I verified the change compiles clean against the PG server headers but did not run the sync suite. A minimal SQL repro from the issue: