Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for cache reference leak warning #3161

Open
wants to merge 50 commits into
base: BABEL_4_X_DEV
Choose a base branch
from

Conversation

pranavJ23
Copy link
Contributor

@pranavJ23 pranavJ23 commented Nov 25, 2024

Description

Problem: Cache Reference Leaks

  1. During the Alter function/procedure we are doing a sys-cache search without releasing it.
  2. During the routines-definition, in one case we were not releasing the syscache search.

Fix:
a. To do sys-cache search via tuple and releasing it after the sys-cache search.
b. Also we are generating the logfile but the path that we were giving it github action workflow was not correct, updated
that too.
c. Added a step in jdbc-test workflow which will compare the warnings in logfile with expected warnings

Issues Resolved

[BABEL-5551]

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@coveralls
Copy link
Collaborator

coveralls commented Nov 25, 2024

Pull Request Test Coverage Report for Build 13101407937

Details

  • 6 of 8 (75.0%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.004%) to 74.945%

Changes Missing Coverage Covered Lines Changed/Added Lines %
contrib/babelfishpg_tsql/src/pl_handler.c 5 6 83.33%
contrib/babelfishpg_tsql/src/pltsql_ruleutils.c 1 2 50.0%
Files with Coverage Reduction New Missed Lines %
contrib/babelfishpg_tsql/src/pltsql_ruleutils.c 1 61.09%
contrib/babelfishpg_tds/src/backend/tds/tdscomm.c 2 76.03%
Totals Coverage Status
Change from base Build 13049387475: -0.004%
Covered Lines: 47013
Relevant Lines: 62730

💛 - Coveralls

@pranavJ23 pranavJ23 changed the title [OSS ONLY] Fix to throw error instead of warning in JDBC test logfile Fix for cache reference leak warning Jan 17, 2025
@pranavJ23 pranavJ23 requested a review from tanscorpio7 January 17, 2025 12:54
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
@pranavJ23 pranavJ23 requested a review from sumitj824 January 21, 2025 11:55
if(get_bbf_function_tuple_from_proctuple(SearchSysCache1(PROCOID, ObjectIdGetDatum(oldoid))) == NULL)
proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(oldoid));

if (proctup == NULL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use HeapTupleIsValid & curly braces can be removed in next line.

if (get_bbf_function_tuple_from_proctuple(proctup) == NULL)
{
/* Release the tuple before reporting the error */
ReleaseSysCache(proctup);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of error we do not need to release syscache

Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
EXPECTED_WARNINGS_FILE=~/work/babelfish_extensions/babelfish_extensions/test/JDBC/expected/expected-warnings.out

# Collect all leak warnings from log files, extracting only the warning type
grep -i "WARNING:.*leak" ~/psql/data/logfile | sed -E 's/^.*WARNING:\s*([^:]+(\s+reference\s+leak|connection leak)[^:]*):.*/WARNING: \1/' | sort > actual-leak-warnings.out
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some warnings do not contain the word leak but are still important for us & need to be resolved.
For example elog(WARNING, "snapshot %p still active", active);
Will this check be able to catch this WARNING if it comes up in future ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To grep the warnings which do not contains leak, we need to either grep only "WARNING" or we need to grep the whole line containing "WARNING" keyword.
If we only grep WARNING in the expected-file then while debugging it might be difficult to tell that which type of new warning we are getting.
If we grep whole line containing "WARNING" then there are some reference tuple which might be dynamically allocated during test run, so we cannot compare the expected and actual files.
Open to suggestions!

Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
@@ -537,7 +537,10 @@ tsql_get_functiondef(PG_FUNCTION_ARGS)
(void) tsql_print_function_arguments(&buf, proctup, false, true, &typmod_arr, &has_tvp);
/* TODO: In case of Table Valued Functions, return NULL. */
if (has_tvp)
{
ReleaseSysCache(proctup);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one more PG_RETURN_NULL() at line 528, we need to release sys cache there as well.

if: always()
id: check-warnings
run: |
LOG_FILE=~/psql/data/logfile
Copy link
Contributor

@sumitj824 sumitj824 Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets have bash script for this check and you can invoke that script directly.

@pranavJ23 pranavJ23 requested a review from sumitj824 January 29, 2025 05:05
Comment on lines 154 to 157
if [[ "$LEAK_COUNT" -ne 380 ]]; then
echo "Error: Expected 380 leak warnings, but found $LEAK_COUNT"
ERROR_FOUND=true
fi
Copy link
Contributor

@sumitj824 sumitj824 Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that for any new testfile which will generate warnings that are expected will increase this count and everytime developer needs to update this?

Copy link
Contributor Author

@pranavJ23 pranavJ23 Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes if that generates the LEAK or SNAPSHOT warning then only this count will change, for that case developer should check why the warning is generated
If there is some other warning, which is expected we are ignoring them. For a new kind of warning we are saying other unexpected warning

Signed-off-by: pranav jain <[email protected]>
Signed-off-by: pranav jain <[email protected]>
@pranavJ23 pranavJ23 requested a review from sumitj824 January 30, 2025 05:23
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.

4 participants