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.

Sign up for GitHub

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

Jump to bottom

feat: support postgres query hook #51

Merged
merged 1 commit into from
May 3, 2022
Merged

feat: support postgres query hook #51

merged 1 commit into from
May 3, 2022

Conversation

yihong0618
Copy link
Contributor

@yihong0618 yihong0618 commented May 2, 2022 •
edited
Loading

Fix:#48 basic support for Postgres 10+ will do the return value in another PR if could

image

cfc4n
cfc4n reviewed May 2, 2022
View reviewed changes
cli/cmd/postgres.go Outdated

func init() {
postgresCmd.PersistentFlags().StringVarP(&postgresConfig.PostgresPath, "postgres", "m", "/usr/bin/postgres", "postgres binary file path, use to hook")
postgresCmd.PersistentFlags().Uint64VarP(&postgresConfig.Offset, "offset", "", 0, "0x710410")
Copy link
Member

Choose a reason for hiding this comment

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

These values should not be constant. Why not use symbol name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will take a look and change it ~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I think I should drop offset

cfc4n
cfc4n reviewed May 2, 2022
View reviewed changes
kern/postgres_kern.c Outdated
@@ -0,0 +1,42 @@
#include "ecapture.h"
#include "common.h"
Copy link
Member

Choose a reason for hiding this comment

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

It was included in ecapture.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy that

cfc4n
cfc4n reviewed May 2, 2022
View reviewed changes
Copy link
Member

@cfc4n cfc4n left a comment •
edited
Loading

Choose a reason for hiding this comment

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

How to know that It works with postgres binrary? Can you give some screenshots of catrure with postgres?

@cfc4n cfc4n added the enhancement New feature or request label May 2, 2022
cfc4n
cfc4n reviewed May 2, 2022
View reviewed changes
kern/postgres_kern.c
// versions 10 - now
// static void exec_simple_query(const char *query_string)
SEC("uprobe/exec_simple_query")
int postgres_query(struct pt_regs *ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

what is the difference between exec_simple_query and exec_parse_message, Why not exec_parse_message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exec_parse_message called in exec_simple_query.

cfc4n
cfc4n reviewed May 2, 2022
View reviewed changes
Copy link
Member

@cfc4n cfc4n left a comment

Choose a reason for hiding this comment

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

portgres use two function to exec queries in https://github.com/postgres/postgres/blob/7b7ed046cb2ad9f6efac90380757d5977f0f563f/src/backend/tcop/postgres.c#L4539 And https://github.com/postgres/postgres/blob/7b7ed046cb2ad9f6efac90380757d5977f0f563f/src/backend/tcop/postgres.c#L4571 whit params Q and P, why is that ? and this PR can capture all query commands?

@yihong0618
Copy link
Contributor Author

How to know that It works with postgres binrary? Can you give some screenshots of catrure with postgres?

Done, the screenshoot in the content now.

@yihong0618
Copy link
Contributor Author

yihong0618 commented May 2, 2022 •
edited
Loading

portgres use two function to exec queries in https://github.com/postgres/postgres/blob/7b7ed046cb2ad9f6efac90380757d5977f0f563f/src/backend/tcop/postgres.c#L4539 And https://github.com/postgres/postgres/blob/7b7ed046cb2ad9f6efac90380757d5977f0f563f/src/backend/tcop/postgres.c#L4571 whit params Q and P, why is that ? and this PR can capture all query commands?

yes, it capture all the query I think its the same logic as mysqld

And FYI, we can not hook exec_parse_message in bin/postgres if we did not add the trace
image

@yihong0618
Copy link
Contributor Author

@cfc4n Done and clean some code.

@cfc4n
Copy link
Member

cfc4n commented May 2, 2022

please merge all commits into one commit with command

git reset --soft HEAD~3
git commit
git push -f

cfc4n
cfc4n reviewed May 2, 2022
View reviewed changes
Copy link
Member

@cfc4n cfc4n left a comment

Choose a reason for hiding this comment

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

refer : https://www.internalpointers.com/post/squash-commits-into-one-git

@yihong0618
Copy link
Contributor Author

Of course I did that in last PR. #15 (comment)

And I think the the repo owner can use squash and merge which can also merge all commit into one.

@yihong0618
Copy link
Contributor Author

Done

@cfc4n
Copy link
Member

cfc4n commented May 3, 2022

LGTM, thanks.

@cfc4n cfc4n merged commit fe5ca3a into gojue:master May 3, 2022
@yihong0618
Copy link
Contributor Author

LGTM, thanks.

Will do some test on other env and add it to README

@yihong0618 yihong0618 mentioned this pull request Mar 21, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfc4n cfc4n cfc4n left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yihong0618 @cfc4n