On Tue, Feb 13, 2024 at 7:53 PM Heikki Linnakangas <hlinn...@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
