On 2019-03-11 13:31:26 -0700, Andres Freund wrote: > On 2019-03-11 12:37:46 -0700, Andres Freund wrote: > > Hi, > > > > On 2019-03-08 19:13:10 -0800, Andres Freund wrote: > > > Changes: > > > - I've added comments to all the callbacks in the first commit / the > > > scan commit > > > - I've renamed table_gimmegimmeslot to table_slot_create > > > - I've made the callbacks and their wrappers more consistently named > > > - I've added asserts for necessary callbacks in scan commit > > > - Lots of smaller cleanup > > > - Added a commit message > > > > > > While 0001 is pretty bulky, the interesting bits concentrate on a > > > comparatively small area. I'd appreciate if somebody could give the > > > comments added in tableam.h a read (both on callbacks, and their > > > wrappers, as they have different audiences). It'd make sense to first > > > read the commit message, to understand the goal (and I'd obviously also > > > appreciate suggestions for improvements there as well). > > > > > > I'm pretty happy with the current state of the scan patch. I plan to do > > > two more passes through it (formatting, comment polishing, etc. I don't > > > know of any functional changes needed), and then commit it, lest > > > somebody objects. > > > > Here's a further polished version. Pretty boring changes: > > - newlines > > - put tableam.h into the correct place > > - a few comment improvements, including typos > > - changed reorderqueue_push() to accept the slot. That avoids an > > unnecessary heap_copytuple() in some cases > > > > No meaningful changes in later patches. > > I pushed this. There's a failure on 32bit machines, unfortunately. The > problem comes from the ParallelTableScanDescData embedded in BTShared - > after the change the compiler can't see that that actually needs more > alignment, because ParallelTableScanDescData doesn't have any 8byte > members. That's a problem for just about all such "struct inheritance" > type tricks postgres, but we normally just allocate them separately, > guaranteeing maxalign. Given that we here already allocate enough space > after the BTShared struct, it's probably easiest to just not embed the > struct anymore.
I've pushed an attempt to fix this, which locally fixes 32bit builds. It's copying the alignment logic for shm_toc_allocate, namely using BUFFERALIGN for alignment. We should probably invent a more appropriate define for this at some point... Greetings, Andres Freund