Re: Datum as struct

Peter Eisentraut <peter@eisentraut.org> writes:
> But I think the patches 0001 through 0005 are useful now.
> I tested this against the original patch in [0].  It fixes some of the 
> issues discussed there but not all of them.

I looked through this.  I think 0001-0003 are unconditionally
improvements and you should just commit them.  Many of the
pointer-related changes are identical to ones that I had arrived at
while working on cleaning up the gcc warnings from my 8-byte-Datum
patch, so this would create merge conflicts with that patch, and
getting these changes in would make that one shorter.

I do have one review comment on 0003: in
brin_minmax_multi_distance_numeric, you wrote

-    PG_RETURN_FLOAT8(DirectFunctionCall1(numeric_float8, d));
+    PG_RETURN_FLOAT8(DatumGetFloat8(DirectFunctionCall1(numeric_float8, d)));

Another way to do that could be

    PG_RETURN_DATUM(DirectFunctionCall1(numeric_float8, d));

I'm not sure we should expect the compiler to be able to elide
the conversions to float8 and back, so I prefer the second way.

Also I see a "// XXX" in pg_get_aios, which I guess is a note
to confirm the data type to use for ioh_id?

My only reservation about 0004 is whether we want to do it like
this, or with the separate "_D" functions that I proposed in
the other thread.  Right now the vote seems to be 2 to 1 for
adding DatumGetPointer calls, so unless there are more votes
you should go ahead with 0004 as well.

No objection to 0005 either.

I agree that 0006 is not something we want to commit.  Aside
from all the replacements for "(Datum) 0" --- which'd be fine
I guess if there weren't so darn many --- it seems to me that
bits like this are punching too many holes in the abstraction:

-    if (PointerGetDatum(key) != entry->key)
+    if (PointerGetDatum(key).value != entry->key.value)

It'd probably be an improvement to code that like

    if ((Pointer) key != DatumGetPointer(entry->key))

which matches the way we do things in many other places.
But in general any direct reference to .value outside the
DatumGetFoo/FooGetDatum functions seems like an abstraction fail.

However, the sheer number of quasi-cosmetic issues you found
this way gives me pause.   If we don't have something like
0006 available for test purposes, more bugs of the same
ilk are surely going to creep in.

            regards, tom lane