Don't allocate IndexAmRoutine dynamically?

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