On Sun, May 10, 2026 at 10:12 AM ChenhuiMo <[email protected]> wrote: > > Hi all, > > This patch aims to make the memory and ownership semantics of NumericVar > explicit in PostgreSQL, and to reorganize the related helper functions > accordingly, so that the rules for digit-buffer acquisition, release, > borrowing, and writing become clearer, more consistent, and easier > to maintain.
Hi, welcome to Postgres development! > The new memory model > -------------------- > > This patch introduces a more explicit NumericVar storage-state model to > describe the meaning of the current digit storage of a NumericVar. I think we can all agree that explicit is generally better than implicit, all else being equal. This patch, however, conflates that intention with other unrelated things. > In addition to the existing numeric metadata, NumericVar now has a few > state-related fields, including: > > capacity: the physical capacity of the current owned digit buffer; > state: the current storage state of the NumericVar; > borrow_kind: in borrowed state, which kind of storage the current digits > are borrowed from; > inline_buf: whether the variable is associated with a fixed-size inline home. So this goes from 2 storage model fields to 6. That makes it more difficult for someone wanting to learn how this works. Also, the new model states are redundant -- it seems the 'state' member is trivially derivable from the others? If so, that's a maintenance hazard, not an improvement . 0001: 1 file changed, 950 insertions(+), 248 deletions(-) This adds more code and more branches. That means there's more that the reader has to take in and understand. > This patch also introduces BufferedNumericVar, which wraps a NumericVar > together with a fixed-size inline buffer, used for: > > inline writable digit storage; > or, when needed, as workspace to hold a detoasted numeric datum. This seems like a performance optimization, contradicting the statement above: "Although this patch is not aimed at improving performance...". IIUC, a lot of the new machinery around this (the inline stack buffer, 'capacity', and some helper functions) all exist for performance (avoid palloc, compute in-place), not for clarity. There is a separate proposal to introduce an abstraction to wrap the pattern "use a stack var if possible, alloc if necessary" here: https://commitfest.postgresql.org/patch/6591/ ...so if that gets in, it would be easier to consider something like that here, but again that would be a performance optimization and a separate proposal. Out of curiosity, I ran this briefly through Claude Opus 4.8 and it found these issues (I haven't verified them, but #1 looks especially concerning given that 0002 greatly expands the tests): ``` 1. accum_sum_final leaks and corrupts state (near the end). The patch inserts alloc_var(&pos_var, …) / alloc_var(&neg_var, …), then two lines later overwrites pos_var.buf = pos_var.digits = digitbuf_alloc_raw(...). The alloc_var buffers are leaked, and now buf/digits/capacity/state are mutually inconsistent (capacity says ndigits+1 with a spare leading digit; the raw buffer is ndigits with digits == buf, no spare). The two new alloc_var calls look like they shouldn't be there at all. 2. Per-call leaks in the borrow paths. cmp_numerics, numeric_smaller, and numeric_larger create BufferedNumericVars (or detoast via PG_DETOAST_DATUM_PACKED) but never free_var/PG_FREE_IF_COPY them. For values that fit the inline buffer this is free, but when numeric_detoast_attr_to_buffer falls back to palloc (toasted/compressed or >~128-digit values), each comparison leaks into the surrounding context — and cmp_numerics runs once per sort comparison. 3. The NUMVAR_BORROW_INLINE in-place-overwrite contract is fragile. digitbuf_get converts an inline-borrowed view to OWNED_INLINE by resetting buf to the start of the inline buffer — overwriting the very bytes digits was pointing at. It's only safe because of an unwritten "caller guarantees the borrowed view isn't needed" promise. Worth confirming every caller actually honors it. 4. free_var no longer poisons sign. Old code set sign = NUMERIC_NAN on free as a use-after-free tripwire; this sets NUMERIC_POS. Minor, but it removes a debugging safety net and is a behavior change. 5. Redundant assignments hinting at uncertainty. set_var_from_str/set_var_from_num now re-set dest->ndigits and dest->digits = dest->buf + 1 immediately after alloc_var() already set them. Harmless but suggests the author wasn't sure alloc_var established them. ``` Note, I'm not requesting you go through and fix these, but I take this as a signal that this patch not only fails make things more clear, but actually increases the surface area for bugs. In short, a focused, genuine clarity-only change might be worth doing, but this patch is not the way to do it. I also have some more general advice for working with the community: 1. Please refrain from pinging us just because we haven't responded. We have a lot more proposals than we can consider, and it's getting worse because coding agents make it easy to generate vast volumes of code. 2. Further to the above, it helps greatly if you can justify every single change in the patch. This hunk: -#define init_var(v) memset(v, 0, sizeof(NumericVar)) +/* + * init_var() - + * + * Initialize a NumericVar in NUMVAR_EMPTY storage state, with no active digit + * storage, no borrowed digit view, and no current digit payload. + * + * This establishes a clean starting point for later transitions into + * borrowed or owned states. + */ +static inline void +init_var(NumericVar *v) +{ + memset(v, 0, sizeof(NumericVar)); +} ...is unnecessary, and things like this may just annoy the reviewer. It helps if you put forth the effort to make things easy for humans to read and evaluate. 3. Please don't top-post: Use in-line quoting style in emails. 4. Use version numbers for patches so we can tell them apart, e.g. `git format-patch master -v 3` -- John Naylor Amazon Web Services
