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

example(hostname): example extension to print system hostname #100

Merged
merged 13 commits into from
Oct 25, 2024

Conversation

divyenduz
Copy link
Contributor

@divyenduz divyenduz requested a review from urso October 21, 2024 18:49

fn pghostname_zig() ![]const u8 {
var buffer: [std.posix.HOST_NAME_MAX]u8 = undefined;
const hostname = try std.posix.gethostname(&buffer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
const hostname = try std.posix.gethostname(&buffer);
const hostname = std.posix.gethostname(&buffer) catch |err| switch (err) {
error.PermissionDenied => {
std.debug.print("Error getting hostname: permission denied\n", .{});
return "unknown"[0..];
},
error.Unexpected => {
std.debug.print("Error getting hostname: unexpected error\n", .{});
return "unknown"[0..];
},
};

This compiles but returns empty hostname in SQL (the elog before return works fine). I suspect that's a bug in pgzx? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why you get an empty hostname. I don't think this is pgzx, but Zig returning an empty string here. I haven't had any problems. But why do you need to return "unknown"[0..]? You are returning a string anyways.

For logging, don't use std.debug.print. Use elog to emit debug logs. This way you are using the postgres internal logging/reporting system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed via try pgzx.mem.PGCurrentContextAllocator.dupeZ(u8, hostname);, thanks for the help in slack.

@@ -0,0 +1,13 @@
.{
.name = "pg_audit_zig",
Copy link
Collaborator

Choose a reason for hiding this comment

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

oopsie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch, I copied it from char_count_zig, fixed in both places.

comptime {
pgzx.PG_MODULE_MAGIC();

pgzx.PG_FUNCTION_V1("pghostname_zig", pghostname_zig);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm... I haven't tried this, but I wonder if pgzx.PG_EXPORT(@This()) will do the trick (this will require you to declare your function as pub).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This worked ✅

@divyenduz divyenduz marked this pull request as ready for review October 23, 2024 17:28
pgzx.PG_EXPORT(@This());
}

pub fn pghostname_zig() ![]const u8 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: change the return type to [:0]const u8. By returning []const u8 you actually have implicitly casted the [:0]const u8 from dupeZ into a []const u8. As postgres requires proper zero terminated C strings we will implicitly allocate and copy the string again once your function returns. You can prevent the extra allocation and copying by returning [:0]const u8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this is done.

@urso urso merged commit a985f6b into main Oct 25, 2024
1 check passed
@urso urso deleted the example_pg_hostname branch October 25, 2024 10:26
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