On 06/11/2023 12:43, Peter Eisentraut wrote:
> It looks like this patch set needs a bit of surgery to adapt to the LLVM
> changes in 9dce22033d. The cfbot is reporting compiler warnings about
> this, and also some crashes, which might also be caused by this.
Updated patch set attached. I fixed those LLVM crashes, and reordered
the fields in the ResourceOwner struct per Andres' suggestion.
> I do like the updated APIs. (Maybe the repeated ".DebugPrint = NULL,
> /* default message is fine */" lines could be omitted?)
>
> I like that one can now easily change the elog(WARNING) in
> ResourceOwnerReleaseAll() to a PANIC or something to get automatic
> verification during testing. I wonder if we should make this the
> default if assertions are on? This would need some adjustments to
> src/test/modules/test_resowner because it would then fail.
Yeah, perhaps, but I'll leave that to possible a follow-up patch.
I feel pretty good about this overall. Barring objections or new cfbot
failures, I will commit this in the next few days.
--
Heikki Linnakangas
Neon (https://neon.tech)