Hi, On 2019-04-04 16:15:54 -0300, Alvaro Herrera wrote: > On 2019-Apr-04, Andres Freund wrote: > > > I'm totally not OK with this from a layering > > POV. CheckAndSetAllVisibleBulkInsertState is entirely heap specific > > (without being named such), whereas all the heap specific bits are > > getting moved below tableam. > > This is a fair complaint, but on the other hand the COPY changes for > table AM are still being developed, so there's no ground on which to > rebase this patch. Do you have a timeline on getting the COPY one > committed?
~2h. Just pondering the naming of some functions etc. Don't think there's a large interdependency though. But even if tableam weren't committed, I'd still argue that it's structurally done wrong in the patch right now. FWIW, I actually think this whole approach isn't quite right - this shouldn't be done as a secondary action after we'd already inserted, with a separate lock-unlock cycle etc. Also, how is this code even close to correct? CheckAndSetPageAllVisible() modifies the buffer in a crucial way, and there's no WAL logging? Without even a comment arguing why that's OK (I don't think it is)? Greetings, Andres Freund
