Re: undersized unions

Hi

On 2023-02-06 11:42:57 -0500, Robert Haas wrote:
> On Sun, Feb 5, 2023 at 6:28 AM Andres Freund <andres@anarazel.de> wrote:
> > On the other hand, it also just seems risky from a code writing perspective. It's not immediate obvious that it'd
beunsafe to create an on-stack Numeric by assigning *ptr. But it is.
 
>
> Well, I think that is pretty obvious: we have lots of things that are
> essentially variable-length types, and you can't put any of them on
> the stack.

There's a difference between a stack copy not containing all the data, and a
stack copy triggering undefined behaviour. But I agree, it's not something
that'll commonly endanger us.


> But I do also think that the Numeric situation is messier than some
> others we have got, and that's partly my fault, and it would be nice
> to make it better.
>
> I do not really know exactly how to do that, though. Our usual pattern
> is to just have a struct and end with a variable-length array, or
> alternatively add a comment says "other stuff follows!" at the end of
> the struct definition, without doing anything that C knows about at
> all. But here it's more complicated: there's a uint16 value for sure,
> and then maybe an int16 value, and then some number of NumericDigit
> values. That "maybe an int16 value" part is not something that C has a
> built-in way of representing, to my knowledge, which is why we end up
> with this hackish thing.

Perhaps something like

typedef struct NumericBase
{
    uint16        n_header;
} NumericBase;

typedef struct NumericData
{
    int32        vl_len_;        /* varlena header (do not touch directly!) */
    NumericBase data;
} NumericData;

/* subclass of NumericBase, needs to start in a compatible way */
typedef struct NumericLong
{
    uint16        n_sign_dscale;    /* Sign + display scale */
    int16        n_weight;        /* Weight of 1st digit    */
} NumericLong;

/* subclass of NumericBase, needs to start in a compatible way */
struct NumericShort
{
    uint16        n_header;        /* Sign + display scale + weight */
    NumericDigit n_data[FLEXIBLE_ARRAY_MEMBER]; /* Digits */
};

Macros that e.g. access n_long would need to cast, before they're able to
access n_long. So we'd end up with something like

#define NUMERIC_SHORT(n)  ((NumericShort *)&((n)->data))
#define NUMERIC_LONG(n)  ((NumericLong *)&((n)->data))

#define NUMERIC_WEIGHT(n)    (NUMERIC_HEADER_IS_SHORT((n)) ? \
    ((NUMERIC_SHORT(n)->n_header & NUMERIC_SHORT_WEIGHT_SIGN_MASK ? \
        ~NUMERIC_SHORT_WEIGHT_MASK : 0) \
     | (NUMERIC_SHORT(n)->n_header & NUMERIC_SHORT_WEIGHT_MASK)) \
    : (NUMERIC_LONG(n)->n_weight))

Although I'd actually be tempted to rip out all the casts but NUMERIC_LONG()
in this case, because it's all all just accessing the base n_header anyway.

Greetings,

Andres Freund