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

add "show help" command #505

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Conversation

sebastianwebber
Copy link
Contributor

Hey Everyone, I'm new to Rust and this is an attempt to implement the show help command.

Currently, it has a bug in the package that I'm trying to figure out:

❯ PGPASSWORD=postgres psql -h 127.0.0.1 -p 6432 -U postgres pgbouncer -c 'show help';
NOTICE:  Console usage
DETAIL:
	SHOW HELP|CONFIG|DATABASES|POOLS|CLIENTS|SERVERS|USERS|VERSION
	SHOW LISTS
	SHOW STATS
	SET key = arg
	RELOAD
	PAUSE [<db>, <user>]
	RESUME [<db>, <user>]
	SHUTDOWN
message contents do not agree with length in message type "N"
SHOW

I've understood that pgcat is a replacement for pgbouncer, so I copied the help in the same format as pgbouncer does in admin.c.

Currently trying to figure what is the cause of the error below:

message contents do not agree with length in message type "N"


let mut res = BytesMut::new();
res.put_u8(b'N');
res.put_i32(notify_cmd.len() as i32 + 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Subtract one (-1). The length of the message does not include the message identifier (N).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when chaging to +3, like this:

diff --git a/src/messages.rs b/src/messages.rs
index cb3b595..b1cd3f9 100644
--- a/src/messages.rs
+++ b/src/messages.rs
@@ -541,7 +541,7 @@ pub fn notify(message: &str, details: String) -> BytesMut {
 
     let mut res = BytesMut::new();
     res.put_u8(b'N');
-    res.put_i32(notify_cmd.len() as i32 + 4);
+    res.put_i32(notify_cmd.len() as i32 + 3);
     res.put(notify_cmd);
 
     res

it drops the connection:


❯ PGPASSWORD=postgres psql -h 127.0.0.1 -p 6432 -U postgres pgbouncer -c 'show help';
NOTICE:  Console usage
DETAIL:
	SHOW HELP|CONFIG|DATABASES|POOLS|CLIENTS|SERVERS|USERS|VERSION
	SHOW LISTS
	SHOW STATS
	SET key = arg
	RELOAD
	PAUSE [<db>, <user>]
	RESUME [<db>, <user>]
	SHUTDOWN
message contents do not agree with length in message type "N"
lost synchronization with server: got message type "
connection to server was lost

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the log:

[2023-07-13T20:43:36.328145Z WARN  pgcat] Client disconnected with error SocketError("Error reading message code from socket - Error Kind(UnexpectedEof)")

Copy link
Contributor

@levkk levkk Jul 13, 2023

Choose a reason for hiding this comment

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

Ah nevermind, it must be something else. From the looks of it, your implementation seems correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you have any idea how to troubleshoot this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh! I think there is a missing NULL byte. This is how we format the error message (same format as notice): https://github.com/postgresml/pgcat/blob/main/src/messages.rs#L357-L358

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It worked! 💪

I've rebased and added a better commit message.

Is there anything else that you need me to change?

Copy link
Contributor

Choose a reason for hiding this comment

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

looks great!

This commit adds a new function to handle notify and use it
in the SHOW HELP command, which displays the available options
in the admin console.

Also, adding Fabrízio as a co-author for all the help with the
protocol and the help to structure this PR.

Signed-off-by: Sebastian Webber <sebastian@swebber.me>
Co-authored-by: Fabrízio de Royes Mello <fabriziomello@gmail.com>
@levkk levkk merged commit 15b6db8 into postgresml:main Jul 14, 2023
@sebastianwebber sebastianwebber deleted the add-admin-help branch July 14, 2023 12:13
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.

2 participants