Re: Don't allocate IndexAmRoutine dynamically?

From: Andres Freund <andres(at)anarazel(dot)de>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Don't allocate IndexAmRoutine dynamically?
Date: 2019-06-25 22:55:43
Message-ID: 03014654-D5D3-46A7-AD92-B64BC999FEDC@anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On June 25, 2019 5:53:47 PM EDT, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>Andres Freund <andres(at)anarazel(dot)de> writes:
>> On 2019-06-25 17:25:12 -0400, Tom Lane wrote:
>>> Yeah, I think trying to make such pointer fields "const", within
>>> structures that are otherwise not const, is just more trouble than
>it's
>>> worth. To start with, how will you assign the handler's output
>pointer
>>> to such a field?
>
>> Yea, it's annoying. C++ is slightly cleaner in this case, but it's
>still not
>> great. In most cases it's perfectly legal to cast the const away
>(that's
>> always legal) *and* write through that. The standard's requirement is
>> quite minimal - C99's 6.7.3 5) says:
>
>> If an attempt is made to modify an object defined with a
>> const-qualified type through use of an lvalue with non-
>> const-qualified type, the behavior is undefined. ...
>
>I'm not sure how you are parsing "the behavior is undefined" as "it's
>legal".

Because of "defined". There's no object defined that way for dynamic memory allocations, at the very least at the time malloc has been called, before the return value is casted to the target type. So I don't see how something like *(TableamRoutine**)((char*) malloced + offsetof(RelationData, tableamroutine)) = whatever; after the memory allocations could be undefined.

But that's obviously somewhat ugly. And it's not that clear whether it ever could be problematic for cache entry rebuild cases, at least theoretically (would a memcpy without changing values be invalid once used via RelationData be invalid? What if we ever wanted to allow changing the AM of a relation?).

> But in any case, I'm not on board with const-qualifying stuff
>if we just have to cast the const away in common situations. I think
>it'd be far more valuable to get to a state where cast-away-const can
>be made an error.

I'm not sure I agree that low level details inside relcache.c, while initially building an entry can really be considered "common". But I agree it's probably not worth const'ing the routines.

Don't think the compiler could actually use it for optimizations in this case. If it could, it might be worthwhile. E.g. not having to repeatedly read/dereference the routine pointer when repeatedly calling routine callbacks would sure be nice.

Andres

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2019-06-25 23:22:05 Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Previous Message Tom Lane 2019-06-25 21:53:47 Re: Don't allocate IndexAmRoutine dynamically?