> IMHO, actually there are bug here. > ExecGetChildToRootMap is clear, is possible returning NULL. > To discover if the map is NULL, ExecGetChildToRootMap needs to process > "ResultRelInfo *leaf_part_rri". > So, the argument "if the map is NULL, this function should not be called", > is contradictory.
I was not explicit enough. I meant "if no map is needed to adjust columns, then this function should not be called". The caller already knows if it's needed or not; it doesn't depend on literally testing 'map'. If somebody mis-calls this function, it would have crashed, yes; but that's a caller bug, not this function's.
Thanks for the explanation.
A few days ago, the community Coverity also complained about this, so I added an Assert that the map is not null, which should silence it.
Thanks for hardening this.
> If the right fix is to return the original list, here is the patch attached.
... for a buggy caller (one that calls it when unnecessary), then yes this would be the correct code -- except that now the caller doesn't know if the returned list needs to be freed or not. So it seems better to avoid accumulating pointless calls to this function by just not coping with them.
Sure, it is always better to avoid doing work, unless strictly necessary.
