On Tue, Feb 13, 2024 at 7:53 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> However, I must say that the dsm_impl_op() interface is absolutely
> insane. It's like someone looked at ioctl() and thought, "hey that's a
> great idea!".
As the person who wrote that code, this made me laugh.
I agree it's not the prettiest interface, but I thought that was OK
considering that it should only ever have a very limited number of
callers. I believe I did it this way in the interest of code
compactness. Since there are four DSM implementations, I wanted the
implementation-specific code to be short and all in one place, and
jamming it all into one function served that purpose. Also, there's a
bunch of logic that is shared by multiple operations - detach and
destroy tend to be similar, and so do create and attach, and there are
even things that are shared across all operations, like the snprintf
at the top of dsm_impl_posix() or the slightly larger amount of
boilerplate at the top of dsm_impl_sysv().
I'm not particularly opposed to refactoring this to make it nicer, but
my judgement was that splitting it up into one function per operation
per implementation, say, would have involved a lot of duplication of
small bits of code that might then get out of sync with each other
over time. By doing it this way, the logic is a bit tangled -- or
maybe more than a bit -- but there's very little duplication because
each implementation gets jammed into the smallest possible box. I'm OK
with somebody deciding that I got the trade-offs wrong there, but I
will be interested to see the number of lines added vs. removed in any
future refactoring patch.
--
Robert Haas
EDB: http://www.enterprisedb.com