On Sun, 3 May 2026 11:04:59 -0700
SATYANARAYANA NARLAPURAM <[email protected]> wrote:
Thank you for updating the patch!
There are a few comments on v2 patch.
In compute_expr_stats(),
+ PG_CATCH();
+ {
+ ExecDropSingleTupleTableSlot(slot);
+ FreeExecutorState(estate);
+ PG_RE_THROW();
}
Should we switch the context to expr_context before releasing
slot and estate? Also, should we call MemoryContextDelete(expr_context)
to release palloc'ed memory in the loop?
(In fact, the error would not be cached in compute_expr_stats(), though,
since it would be cached in make_build_data() if any.)
Should we switch to expr_context before releasing slot and estate?
Also, should we call MemoryContextDelete(expr_context) to release
memory allocated by palloc() in the loop?
(In practice, errors would not catched in compute_expr_stats() itself,
since they would already be caught in make_build_data() if any.)
+ /*
+ * Wrap expression evaluation and stats computation in PG_TRY so
+ * that errors from evaluating expressions (e.g. division by zero
+ * in virtual generated columns) don't cause ANALYZE to fail
+ * entirely. Skip the statistics object and issue a WARNING
+ * instead.
How about rewriting the comments to reflect the more general case?
Extended statistics involving virtual generated columns are a somewhat
special case, while errors in expression statistics seem more common
in practice. The same applies to the commit message as well.
Regards,
Yugo Nagata
--
Yugo Nagata <[email protected]>
