public inbox for [email protected]
help / color / mirror / Atom feedFrom: Aleksander Alekseev <[email protected]>
To: pgsql-hackers <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: Define DatumGetInt8 function.
Date: Fri, 13 Mar 2026 13:55:44 +0300
Message-ID: <CAJ7c6TMcGu8qmRe1gZfJ-gOzVnZq-t=fwn-UuyStx1w6ZyydMw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CALdSSPhFyb9qLSHee73XtZm1CBWJNo9+JzFNf-zUEWCRW5yEiQ@mail.gmail.com>
<[email protected]>
<CALdSSPg6UgK+6LJmFQ6G3av4J6dbngN7=QwQEuFZApnpmXgVWQ@mail.gmail.com>
<CAApHDvoShMiWPSkV8zE3tu7uo9GEmuoe=gTpM0+GfOFh2iDmmw@mail.gmail.com>
<CALdSSPjmukA1WGyYVqJMPD8080Rm9oXPVLn5T64xFCj6wKAenQ@mail.gmail.com>
<CAJ7c6TPsaLW7O67vqv8YyV_qUUjF0pncDB7dCQJFDw0so7wrdw@mail.gmail.com>
<CALdSSPjSaTvQL28csVo-8hNg8xJ5=YFr6qwtF=JAMLb1eJO2gQ@mail.gmail.com>
<CAJ7c6TM4zecvHj0F9Xk-V1gKqjYQOR4EjerTVkrZ18+w-6JN3Q@mail.gmail.com>
<[email protected]>
Hi Peter,
Thanks for your feedback.
> I'm tempted to think the correct direction here would be to use uint8
> and its associated macros and functions more rigorously, not less.
OK, if my understanding is correct this leaves us char_increment() and
char_decrement() which currently use DatumGetUInt8 inconsistently with
CharGetDatum for the argument and return value correspondingly.
> I don't think it's worth making an isolated fix here for the one use of
> UInt8GetDatum()
I think you meant not this particular change so I included it to the
patch. We can keep nbtcompare.c as if you believe this change is not
that important (it arguably isn't).
> The change in pageinspect looks correct, but then when you look
> nearby and further around, you will find that there are also a bunch of
> (if not most) UInt16GetDatum and UInt32GetDatum that are wrong
> [...]
Agree. I did my best to fix all the inconsistencies in pageinsect.
> There are a lot of opportunities to clean this up, and I suspect that
> while many of these will work either way in practice because the actual
> values don't go far enough to hit the signed/unsigned boundary, I think
> there could a number of little bugs here to be fixed.
I believe you were referring to places other than pageinspect. I will
investigate and create separate threads with corresponding patches
later.
--
Best regards,
Aleksander Alekseev
Attachments:
[text/x-patch] v3-0001-Fix-several-Datum-conversion-inconsistencies.patch (8.9K, 2-v3-0001-Fix-several-Datum-conversion-inconsistencies.patch)
download | inline diff:
From 2b170fe6dd6f46b16802804019dc3c3aac0c2153 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Fri, 13 Mar 2026 12:51:56 +0300
Subject: [PATCH v3] Fix several Datum conversion inconsistencies
In nbtcompare.c make char_decrement() and char_increment() use UInt8GetDatum()
consistently with DatumGetUInt8() for return values.
In pageinspect/ use Datum conversion functions matching the SQL types -
Int16GetDatum for smallint columns, Int32GetDatum for integer columns, etc.
Author: Aleksander Alekseev <[email protected]>
Suggested-by: Peter Eisentraut <[email protected]>
Reviewed-by: TODO FIXME
Discussion: https://postgr.es/m/CALdSSPhFyb9qLSHee73XtZm1CBWJNo9%2BJzFNf-zUEWCRW5yEiQ%40mail.gmail.com
---
contrib/pageinspect/brinfuncs.c | 8 ++++----
contrib/pageinspect/btreefuncs.c | 4 ++--
contrib/pageinspect/ginfuncs.c | 4 ++--
contrib/pageinspect/gistfuncs.c | 8 ++++----
contrib/pageinspect/heapfuncs.c | 20 ++++++++++----------
contrib/pageinspect/rawpage.c | 14 +++++++-------
src/backend/access/nbtree/nbtcompare.c | 4 ++--
7 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index 26cf78252ed..ec64d3e4099 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -245,7 +245,7 @@ brin_page_items(PG_FUNCTION_ARGS)
if (unusedItem)
{
- values[0] = UInt16GetDatum(offset);
+ values[0] = Int32GetDatum(offset);
nulls[1] = true;
nulls[2] = true;
nulls[3] = true;
@@ -258,7 +258,7 @@ brin_page_items(PG_FUNCTION_ARGS)
{
int att = attno - 1;
- values[0] = UInt16GetDatum(offset);
+ values[0] = Int32GetDatum(offset);
switch (TupleDescAttr(rsinfo->setDesc, 1)->atttypid)
{
case INT8OID:
@@ -266,12 +266,12 @@ brin_page_items(PG_FUNCTION_ARGS)
break;
case INT4OID:
/* support for old extension version */
- values[1] = UInt32GetDatum(dtup->bt_blkno);
+ values[1] = Int32GetDatum(dtup->bt_blkno);
break;
default:
elog(ERROR, "incorrect output types");
}
- values[2] = UInt16GetDatum(attno);
+ values[2] = Int32GetDatum(attno);
values[3] = BoolGetDatum(dtup->bt_columns[att].bv_allnulls);
values[4] = BoolGetDatum(dtup->bt_columns[att].bv_hasnulls);
values[5] = BoolGetDatum(dtup->bt_placeholder);
diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index 0585b7cee40..3381e7cc2a7 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -507,9 +507,9 @@ bt_page_print_tuples(ua_page_items *uargs)
j = 0;
memset(nulls, 0, sizeof(nulls));
- values[j++] = UInt16GetDatum(offset);
+ values[j++] = Int16GetDatum(offset);
values[j++] = ItemPointerGetDatum(&itup->t_tid);
- values[j++] = Int32GetDatum((int) IndexTupleSize(itup));
+ values[j++] = Int16GetDatum(IndexTupleSize(itup));
values[j++] = BoolGetDatum(IndexTupleHasNulls(itup));
values[j++] = BoolGetDatum(IndexTupleHasVarwidths(itup));
diff --git a/contrib/pageinspect/ginfuncs.c b/contrib/pageinspect/ginfuncs.c
index b6574083b0a..e4461db7f0a 100644
--- a/contrib/pageinspect/ginfuncs.c
+++ b/contrib/pageinspect/ginfuncs.c
@@ -73,7 +73,7 @@ gin_metapage_info(PG_FUNCTION_ARGS)
values[0] = Int64GetDatum(metadata->head);
values[1] = Int64GetDatum(metadata->tail);
- values[2] = UInt32GetDatum(metadata->tailFreeSize);
+ values[2] = Int32GetDatum(metadata->tailFreeSize);
values[3] = Int64GetDatum(metadata->nPendingPages);
values[4] = Int64GetDatum(metadata->nPendingHeapTuples);
@@ -258,7 +258,7 @@ gin_leafpage_items(PG_FUNCTION_ARGS)
memset(nulls, 0, sizeof(nulls));
values[0] = ItemPointerGetDatum(&cur->first);
- values[1] = UInt16GetDatum(cur->nbytes);
+ values[1] = Int16GetDatum(cur->nbytes);
/* build an array of decoded item pointers */
tids = ginPostingListDecode(cur, &ndecoded);
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c
index a205cb8a334..cb491412d00 100644
--- a/contrib/pageinspect/gistfuncs.c
+++ b/contrib/pageinspect/gistfuncs.c
@@ -176,9 +176,9 @@ gist_page_items_bytea(PG_FUNCTION_ARGS)
memset(nulls, 0, sizeof(nulls));
- values[0] = UInt16GetDatum(offset);
+ values[0] = Int16GetDatum(offset);
values[1] = ItemPointerGetDatum(&itup->t_tid);
- values[2] = Int32GetDatum((int) IndexTupleSize(itup));
+ values[2] = Int16GetDatum(IndexTupleSize(itup));
tuple_bytea = (bytea *) palloc(tuple_len + VARHDRSZ);
SET_VARSIZE(tuple_bytea, tuple_len + VARHDRSZ);
@@ -283,9 +283,9 @@ gist_page_items(PG_FUNCTION_ARGS)
memset(nulls, 0, sizeof(nulls));
- values[0] = UInt16GetDatum(offset);
+ values[0] = Int16GetDatum(offset);
values[1] = ItemPointerGetDatum(&itup->t_tid);
- values[2] = Int32GetDatum((int) IndexTupleSize(itup));
+ values[2] = Int16GetDatum(IndexTupleSize(itup));
values[3] = BoolGetDatum(ItemIdIsDead(id));
if (index_columns)
diff --git a/contrib/pageinspect/heapfuncs.c b/contrib/pageinspect/heapfuncs.c
index 3a61954e1d9..1fba644aee8 100644
--- a/contrib/pageinspect/heapfuncs.c
+++ b/contrib/pageinspect/heapfuncs.c
@@ -189,10 +189,10 @@ heap_page_items(PG_FUNCTION_ARGS)
lp_flags = ItemIdGetFlags(id);
lp_len = ItemIdGetLength(id);
- values[0] = UInt16GetDatum(inter_call_data->offset);
- values[1] = UInt16GetDatum(lp_offset);
- values[2] = UInt16GetDatum(lp_flags);
- values[3] = UInt16GetDatum(lp_len);
+ values[0] = Int16GetDatum(inter_call_data->offset);
+ values[1] = Int16GetDatum(lp_offset);
+ values[2] = Int16GetDatum(lp_flags);
+ values[3] = Int16GetDatum(lp_len);
/*
* We do just enough validity checking to make sure we don't reference
@@ -209,14 +209,14 @@ heap_page_items(PG_FUNCTION_ARGS)
/* Extract information from the tuple header */
tuphdr = (HeapTupleHeader) PageGetItem(page, id);
- values[4] = UInt32GetDatum(HeapTupleHeaderGetRawXmin(tuphdr));
- values[5] = UInt32GetDatum(HeapTupleHeaderGetRawXmax(tuphdr));
+ values[4] = TransactionIdGetDatum(HeapTupleHeaderGetRawXmin(tuphdr));
+ values[5] = TransactionIdGetDatum(HeapTupleHeaderGetRawXmax(tuphdr));
/* shared with xvac */
- values[6] = UInt32GetDatum(HeapTupleHeaderGetRawCommandId(tuphdr));
+ values[6] = Int32GetDatum(HeapTupleHeaderGetRawCommandId(tuphdr));
values[7] = PointerGetDatum(&tuphdr->t_ctid);
- values[8] = UInt32GetDatum(tuphdr->t_infomask2);
- values[9] = UInt32GetDatum(tuphdr->t_infomask);
- values[10] = UInt8GetDatum(tuphdr->t_hoff);
+ values[8] = Int32GetDatum(tuphdr->t_infomask2);
+ values[9] = Int32GetDatum(tuphdr->t_infomask);
+ values[10] = Int16GetDatum(tuphdr->t_hoff);
/*
* We already checked that the item is completely within the raw
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 86fe245cac5..3f25a6d9ae1 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -285,8 +285,8 @@ page_header(PG_FUNCTION_ARGS)
}
else
values[0] = LSNGetDatum(lsn);
- values[1] = UInt16GetDatum(pageheader->pd_checksum);
- values[2] = UInt16GetDatum(pageheader->pd_flags);
+ values[1] = Int16GetDatum(pageheader->pd_checksum);
+ values[2] = Int16GetDatum(pageheader->pd_flags);
/* pageinspect >= 1.10 uses int4 instead of int2 for those fields */
switch (TupleDescAttr(tupdesc, 3)->atttypid)
@@ -295,10 +295,10 @@ page_header(PG_FUNCTION_ARGS)
Assert(TupleDescAttr(tupdesc, 4)->atttypid == INT2OID &&
TupleDescAttr(tupdesc, 5)->atttypid == INT2OID &&
TupleDescAttr(tupdesc, 6)->atttypid == INT2OID);
- values[3] = UInt16GetDatum(pageheader->pd_lower);
- values[4] = UInt16GetDatum(pageheader->pd_upper);
- values[5] = UInt16GetDatum(pageheader->pd_special);
- values[6] = UInt16GetDatum(PageGetPageSize(page));
+ values[3] = Int16GetDatum(pageheader->pd_lower);
+ values[4] = Int16GetDatum(pageheader->pd_upper);
+ values[5] = Int16GetDatum(pageheader->pd_special);
+ values[6] = Int16GetDatum(PageGetPageSize(page));
break;
case INT4OID:
Assert(TupleDescAttr(tupdesc, 4)->atttypid == INT4OID &&
@@ -314,7 +314,7 @@ page_header(PG_FUNCTION_ARGS)
break;
}
- values[7] = UInt16GetDatum(PageGetPageLayoutVersion(page));
+ values[7] = Int16GetDatum(PageGetPageLayoutVersion(page));
values[8] = TransactionIdGetDatum(pageheader->pd_prune_xid);
/* Build and return the tuple. */
diff --git a/src/backend/access/nbtree/nbtcompare.c b/src/backend/access/nbtree/nbtcompare.c
index 1d343377e98..0c198259e1f 100644
--- a/src/backend/access/nbtree/nbtcompare.c
+++ b/src/backend/access/nbtree/nbtcompare.c
@@ -631,7 +631,7 @@ char_decrement(Relation rel, Datum existing, bool *underflow)
}
*underflow = false;
- return CharGetDatum((uint8) cexisting - 1);
+ return UInt8GetDatum(cexisting - 1);
}
static Datum
@@ -647,7 +647,7 @@ char_increment(Relation rel, Datum existing, bool *overflow)
}
*overflow = false;
- return CharGetDatum((uint8) cexisting + 1);
+ return UInt8GetDatum(cexisting + 1);
}
Datum
--
2.43.0
view thread (10+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Define DatumGetInt8 function.
In-Reply-To: <CAJ7c6TMcGu8qmRe1gZfJ-gOzVnZq-t=fwn-UuyStx1w6ZyydMw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox