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
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
