Horton

Re: Datum as struct

Michael Paquier <michael@paquier.xyz>

View thread View on postgr.es
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Eisentraut <peter@eisentraut.org>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-07-31T23:18:43Z
Lists: pgsql-hackers 
On Thu, Jul 31, 2025 at 01:17:54PM -0400, Tom Lane wrote:
> 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.

Quote from this message:
https://www.postgresql.org/message-id/1520348.1753891591@sss.pgh.pa.us

And relevant paragraph:

> I think the way forward here is to tackle that head-on and split the
> top-level macros into two static inlines, along the lines of
> VARDATA(Pointer ptr) and VARDATA_D(Datum dat) where the _D versions
> are simply DatumGetPointer and then call the non-D versions. 

Will reply on this thread with an option.  Spoiler: I have mixed
feelings about the addition of the _D flavors over the additions of
DatumGetPointer() in the existing calls.

@@ -144,7 +144,7 @@ toast_save_datum(Relation rel, Datum value,
     int            num_indexes;
     int            validIndex;
 
-    Assert(!VARATT_IS_EXTERNAL(value));
+    Assert(!VARATT_IS_EXTERNAL(dval));
[...]
+++ b/src/backend/replication/logical/proto.c
             if (isnull[i])
                 continue;
-            else if (VARATT_IS_EXTERNAL_ONDISK(value))
+            else if (VARATT_IS_EXTERNAL_ONDISK(DatumGetPointer(value)))
                 toast_delete_datum(rel, value, is_speculative);
[...]
+++ b/src/backend/replication/logical/proto.c
-		if (att->attlen == -1 && VARATT_IS_EXTERNAL_ONDISK(values[i]))
+		if (att->attlen == -1 && VARATT_IS_EXTERNAL_ONDISK(DatumGetPointer(values[i])))

I was wondering the impact of this change in terms of the refactoring
work I've been for TOAST at [1], which is something that I'm also
planning to get in committable shape very soon to ease the addition of
more on-disk external TOAST pointers.  When I've looked at the
problem, I've found the current non-distinction between Datums and
varlenas confusing, so forcing more control with types is an
improvement IMO.  At the end the impact is I think null, my stuff uses
varlenas in the refactored code rather than Datums. 

[1] https://www.postgresql.org/message-id/aFOnKHG7Wn-Srnpv@paquier.xyz
--
Michael

Commits

  1. Use consistent type for pgaio_io_get_id() result

  2. Mop-up for Datum conversion cleanups.

  3. Add missing Datum conversions

  4. Remove useless/superfluous Datum conversions

  5. Fix incorrect lack of Datum conversion in _int_matchsel()

  6. Improve tests of date_trunc() with infinity and unsupported units

  7. Fix incorrect Datum conversion in timestamptz_trunc_internal()

  8. Fix various hash function uses

  9. Fix mixups of FooGetDatum() vs. DatumGetFoo()