Add macro to assert or error

[mkindahl]

For some unexpected conditions, we have a check and an error that is generated. Since this always generate an error, it is more difficult to find the bug if the error is generated rather than an assert fired generating a core dump. Similarly, some asserts can occur in production builds and will lead to strange situations triggering a crash. For those cases we should instead generate an error.

This commit introduces a macro ENSURE that will result in an assert in debug builds, but an error message in release build. This macro should only be used for conditions that should not occur during normal runtime, but which can happen is odd corner-cases in release builds and therefore warrants an error message.

It also replaces some existing checks with such errors to demonstrate usage.

[codecov[bot]]

Codecov Report

Merging #4396 (2b98e41) into main (8cda0e1) will increase coverage by 0.05%. The diff coverage is 89.70%.

@@            Coverage Diff             @@
##             main    #4396      +/-   ##
==========================================
+ Coverage   90.95%   91.00%   +0.05%     
==========================================
  Files         224      224              
  Lines       42590    44100    +1510     
==========================================
+ Hits        38738    40135    +1397     
- Misses       3852     3965     +113     

Impacted Files	Coverage Δ
src/chunk_index.c	94.83% <ø> (+0.10%)	:arrow_up:
src/compat/compat.h	92.98% <ø> (ø)
src/copy.c	94.42% <ø> (-0.08%)	:arrow_down:
src/hypertable_restrict_info.c	94.86% <ø> (+0.01%)	:arrow_up:
src/loader/bgw_message_queue.c	88.81% <ø> (+0.66%)	:arrow_up:
src/process_utility.c	94.82% <ø> (+0.15%)	:arrow_up:
src/ts_catalog/catalog.h	100.00% <ø> (ø)
tsl/src/bgw_policy/policies_v2.c	96.05% <ø> (+0.05%)	:arrow_up:
tsl/src/chunk_api.c	95.06% <ø> (+0.15%)	:arrow_up:
tsl/src/compression/create.c	96.88% <ø> (+0.07%)	:arrow_up:
... and 228 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f55aaf0...2b98e41. Read the comment docs.

[mfundul]

This blurs the line between assertions and errors which we are already pretty bad at. There have been previous attempts to implement something similar and previously the agreement was to not have something like this for those reasons.

I kind of agree with this. We should probably make more effort to recovery from unexpected errors. It could be silent data corruption, or a bug that manifests with a similar way. The exception should contain more information than what an assertion condition looks like.

[mkindahl]

This blurs the line between assertions and errors which we are already pretty bad at. There have been previous attempts to implement something similar and previously the agreement was to not have something like this for those reasons.

I do not think there is a blurring of lines here. We already have checks that generate error for internal conditions that are not satisfied (because of bugs). Capturing them in Debug builds where you can get a proper stack trace to investigate is far superior to getting an error and much easier to work with. This still leaves the error printout for release builds, which is what we have now.

[mkindahl]

This blurs the line between assertions and errors which we are already pretty bad at. There have been previous attempts to implement something similar and previously the agreement was to not have something like this for those reasons.

I kind of agree with this. We should probably make more effort to recovery from unexpected errors. It could be silent data corruption, or a bug that manifests with a similar way. The exception should contain more information than what an assertion condition looks like.

See above. For release builds there is no difference compared to the current behavior, but for debug builds we get an assertion failure, which is significantly easier to work with.

[akuzm]

We could also make elog abort in debug. I did a similar thing for some other DBMS and it was very efficient in enforcing separation between "internal program logic errors" and "user/environment errors". Combined with fuzzing, even more so, because you know for sure if you hit something you shouldn't. Unfortunately it's postgres code so we can't change that. Your ASSERT_OR_ERROR does this and I think it's good, although the name is a bit unwieldy. Maybe Check?

[mkindahl]

We could also make elog abort in debug.

If we add an assert to elog it would mean that it aborts all the time and it is also missing the explicit check, which means that it cannot be incorporated into the error message as is done here.

I did a similar thing for some other DBMS and it was very efficient in enforcing separation between "internal program logic errors" and "user/environment errors".

Yes, I have used similar methods elsewhere I find them very suited to provide clear intention in coding and also helps with making the code brief.

Combined with fuzzing, even more so, because you know for sure if you hit something you shouldn't. Unfortunately it's postgres code so we can't change that. Your ASSERT_OR_ERROR does this and I think it's good, although the name is a bit unwieldy. Maybe Check?

I agree that the name is unwieldy. I suggested AssertOr with an error level as first argument, before but got a suggestion to use something like this name.

[mkindahl]

We could also make elog abort in debug. I did a similar thing for some other DBMS and it was very efficient in enforcing separation between "internal program logic errors" and "user/environment errors". Combined with fuzzing, even more so, because you know for sure if you hit something you shouldn't. Unfortunately it's postgres code so we can't change that. Your ASSERT_OR_ERROR does this and I think it's good, although the name is a bit unwieldy. Maybe Check?

I renamed the macro to ENSURE which seems less ambiguous since it is not as widely used.

[akuzm]

I'd prefer Ensure because all-caps is inconvenient to type.

[mkindahl]

I'd prefer Ensure because all-caps is inconvenient to type.

There is really no convention for how to type macros (although most seem to be all-caps), so following the convention of Assert on this one.