Horton

Thread

Commits

  1. Convert *GetDatum() and DatumGet*() macros to inline functions

  2. Fix incorrect uses of Datum conversion macros

  1. Convert *GetDatum() and DatumGet*() macros to inline functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-08-28T15:55:15Z

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

  2. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Aleksander Alekseev <aleksander@timescale.com> — 2022-08-30T13:36:11Z

    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

  3. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-30T13:46:35Z

    Aleksander Alekseev <aleksander@timescale.com> writes:
> Do you think this should be backported?

Impossible, it's an ABI break.

			regards, tom lane

  4. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Aleksander Alekseev <aleksander@timescale.com> — 2022-08-30T13:59:45Z

    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

  5. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-30T14:13:20Z

    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

  6. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Robert Haas <robertmhaas@gmail.com> — 2022-08-30T14:16:45Z

    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

  7. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Aleksander Alekseev <aleksander@timescale.com> — 2022-08-30T14:25:32Z

    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

  8. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Robert Haas <robertmhaas@gmail.com> — 2022-08-30T14:27:48Z

    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

  9. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-30T14:28:52Z

    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

  10. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Aleksander Alekseev <aleksander@timescale.com> — 2022-08-30T14:52:51Z

    Hi hackers,

> Cfbot is making its mind at the moment of writing.

Here is v3 with silenced compiler warnings.

-- 
Best regards,
Aleksander Alekseev

  11. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Aleksander Alekseev <aleksander@timescale.com> — 2022-08-30T18:15:40Z

    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

  12. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-09-05T12:57:10Z

    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.

  13. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Aleksander Alekseev <aleksander@timescale.com> — 2022-09-05T14:20:03Z

    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

  14. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Aleksander Alekseev <aleksander@timescale.com> — 2022-09-08T09:26:06Z

    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

  15. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-09-12T15:59:09Z

    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

  16. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Julien Rouhaud <rjuju123@gmail.com> — 2022-09-12T17:03:14Z

    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

  17. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-09-12T17:59:36Z

    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.

  18. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-09-26T14:55:22Z

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

I have tried to analyze these issues, but I'm quite stuck.  If anyone 
else has any ideas, it would be helpful.

  19. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-26T17:34:28Z

    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

  20. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-09-27T19:26:20Z

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

  21. Re: Convert *GetDatum() and DatumGet*() macros to inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-27T20:13:36Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> 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.

> (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.)

Yeah.  I had sort of expected to need a UInt64GetDatumFast variant
that would accept uint64, but there doesn't appear to be anyplace
that wants that today.  We should be willing to add it if anyone
complains, though.

			regards, tom lane