Use pre_len in package cleanup debug logging#1215
Use pre_len in package cleanup debug logging#1215c2main wants to merge 1 commit intoIvorySQL:masterfrom
Conversation
Keep pre-removal list length available where it is actually useful by including it in the DEBUG1 cleanup message, while preserving the length invariant Assert() around list_delete_ptr().
📝 WalkthroughWalkthroughThe change enhances diagnostic logging in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/pl/plisql/src/pl_package.c`:
- Around line 2733-2739: The code currently assumes list_delete_ptr removed one
element and unconditionally decrements psource->source.use_count, which breaks
in non-Assert builds if func wasn't present; modify the logic around
psource->source.funclist and use_count so you compute post_len =
list_length(psource->source.funclist) after calling list_delete_ptr (or check
whether pre_len > post_len), only decrement psource->source.use_count when
pre_len == post_len + 1 (and keep the Assert for the debug case if desired), and
do not change use_count when no element was removed to avoid corrupting the
refcount and prematurely calling plisql_free_package_function().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb452cee-6464-4045-b8f3-6492fd41f7f0
📒 Files selected for processing (1)
src/pl/plisql/src/pl_package.c
| int pre_len = list_length(psource->source.funclist); | ||
|
|
||
| psource->source.funclist = list_delete_ptr(psource->source.funclist, (void *) func); | ||
|
|
||
| Assert(pre_len == list_length(psource->source.funclist) + 1); | ||
|
|
||
| psource->source.use_count--; |
There was a problem hiding this comment.
Don’t rely on Assert() to protect the refcount.
On Line 2737 the invariant disappears in non-assert builds, but Line 2739 still decrements use_count. If func is not actually present in funclist, production builds will corrupt the refcount and can prematurely reach plisql_free_package_function().
Suggested hardening
int pre_len = list_length(psource->source.funclist);
psource->source.funclist = list_delete_ptr(psource->source.funclist, (void *) func);
- Assert(pre_len == list_length(psource->source.funclist) + 1);
+ if (pre_len == list_length(psource->source.funclist))
+ elog(ERROR, "function reference for package %u was not registered", item->pkey);
+ Assert(pre_len == list_length(psource->source.funclist) + 1);
psource->source.use_count--;📝 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.
| int pre_len = list_length(psource->source.funclist); | |
| psource->source.funclist = list_delete_ptr(psource->source.funclist, (void *) func); | |
| Assert(pre_len == list_length(psource->source.funclist) + 1); | |
| psource->source.use_count--; | |
| int pre_len = list_length(psource->source.funclist); | |
| psource->source.funclist = list_delete_ptr(psource->source.funclist, (void *) func); | |
| if (pre_len == list_length(psource->source.funclist)) | |
| elog(ERROR, "function reference for package %u was not registered", item->pkey); | |
| Assert(pre_len == list_length(psource->source.funclist) + 1); | |
| psource->source.use_count--; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pl/plisql/src/pl_package.c` around lines 2733 - 2739, The code currently
assumes list_delete_ptr removed one element and unconditionally decrements
psource->source.use_count, which breaks in non-Assert builds if func wasn't
present; modify the logic around psource->source.funclist and use_count so you
compute post_len = list_length(psource->source.funclist) after calling
list_delete_ptr (or check whether pre_len > post_len), only decrement
psource->source.use_count when pre_len == post_len + 1 (and keep the Assert for
the debug case if desired), and do not change use_count when no element was
removed to avoid corrupting the refcount and prematurely calling
plisql_free_package_function().
|
@NotHimmel follow up this PR |
|
Hi, Cedric, the code change looks good to me. Are you going to change the state from Draft to Open? @c2main |
Keep pre-removal list length available where it is actually useful by including it in the DEBUG1 cleanup message, while preserving the length invariant Assert() around list_delete_ptr().
Summary by CodeRabbit