4 ms·

From what I skimmed manually, not that many, but the code itself seems labyrinthical. Like, why have both Rust Try-supporting Error-like tagged union, but also

From what I skimmed manually, not that many, but the code itself seems labyrinthical. Like, why have both Rust Try-supporting Error-like tagged union, but also booleans, for error handling, in the same function? https://github.com/malisper/pgrust/blob/3646a73515a5e4ac7d0b7df88c61b180e1cabdb2/crates/backend/catalog/pg_inherits/src/lib.rs#L450 https://github.com/malisper/pgrust/blob/3646a73515a5e4ac7d0b... https://github.com/malisper/pgrust/blob/3646a73515a5e4ac7d0b7df88c61b180e1cabdb2/crates/backend/catalog/pg_inherits/src/lib.rs#L221 https://github.com/malisper/pgrust/blob/3646a73515a5e4ac7d0b...

malisper 3mo ago
I'm not sure what you mean? The rust code you're showing mimics the Postgres code: https://github.com/postgres/postgres/blob/2e6578292a9184dcaae6c5abc235a26db9cb2973/src/backend/catalog/pg_inherits.c#L356 https://github.com/postgres/postgres/blob/2e6578292a9184dcaa... The boolean being returned is the return value of the function. It's not used to return an error.
- iigijshaba 3mo ago
  Sorry, I wrongly assumed in the C code when I skimmed it that the boolean was for error handling, not the result value. The elog() macro is used for error handling.
- iigijshaba 3mo ago
  Now that I have taken a closer look, the code looks significantly better than it seemed at first glance, though there are still peculiarities, and some drawbacks. An unfortunate aspect is that the code has become a bit more bloated in some regards due to usage of Result, instead of an implicit elog() macro and similar. Passing Result around, in some ways as an alternative to an unwinding exception, is cleaner in some ways, but it also bloats the code somewhat. The rewrite also could have simpler code in some cases, like https://github.com/malisper/pgrust/blob/3646a73515a5e4ac7d0b7df88c61b180e1cabdb2/crates/backend/catalog/pg_inherits/src/lib.rs#L457 https://github.com/malisper/pgrust/blob/3646a73515a5e4ac7d0b... could perhaps just be match syscache_seams::search_pg_class_full_form::call(ctx.mcx(), relationId)? { Some(form) => Ok(form.relhassubclass), None => { Err(ereport(ERROR) .errmsg(format!("cache lookup failed for relation {relationId}")) .into_error()) } } but that is a smaller thing. I see a lot of MemoryContext. I am not sure how much that bloats the code (though the C code is bloated due to C's issues and problems, like re-using collections and such). Does it incur an overhead?
  - malisper 3mo ago
    > The rewrite also could have simpler code in some cases The Rust code is a literal translation of the Postgres code which returns the value at the end instead of an early return. > I see a lot of MemoryContext MemoryContext in C is used for multiple reasons: 1) performance 2) keeping track of how much memory has been allocated and where and 3) preventing memory leaks. Reasons 1 and 2 are still relevant for Rust. The challenge is in C memory contexts are stored in a global variable. Global variables don't work well with the rust borrow checker so I opted for passing memory contexts as function arguments instead.

malisper 3mo ago

I'm not sure what you mean? The rust code you're showing mimics the Postgres code: https://github.com/postgres/postgres/blob/2e6578292a9184dcaae6c5abc235a26db9cb2973/src/backend/catalog/pg_inherits.c#L356 https://github.com/postgres/postgres/blob/2e6578292a9184dcaa... The boolean being returned is the return value of the function. It's not used to return an error.

iigijshaba 3mo ago
Sorry, I wrongly assumed in the C code when I skimmed it that the boolean was for error handling, not the result value. The elog() macro is used for error handling.
iigijshaba 3mo ago
Now that I have taken a closer look, the code looks significantly better than it seemed at first glance, though there are still peculiarities, and some drawbacks. An unfortunate aspect is that the code has become a bit more bloated in some regards due to usage of Result, instead of an implicit elog() macro and similar. Passing Result around, in some ways as an alternative to an unwinding exception, is cleaner in some ways, but it also bloats the code somewhat. The rewrite also could have simpler code in some cases, like https://github.com/malisper/pgrust/blob/3646a73515a5e4ac7d0b7df88c61b180e1cabdb2/crates/backend/catalog/pg_inherits/src/lib.rs#L457 https://github.com/malisper/pgrust/blob/3646a73515a5e4ac7d0b... could perhaps just be match syscache_seams::search_pg_class_full_form::call(ctx.mcx(), relationId)? { Some(form) => Ok(form.relhassubclass), None => { Err(ereport(ERROR) .errmsg(format!("cache lookup failed for relation {relationId}")) .into_error()) } } but that is a smaller thing. I see a lot of MemoryContext. I am not sure how much that bloats the code (though the C code is bloated due to C's issues and problems, like re-using collections and such). Does it incur an overhead?
- malisper 3mo ago
  > The rewrite also could have simpler code in some cases The Rust code is a literal translation of the Postgres code which returns the value at the end instead of an early return. > I see a lot of MemoryContext MemoryContext in C is used for multiple reasons: 1) performance 2) keeping track of how much memory has been allocated and where and 3) preventing memory leaks. Reasons 1 and 2 are still relevant for Rust. The challenge is in C memory contexts are stored in a global variable. Global variables don't work well with the rust borrow checker so I opted for passing memory contexts as function arguments instead.