Skip to content

Replace std::shared_ptr<File> by std::unique_ptr<File>#1054

Merged
gfphoenix78 merged 2 commits intoapache:mainfrom
gfphoenix78:use-unique-ptr
Jul 17, 2025
Merged

Replace std::shared_ptr<File> by std::unique_ptr<File>#1054
gfphoenix78 merged 2 commits intoapache:mainfrom
gfphoenix78:use-unique-ptr

Conversation

@gfphoenix78
Copy link
Copy Markdown
Contributor

Shared smart pointer will introduce additional cost, and it also make the ownership complicated.
Use unique pointer to make the ownership explicitly visible.

Test Plan

  • Unit tests added/updated
  • Integration tests added/updated
  • Passed make installcheck
  • Passed make -C src/test installcheck-cbdb-parallel

Impact

Performance:

User-facing changes:

Dependencies:

Checklist

Additional Context

CI Skip Instructions


Comment thread contrib/pax_storage/src/cpp/storage/orc/orc_test.cc
Copy link
Copy Markdown
Contributor

@Smyatkin-Maxim Smyatkin-Maxim left a comment

Choose a reason for hiding this comment

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

lgtm

@Smyatkin-Maxim
Copy link
Copy Markdown
Contributor

A side question about PAX not related to this pr: as far as I can see PAX relies on C++ memory allocators. It doesn't use PG memory contexts, right? This may become problematic for production cases with high memory usage when overcommit is off.

@gfphoenix78
Copy link
Copy Markdown
Contributor Author

A side question about PAX not related to this pr: as far as I can see PAX relies on C++ memory allocators. It doesn't use PG memory contexts, right? This may become problematic for production cases with high memory usage when overcommit is off.

Yes, the c++ code doesn't use PG memory context. Because we expect to scan multiple pax data file concurrently. High memory usage may throw an exception that will be caught by the wrapper code. When the wrapper catches an exception, it will raise a PG error, the c++ objects will be released in the clean stage.

@gfphoenix78 gfphoenix78 force-pushed the use-unique-ptr branch 3 times, most recently from 84a28c7 to 2860329 Compare July 15, 2025 07:01
Shared smart pointer will introduce additional cost, and it
also make the ownership complicated.
Use unique pointer to make the ownership explicitly visible.
The function OrcGroup::GetColumnValue() has a critical performance
issue cause by shared smart pointer. This commit replace the shared
pointer by unique pointer to eliminate the lock issue.
@gfphoenix78 gfphoenix78 merged commit 1cc7649 into apache:main Jul 17, 2025
26 checks passed
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.

3 participants