Re: [PATCH v4] Make NumericVar storage semantics explicit

On Sun, May 10, 2026 at 10:12 AM ChenhuiMo <chenhuimo(dot)mch(at)qq(dot)com> 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