On Wed, Dec 18, 2019 at 10:59 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> On 2019-Dec-18, Robert Haas wrote:
> > - code: Declare values/nulls arrays only once at function scope
> > instead of 3x, and tighten up code, per Andres and self-review.
>
> Really small nit: I'd rather have the "nulls" assignment in all cases
> even when the previous value is still valid. This tight coding seems
> weirdly asymmetrical.
I agree that the asymmetry of it is a bit bothersome, but I think
having extra code setting things to null is worse. It makes the code
significantly bigger, which to me is more problematic than the
asymmetry.
> Can we please stop splitting this error message in two?
>
> + errmsg("materialize mode required, but it is not " \
> + "allowed in this context")));
>
> (What's with the newline escape there anyway?)
That message is like that everywhere in the tree, including the
escape, except for a couple of instances in contrib which deviate. If
you want to go change them all, feel free, and I'll adjust this to
match the then-prevailing style.
> Shmem structs are cacheline-aligned; the padding space is not AFAICS
> considered in ShmemIndexEnt->size. The reported size would be
> under-reported (or reported as "anonymous"?) I think we should include
> the alignment padding, for clarity.
It seems to me that you could plausibly define this view to show
either (a) the amount of space that the caller actually tried to
allocate or (b) the amount of space that the allocator decided to
allocate, after padding, and it's not obvious that (b) is a better
definition than (a).
That having been said, you're correct that the padding space is
currently reported as <anonymous>, and that does seem wrong.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company