| From: | Michael Paquier <michael(at)paquier(dot)xyz> |
|---|---|
| To: | Peter Eisentraut <peter(at)eisentraut(dot)org> |
| Cc: | Aleksander Alekseev <aleksander(at)tigerdata(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Kirill Reshke <reshkekirill(at)gmail(dot)com>, David Rowley <dgrowleyml(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, melanieplageman(at)gmail(dot)com |
| Subject: | Re: Define DatumGetInt8 function. |
| Date: | 2026-04-28 05:13:31 |
| Message-ID: | afBB-yRQqxBASYIr@paquier.xyz |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Fri, Mar 27, 2026 at 01:17:15PM +0100, Peter Eisentraut wrote:
> I'm moving this patch to the next commitfest. It's worth continuing to work
> on making this more correct, but AFAICT no bug has been claimed here, so
> it's not worth rushing this now.
Well, as you have pointed out at [1], this is undoing a portion of
6dcfac9696cb that has changed a set of *GetDatum() functions to match
with the C type of the variables they work on, and not the output of
the function, which is incorrect. This means an open item standing on
top of my head for this release, that needs to be acted on.
The patch proposed at [2] handles the problem partially,
unfortunately. I have spotted a lot more places where we use a
*GetDatum() that does not match with the data type of the SQL function
related to. In terms of locations spotted, some notes:
- Of course, the.. cough.. places that 6dcfac9696cb has touched,
reverted now to their original form to match the SQL data type (BRIN,
pageinspect, GiST, lockfuncs.c, one in pg_buffercache).
- The IndexTupleSize() business in pageinspect is a good catch. All
these related to smallint, used a int32 GetDatum().
- Not completely sure about the ones in nbtcompare.c, TBH.
- lockfuncs.c is much more inconsistent than I thought, even after
reverting the bits of 6dcfac9696cb. pg_lock_status() was a mixed bag
of incorrectness (field3 for LOCKTAG_PAGE, field3 and field4 for
LOCKTAG_TUPLE, two more for PREDLOCKTAG_TUPLE).
- ginlogic.c, for triConsistentFn and consistentFmgrInfo.
nuserentries is a uint32, but the functions require Int32GetDatum() as
use int4 for the data type.
- xlogfuncs.c, pg_walfile_name_offset() should use Int32GetDatum() for
xrecoff.
- pg_proc.c, for pronargs and pronargdefaults. Quite an old one.
- pg_logicalinspect, 4 spots for the snapshot functions.
- pg_walinspect, for various record data.
- pgstatindex, pending pages.
In short, it took me some time to put some order into all that,
finishing with the attached patch set. 0001 is the minimum for v19,
that reverts 6dcfac9696cb so as we have more *GetDatum() matching with
the types in the SQL functions. That would take care of the open item
on top of my head.
0002 is a set of fixes that I have spotted while investigating this
set of issues in depth. These spots are actually wrong, some of them
for a long time. I would be slightly tempted to do something about
these in v19 rather than wait for v20, as these are somewhat latent
bugs, to have more consistency across the board. Has anybody from the
RMT an opinion to offer? There is not much urgency in it, still..
Added the RMT in CC for opinions.
[1]: https://www.postgresql.org/message-id/97f9375a-be61-4272-a44d-408337fe8fa6%40eisentraut.org
[2]: https://www.postgresql.org/message-id/CAJ7c6TMcGu8qmRe1gZfJ-gOzVnZq-t%3Dfwn-UuyStx1w6ZyydMw%40mail.gmail.com
--
Michael
| Attachment | Content-Type | Size |
|---|---|---|
| 0001-Revert-Use-more-consistent-GetDatum-macros-for-some-.patch | text/plain | 7.8 KB |
| 0002-Fix-more-Datum-conversion-inconsistencies.patch | text/plain | 16.3 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Alena Rybakina | 2026-04-28 05:28:45 | Re: Vacuum statistics |
| Previous Message | shveta malik | 2026-04-28 05:12:51 | Re: Proposal: Conflict log history table for Logical Replication |