Обсуждение: Don't allocate IndexAmRoutine dynamically?
Hi, I think it might be worthwhile require that IndexAmRoutine returned by amhandler are allocated statically. Right now we copy them into local/cache memory contexts. That's not free and reduces branch/jump target prediction rates. For tableam we did the same, and that was actually measurable. It seems to me like there's not that many index AMs out there, so changing the signature of amhandler() to require returning a const pointer to a const object ought to both be enough of a warning, and not too big a burden. Comments? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> I think it might be worthwhile require that IndexAmRoutine returned by
> amhandler are allocated statically.
+1. Could only be an issue if somebody were tempted to have time-varying
entries in them, but it's hard to see why that could be a good idea.
Should we enforce this for *all* handler objects? If only index AMs,
why only them?
> It seems to me like there's not that many index AMs out there, so
> changing the signature of amhandler() to require returning a const
> pointer to a const object ought to both be enough of a warning, and not
> too big a burden.
One too many "consts" there. Pointer to const object seems fine.
The other part is either meaningless or will cause problems.
regards, tom lane
Hi, On 2019-06-25 16:15:17 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > I think it might be worthwhile require that IndexAmRoutine returned by > > amhandler are allocated statically. > > +1. Could only be an issue if somebody were tempted to have time-varying > entries in them, but it's hard to see why that could be a good idea. Yea, that seems like a use case we wouldn't want to support. If something like that is needed, they ought to store it in the relcache. > Should we enforce this for *all* handler objects? If only index AMs, > why only them? Well, tableams do that already. Other than indexam and tableam I think there's also FDW and TSM routines - are there any others? Changing the FDW API seems like it'd incur some work to a lot more people than any of the others - I'm not sure it's worth it. > > It seems to me like there's not that many index AMs out there, so > > changing the signature of amhandler() to require returning a const > > pointer to a const object ought to both be enough of a warning, and not > > too big a burden. > > One too many "consts" there. Pointer to const object seems fine. > The other part is either meaningless or will cause problems. Yea - I was thinking of the pointer in RelationData, where having it as const *Routine const; would make sense (but it's annoying to do without invoking technically undefined behaviour, doing ugly things with memcpy or duplicating struct definitions). Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2019-06-25 16:15:17 -0400, Tom Lane wrote:
>> One too many "consts" there. Pointer to const object seems fine.
>> The other part is either meaningless or will cause problems.
> Yea - I was thinking of the pointer in RelationData, where having it as
> const *Routine const; would make sense (but it's annoying to do without
> invoking technically undefined behaviour, doing ugly things with memcpy
> or duplicating struct definitions).
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?
regards, tom lane
Hi, On 2019-06-25 17:25:12 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2019-06-25 16:15:17 -0400, Tom Lane wrote: > >> One too many "consts" there. Pointer to const object seems fine. > >> The other part is either meaningless or will cause problems. > > > Yea - I was thinking of the pointer in RelationData, where having it as > > const *Routine const; would make sense (but it's annoying to do without > > invoking technically undefined behaviour, doing ugly things with memcpy > > or duplicating struct definitions). > > 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. ... Which, in my reading, appears to mean that in the case of dynamically allocated memory, the underlying memory can just be initialized ignoring the constness. At least before the object is used via the struct, but I'm not sure that strictly speaking matters. In the case of relcache it's a bit more complicated, because we copy over existing entries - but we don't ever actually change the constant fields, even though they're part of a memcpy.... Greetings, Andres Freund
Andres Freund <andres@anarazel.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". 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.
regards, tom lane
Hi, On June 25, 2019 5:53:47 PM EDT, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Andres Freund <andres@anarazel.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 mallochas 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 couldbe undefined. But that's obviously somewhat ugly. And it's not that clear whether it ever could be problematic for cache entry rebuildcases, 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 benice. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.