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

postgres: do not copy and linearize received data when it is not going to be used #13393

Merged
merged 10 commits into from
Oct 16, 2020

Conversation

cpakulski
Copy link
Contributor

@cpakulski cpakulski commented Oct 5, 2020

Commit Message:
When the decoder analyzes postgres messages in order to produce statistics, the message body needs to be linearized and copied to the internal buffer.

From around 40 types of postgres messages, only about 8-10 are used to produce stats, but linearization and copying to internal buffer was done for all messages. Code in this PR checks if linearization and copying is needed.

Also, after linearization, the decoder stored the linearized message until the next message arrived instead of releasing the memory immediately after processing. Code in this PR releases memory after the message has been processed.

Additional Description:

Risk Level: Low

Testing: Adjusted unit test to verify that memory is released after processing the message.

Docs Changes: No

Release Notes: No

[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

going to be processed.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Buffer::linearize when received message is not used for processing.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
@cpakulski cpakulski requested a review from dio October 7, 2020 21:50
@dio
Copy link
Member

dio commented Oct 7, 2020

Looks good. Asking help from @zuercher for senior maintainer review. Thanks!

zuercher
zuercher previously approved these changes Oct 12, 2020
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

lgtm

@dio
Copy link
Member

dio commented Oct 12, 2020

Thanks, @zuercher!

@cpakulski, sorry, seems like docs build is hanging. See if merging main could help.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
…nce).

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
@cpakulski
Copy link
Contributor Author

@zuercher I need your "lgtm" again. Nothing really changed from the last review. I had to remove one mock method because it was removed from the base class. Thanks!

@dio
Copy link
Member

dio commented Oct 16, 2020

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Check envoy-presubmit didn't fail.

🐱

Caused by: a #13393 (comment) was created by @dio.

see: more, trace.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
@mattklein123
Copy link
Member

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13393 (comment) was created by @mattklein123.

see: more, trace.

@mattklein123 mattklein123 merged commit c97c0f3 into envoyproxy:master Oct 16, 2020
mpuncel added a commit to mpuncel/envoy that referenced this pull request Oct 17, 2020
* master: (22 commits)
  delay health checks until transport socket secrets are ready. (envoyproxy#13516)
  test, oauth2: Make sure config test runs field validation (envoyproxy#13496)
  [http] swap codec implementations to default new (envoyproxy#13579)
  wasm: update proxy-wasm-cpp-host (envoyproxy#13606)
  postgres: do not copy and linearize received data when it is not going to be used (envoyproxy#13393)
  configs: Update configs v2 -> v3 (envoyproxy#13562)
  http2: Remove RELEASE_ASSERTs in sendPendingFrames() error handling (envoyproxy#13546)
  dependencies: track untracked implied dependencies, wrapup dashboard. (envoyproxy#13571)
  listener: add match all filter chain (envoyproxy#13449)
  fix mistakes in docstrings (envoyproxy#13603)
  ratelimit: add route entry metadata to ratelimit actions (envoyproxy#13269)
  cluster manager: avoid immediate activation for dynamic inserted cluster when initialize (envoyproxy#12783)
  ext_authz: Avoid calling check multiple times (envoyproxy#13288)
  docs: Unexclude remaining configs from validation (envoyproxy#13534)
  build: update rules_rust to allow Rustc in RBE (envoyproxy#13595)
  docs: Update sphinxext.rediraffe (envoyproxy#13589)
  Deprecate moonjit support on Windows before beta (envoyproxy#13541)
  dependencies: bump LuaJIT to 2.1 branch HEAD @ e9af1ab. (envoyproxy#13474)
  docs: add TLS stats to cluster stats doc (envoyproxy#13561)
  ci: stop building alpine-debug images in favor of ubuntu-based debug image (envoyproxy#13598)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
@cpakulski cpakulski deleted the skip-linearize branch October 21, 2020 22:25
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