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

elog(breaking): more C-like API #73

Merged
merged 2 commits into from
Jun 27, 2024
Merged

elog(breaking): more C-like API #73

merged 2 commits into from
Jun 27, 2024

Conversation

urso
Copy link
Collaborator

@urso urso commented Jun 25, 2024

I found the original builder approach somewhat limiting and inefficient because we used to format strings before attempting to log (even if postgres did drop the log message).

Postgres elog API is somewhat complex, yet composable. When using C, APIs might also allow to report an optional soft error with errsave or ereturn. This cases didn't fit well with the existing Zig API.

The new implementation follows the C macro API more closely by using the functional options pattern. The reporting functions like ereport, ereportNoJump, errsave, ... do capture the options like errcode and errmsg in a struct. When reporting we call errstart which checks if the message is to be reported or not. Only if errstart returns true will we evaluate the options given by errstart or errmsg. This reduces CPU and memory usage especially for debugging logs.
Finally errfinish will emit the log and eventually execute a long jump or return a Zig error.

As the API now resembles the C-API more closely it is also easier to move the Postgres practices to Zig code more easiliy. The documentation in elog.h is (mostly?) true for the Zig implementation as well.

Example usage of new API

Error with long jump:

elog.ereport(src, .Error, .{
  elog.errcode(pg.ERRCODE_OUT_OF_MEMORY),
  elog.errmsg("Not enough memory", .{}),
}),

Error report emitting a Zig error (we use try because some error levels will not return an error, but void).

try elog.ereportNoJump(@src(), .Error, .{
  elog.errcode(pg.ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
  elog.errmsg("password or GSSAPI delegated credentials required", .{}),
  elog.errdetail("Non-superusers must delegate GSSAPI credentials or provide a password in the user mapping.", .{}),
});

Soft error support (will do a longjump using ERROR level if escontext is no ErrorSaveContext node type):

elog.errsave(@src(), escontext, .{
  elog.errcode(pg.ERRCODE_UNDEFINED_OBJECT),
  elog.errmsg("cluster \"{s}\" does not exist", .{name}),
});

@urso urso requested a review from tsg June 25, 2024 20:43
Copy link
Member

@tsg tsg left a comment

Choose a reason for hiding this comment

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

I like staying close to the originals in C.

I think this one will require updates to the README, right? What about the sample apps, do they need adjusting?

@urso
Copy link
Collaborator Author

urso commented Jun 26, 2024

Oh, didn't check the readme. Let me have a look.

The samples are all compiled and tested via CI. I fixed all operations that failed.
We still have the pgzx.elog.Error/Log/...(...) wrapper functions to reduce some boilerplate. These didn't change, but have become a little more efficient as they use the C based API now.

@urso
Copy link
Collaborator Author

urso commented Jun 26, 2024

we only mentioned the simple wrappers in the readme. Added a small note that we also provide the C like API.

@urso urso merged commit 3b6bec8 into xataio:main Jun 27, 2024
1 check passed
@urso urso deleted the elog-api branch July 29, 2024 12:09
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