Convert *GetDatum() and DatumGet*() macros to inline functions
I once wrote code like this:
char *oid = get_from_somewhere();
...
values[i++] = ObjectIdGetDatum(oid);
This compiles cleanly and even appears to work in practice, except of
course it doesn't.
The FooGetDatum() macros just cast whatever you give it to Datum,
without checking whether the input was really foo.
To address this, I converted these macros to inline functions, which
enables type checking of the input argument. For symmetry, I also
converted the corresponding DatumGetFoo() macros (but those are less
likely to cover mistakes, since the input argument is always Datum).
This is patch 0002.
(I left some of the DatumGet... of the varlena types in fmgr.h as
macros. These ultimately map to functions that do type checking, so
there would be little more to be learnt from that. But we could do
those for consistency as well.)
This whole thing threw up a bunch of compiler warnings and errors, which
revealed a number of existing misuses. These are fixed in patch 0001.
These include
- using FooGetDatum on things that are already Datum,
- using DatumGetPointer on things that are already pointers,
- using PG_RETURN_TYPE on things that are Datum,
- using PG_RETURN_TYPE of the wrong type,
and others, including my personal favorite:
- using PointerGetDatum where DatumGetPointer should be used.
(AFAICT, unlike my initial example, I don't think any of those would
cause wrong behavior.)
Attachments:
0001-Fix-incorrect-uses-of-Datum-conversion-macros.patchtext/plain; charset=UTF-8; name=0001-Fix-incorrect-uses-of-Datum-conversion-macros.patchDownload+33-34
0002-Convert-GetDatum-and-DatumGet-macros-to-inline-funct.patchtext/plain; charset=UTF-8; name=0002-Convert-GetDatum-and-DatumGet-macros-to-inline-funct.patchDownload+655-187
Hi Peter,
To address this, I converted these macros to inline functions
This is a great change!
I encountered passing the wrong arguments to these macros many times,
and this is indeed pretty annoying. I wish we could forbid doing other
stupid things as well, e.g. comparing two Datum's directly, which for
Timestamps works just fine but only on 64-bit platforms. Although this
is certainly out of scope of this thread.
The patch looks good to me, I merely added a link to the discussion. I
added it to the CF application. Cfbot is making its mind at the moment
of writing.
Do you think this should be backported?
--
Best regards,
Aleksander Alekseev
Attachments:
v2-0001-Fix-incorrect-uses-of-Datum-conversion-macros.patchapplication/octet-stream; name=v2-0001-Fix-incorrect-uses-of-Datum-conversion-macros.patchDownload+33-34
v2-0002-Convert-GetDatum-and-DatumGet-macros-to-inline-fu.patchapplication/octet-stream; name=v2-0002-Convert-GetDatum-and-DatumGet-macros-to-inline-fu.patchDownload+655-187
Aleksander Alekseev <aleksander@timescale.com> writes:
Do you think this should be backported?
Impossible, it's an ABI break.
regards, tom lane
Hi Tom,
Do you think this should be backported?
Impossible, it's an ABI break.
OK, got it.
Just to clarify, a break in this case is going to be the fact that we
are adding new functions, although inlined, correct? Or maybe
something else? I'm sorry this is the first time I encounter the
question of ABI compatibility in the context of Postgres, so I would
appreciate it if you could elaborate a bit.
--
Best regards,
Aleksander Alekseev
Aleksander Alekseev <aleksander@timescale.com> writes:
Just to clarify, a break in this case is going to be the fact that we
are adding new functions, although inlined, correct? Or maybe
something else? I'm sorry this is the first time I encounter the
question of ABI compatibility in the context of Postgres, so I would
appreciate it if you could elaborate a bit.
After absorbing a bit more caffeine, I suppose that replacing a
macro with a "static inline" function would not be an ABI break,
at least not with most modern compilers, because the code should
end up the same. I'd still vote against back-patching though.
I don't think the risk-reward ratio is good, especially not for
the pre-C99 branches which don't necessarily have "inline".
regards, tom lane
On Tue, Aug 30, 2022 at 10:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Aleksander Alekseev <aleksander@timescale.com> writes:
Just to clarify, a break in this case is going to be the fact that we
are adding new functions, although inlined, correct? Or maybe
something else? I'm sorry this is the first time I encounter the
question of ABI compatibility in the context of Postgres, so I would
appreciate it if you could elaborate a bit.After absorbing a bit more caffeine, I suppose that replacing a
macro with a "static inline" function would not be an ABI break,
at least not with most modern compilers, because the code should
end up the same. I'd still vote against back-patching though.
I don't think the risk-reward ratio is good, especially not for
the pre-C99 branches which don't necessarily have "inline".
Yeah, I don't see a reason to back-patch a change like this, certainly
not right away. If over time it turns out that the different
definitions on different branches cause too many headaches, we could
reconsider. However, I'm not sure that will happen, because the whole
point is that the static inline functions are intended to behave in
the same way as the macros, just with better type-checking.
--
Robert Haas
EDB: http://www.enterprisedb.com
Tom, Robert,
Yeah, I don't see a reason to back-patch a change like this
Maybe we should consider backporting at least 0001 patch, partially
perhaps? I believe if fixes pretty cursed pieces of code, e.g:
```
pg_cryptohash_ctx *context =
- (pg_cryptohash_ctx *) PointerGetDatum(foundres);
+ (pg_cryptohash_ctx *) DatumGetPointer(foundres);
```This being said, personally I don't have a strong opinion here. After
all, the code works and passes the tests. Maybe I'm just being a
perfectionist here.
--
Best regards,
Aleksander Alekseev
On Tue, Aug 30, 2022 at 10:25 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
Yeah, I don't see a reason to back-patch a change like this
Maybe we should consider backporting at least 0001 patch, partially
perhaps? I believe if fixes pretty cursed pieces of code, e.g:``` pg_cryptohash_ctx *context = - (pg_cryptohash_ctx *) PointerGetDatum(foundres); + (pg_cryptohash_ctx *) DatumGetPointer(foundres); ```
Sure, back-porting the bug fixes would make sense to me.
--
Robert Haas
EDB: http://www.enterprisedb.com
Aleksander Alekseev <aleksander@timescale.com> writes:
Maybe we should consider backporting at least 0001 patch, partially
perhaps? I believe if fixes pretty cursed pieces of code, e.g:
Certainly if there are any parts of it that fix actual bugs,
we ought to backport those. I'm not in a big hurry to backport
cosmetic fixes though.
regards, tom lane
Hi hackers,
Cfbot is making its mind at the moment of writing.
Here is v3 with silenced compiler warnings.
--
Best regards,
Aleksander Alekseev
Attachments:
v3-0002-Convert-GetDatum-and-DatumGet-macros-to-inline-fu.patchapplication/octet-stream; name=v3-0002-Convert-GetDatum-and-DatumGet-macros-to-inline-fu.patchDownload+658-190
v3-0001-Fix-incorrect-uses-of-Datum-conversion-macros.patchapplication/octet-stream; name=v3-0001-Fix-incorrect-uses-of-Datum-conversion-macros.patchDownload+33-34
Hi hackers,
Here is v3 with silenced compiler warnings.
Some more warnings were reported by cfbot, so here is v4. Apologies
for the noise.
--
Best regards,
Aleksander Alekseev
Attachments:
v4-0001-Fix-incorrect-uses-of-Datum-conversion-macros.patchapplication/octet-stream; name=v4-0001-Fix-incorrect-uses-of-Datum-conversion-macros.patchDownload+33-34
v4-0002-Convert-GetDatum-and-DatumGet-macros-to-inline-fu.patchapplication/octet-stream; name=v4-0002-Convert-GetDatum-and-DatumGet-macros-to-inline-fu.patchDownload+659-191
On 30.08.22 20:15, Aleksander Alekseev wrote:
Here is v3 with silenced compiler warnings.
Some more warnings were reported by cfbot, so here is v4. Apologies
for the noise.
Looking at these warnings you are fixing, I think there is a small
problem we need to address.
I have defined PointerGetDatum() with a const argument:
PointerGetDatum(const void *X)
This is because in some places the thing that is being passed into that
is itself defined as const, so this is the clean way to avoid warnings
about dropping constness.
However, some support functions for gist and text search pass back
return values via pointer arguments, like
DirectFunctionCall3(g_int_same,
entry->key,
PointerGetDatum(query),
PointerGetDatum(&retval));
The compiler you are using apparently thinks that passing &retval to a
const pointer argument cannot change retval, which seems quite
reasonable. But that isn't actually what's happening here, so we're
lying a bit.
(Which compiler is that, by the way?)
I think to resolve that we could either
1. Not define PointerGetDatum() with a const argument, and just sprinkle
in a few unconstify calls where necessary.
2. Maybe add a NonconstPointerGetDatum() for those few cases where
pointer arguments are used for return values.
3. Go with your patch and just fix up the warnings about uninitialized
variables. But that seems the least principled to me.
Hi Peter,
Which compiler is that, by the way?
The warnings were reported by cfbot during the "clang_warning" step.
According to the logs:
```
using compiler=Debian clang version 11.0.1-2
```
Personally I use Clang 14 on MacOS and I don't get these warnings.
I think to resolve that we could either
1. Not define PointerGetDatum() with a const argument, and just sprinkle
in a few unconstify calls where necessary.2. Maybe add a NonconstPointerGetDatum() for those few cases where
pointer arguments are used for return values.3. Go with your patch and just fix up the warnings about uninitialized
variables. But that seems the least principled to me.
IMO the 3rd option is the lesser evil. Initializing four bools/ints in
order to make Clang 11 happy doesn't strike me as such a big deal. At
least until somebody reports a bottleneck for this particular reason.
We can optimize the code when and if this will happen.
--
Best regards,
Aleksander Alekseev
Hi Peter,
3. Go with your patch and just fix up the warnings about uninitialized
variables. But that seems the least principled to me.IMO the 3rd option is the lesser evil. Initializing four bools/ints in
order to make Clang 11 happy doesn't strike me as such a big deal. At
least until somebody reports a bottleneck for this particular reason.
We can optimize the code when and if this will happen.
Since the first patch was applied, cfbot now complains that it can't
apply the patchset. Here is the rebased version.
--
Best regards,
Aleksander Alekseev
Attachments:
v5-0001-Convert-GetDatum-and-DatumGet-macros-to-inline-fu.patchapplication/octet-stream; name=v5-0001-Convert-GetDatum-and-DatumGet-macros-to-inline-fu.patchDownload+659-191
On 08.09.22 11:26, Aleksander Alekseev wrote:
3. Go with your patch and just fix up the warnings about uninitialized
variables. But that seems the least principled to me.IMO the 3rd option is the lesser evil. Initializing four bools/ints in
order to make Clang 11 happy doesn't strike me as such a big deal. At
least until somebody reports a bottleneck for this particular reason.
We can optimize the code when and if this will happen.Since the first patch was applied, cfbot now complains that it can't
apply the patchset. Here is the rebased version.
committed, thanks
Hi,
On Mon, Sep 12, 2022 at 05:59:09PM +0200, Peter Eisentraut wrote:
committed, thanks
FTR lapwing is complaining about this commit:
https://brekka.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2022-09-12%2016%3A40%3A18.
Snapper is also failing with similar problems:
https://brekka.postgresql.org/cgi-bin/show_log.pl?nm=snapper&dt=2022-09-12%2016%3A42%3A10
On 12.09.22 19:03, Julien Rouhaud wrote:
On Mon, Sep 12, 2022 at 05:59:09PM +0200, Peter Eisentraut wrote:
committed, thanks
FTR lapwing is complaining about this commit:
https://brekka.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2022-09-12%2016%3A40%3A18.Snapper is also failing with similar problems:
https://brekka.postgresql.org/cgi-bin/show_log.pl?nm=snapper&dt=2022-09-12%2016%3A42%3A10
Ok, it has problems with 32-bit platforms. I can reproduce it locally.
I'll need to take another look at this. I have reverted the patch for now.
On 12.09.22 19:59, Peter Eisentraut wrote:
On 12.09.22 19:03, Julien Rouhaud wrote:
On Mon, Sep 12, 2022 at 05:59:09PM +0200, Peter Eisentraut wrote:
committed, thanks
FTR lapwing is complaining about this commit:
https://brekka.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2022-09-12%2016%3A40%3A18.Snapper is also failing with similar problems:
https://brekka.postgresql.org/cgi-bin/show_log.pl?nm=snapper&dt=2022-09-12%2016%3A42%3A10Ok, it has problems with 32-bit platforms. I can reproduce it locally.
I'll need to take another look at this. I have reverted the patch for now.
I have tried to analyze these issues, but I'm quite stuck. If anyone
else has any ideas, it would be helpful.
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
Ok, it has problems with 32-bit platforms. I can reproduce it locally.
I'll need to take another look at this. I have reverted the patch for now.
I have tried to analyze these issues, but I'm quite stuck. If anyone
else has any ideas, it would be helpful.
It looks to me like the problem is with the rewrite of Int64GetDatumFast
and Float8GetDatumFast:
+static inline Datum
+Int64GetDatumFast(int64 X)
+{
+#ifdef USE_FLOAT8_BYVAL
+ return Int64GetDatum(X);
+#else
+ return PointerGetDatum(&X);
+#endif
+}In the by-ref code path, this is going to return the address of the
parameter local variable, which of course is broken as soon as the
function exits. To test, I reverted the mods to those two macros,
and I got through check-world OK in a 32-bit VM.
I think we can do this while still having reasonable type-safety
by adding AssertVariableIsOfTypeMacro() checks to the macros.
An advantage of that solution is that we verify that the code
will be safe for a 32-bit build even in 64-bit builds. (Of
course, it's just checking the variable's type not its lifespan,
but this is still a step forward.)
0001 attached is what you committed, 0002 is a proposed delta
to fix the Fast macros.
regards, tom lane
On 26.09.22 19:34, Tom Lane wrote:
I think we can do this while still having reasonable type-safety
by adding AssertVariableIsOfTypeMacro() checks to the macros.
An advantage of that solution is that we verify that the code
will be safe for a 32-bit build even in 64-bit builds. (Of
course, it's just checking the variable's type not its lifespan,
but this is still a step forward.)0001 attached is what you committed, 0002 is a proposed delta
to fix the Fast macros.
Thanks, I committed it like that.
(I had looked into AssertVariableIsOfTypeMacro() for an earlier variant
of this patch, before I had the idea with the inline functions. It's in
general a bit too strict, such as with short vs int, and signed vs
unsigned, but it should work ok for this limited set of uses.)