Re: const correctness

Thomas Munro <munro(at)ip9(dot)org> writes:
> I am a long time user and fan of PostgreSQL and have built various
> projects large and small on every major release since 6.5. Recently
> I decided to try doing something more with the source than just
> compiling it, and spent some time 'constifying' some parts of the code
> as an exercise (this is an item I saw on the wiki to-do list, and I
> figured it might provide me with an educational traversal of the
> source tree).

There was some discussion of this just a couple days ago in connection
with a patch Peter offered ... did you see that?
http://archives.postgresql.org/pgsql-hackers/2011-11/msg00314.php

> Perhaps there should be a few more 'XXX_const' accessor function
> variants, for example list_nth_const,

This is exactly what was bothering Robert and me about Peter's patch.
If you go down this road you soon start needing duplicate functions
for no other reason than that one takes/returns "const" and one doesn't.
That might be acceptable on a very small scale, but any serious attempt
to const-ify the PG code is going to require it on a very large scale.
The maintenance costs of duplicate code are significant (not even
considering whether there's a performance hit), and it just doesn't seem
likely to be repaid in easier or safer development. Offhand I cannot
remember the last bug in PG which would have been prevented by better
const-ification.

So I'm starting to feel that this idea is a dead end, and we should take
it off the TODO list.

> [1] For functions built on top of expression_tree_walker (like
> expression_returns_set), it seems that it and therefore they can be
> modified to work entirely on pointers to const Node without any
> complaints from GCC at least, but only because the walker function
> pointer is of type bool (*)() (that is, pointer to a function with
> unspecified arguments, thereby side-stepping all static type
> checking). But I guess it would only be honest to change the
> signature of expression_tree_walker to take const Node * if the type
> of the walker function pointer were changed to bool (*)(const Node *,
> void *), and the walker mechanism were declared in the comments not to
> permit any mutation at all (rather than the weaker restriction that
> routines can modify nodes in-place but not add/delete/replace nodes).

Yeah, that was an alternative I considered when designing the tree
walker infrastructure. However, if we force walker functions to be
declared just like that, we lose rather than gain type safety. In the
typical usage, there's a startup function that sets up a context
structure and calls the walker function directly. As things stand,
that call is type-checked: you pass the wrong kind of context object,
you'll get a bleat. Changing the walkers to use void * would remove
that check, while adding a need for explicit casting of the argument
inside the walkers, and gain nothing of value.

As for the question of whether we should insist that walkers never
modify the tree ... yeah, we could, but there are enough instances
of nominal walkers that do modify the tree to make me not enthused
about it. We would have to change each one of those walkers to
instead create a new copy of the tree, with attendant memory consumption
and palloc overhead. It would almost certainly be a noticeable
performance hit, and the benefit seems debatable at best.

There would, I think, be both performance and safety benefits from
getting certain entire modules to not scribble on their input trees;
especially the planner. But that is a high-level consideration and
I'm not sure that const-decoration would really do much to help us
achieve it.

regards, tom lane