Обсуждение: PageGetFreeSpace() isn't quite the right thing for some of its callers
I notice that a number of contrib modules call PageGetFreeSpace() where they should really call PageGetExactFreeSpace() instead. PageGetFreeSpace() assumes that the overhead for one line pointer should be pro-actively subtracted, which is handy for plenty of nbtree code, but doesn't quite make sense for stuff like pageinspect's bt_page_stats() function. I was thinking about fixing one or two of these buglets, without expecting that to be complicated in any way. However, now that I take a closer look I also notice that there is core code that calls PageGetFreeSpace() when it probably shouldn't, either. For example, what business does heap_xlog_visible() have calling PageGetFreeSpace()? I also doubt that terminate_brin_buildstate() should call it -- doesn't BRIN avoid using conventional item pointers? Doesn't GIN's entryIsEnoughSpace() function double-count the item pointer overhead? I wonder if we should add an assertion based on the pd_special offset of the page to PageGetFreeSpace(). That would make it harder to make mistakes like this in the future. Maybe it would be better to revise the whole API instead, though. -- Peter Geoghegan
Hi, On 2019-04-08 14:05:02 -0700, Peter Geoghegan wrote: > However, now that I take a closer look I also notice that there is > core code that calls PageGetFreeSpace() when it probably shouldn't, > either. For example, what business does heap_xlog_visible() have > calling PageGetFreeSpace()? I'm not sure I understand what the problem is. We got to get the information for the fsm from somewhere? Are you arguing we should instead have it included as an explicit xlog record payload? Or just that it should use PageGetExactFreeSpace()? I assume the former based on your "what business" language, but that'd not make terribly much sense to me. I don't think precision terribly matters here... Greetings, Andres Freund
On Mon, Apr 8, 2019 at 2:10 PM Andres Freund <andres@anarazel.de> wrote: > I'm not sure I understand what the problem is. We got to get the > information for the fsm from somewhere? Are you arguing we should > instead have it included as an explicit xlog record payload? No. I am simply pointing out that PageGetFreeSpace() "should usually only be used on index pages" according to its own comments. And yet it's called for other stuff. Maybe it's not that important in that one instance, but I find it pretty distracting that PageGetFreeSpace() is intended for index AMs that use conventional line pointers. -- Peter Geoghegan
On Mon, Apr 8, 2019 at 5:15 PM Peter Geoghegan <pg@bowt.ie> wrote: > On Mon, Apr 8, 2019 at 2:10 PM Andres Freund <andres@anarazel.de> wrote: > > I'm not sure I understand what the problem is. We got to get the > > information for the fsm from somewhere? Are you arguing we should > > instead have it included as an explicit xlog record payload? > > No. I am simply pointing out that PageGetFreeSpace() "should usually > only be used on index pages" according to its own comments. And yet > it's called for other stuff. > > Maybe it's not that important in that one instance, but I find it > pretty distracting that PageGetFreeSpace() is intended for index AMs > that use conventional line pointers. Maybe we should rename it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Apr 9, 2019 at 11:35 AM Robert Haas <robertmhaas@gmail.com> wrote: > Maybe we should rename it. There are only about 20 PageGetFreeSpace() callers, so that shouldn't be too disruptive. We might also need to rename PageGetFreeSpaceForMultipleTuples() and PageGetExactFreeSpace(). And, heapam.c won't be able to call whatever we rename PageGetFreeSpace() to. -- Peter Geoghegan