On 2020-Jan-12, John Naylor wrote: > > I took a brief look at v11 to see if there's anything I can do to help > it move forward. I'm not yet sure how it would look code-wise to > implement Alvaro and Tomas's comments upthread, but I'm pretty sure > this part means the iterator-related structs are just as exposed as > before, but in a roundabout way that completely defeats the purpose of > hiding internals:
Agreed -- I think this patch still needs more work before being committable; I agree with John that the changes after v10 made it worse, not better. Rather than cross-including header files, it seems better to expose some struct definitions after all and let the main iterator interface (detoast_iterate) be a "static inline" function in detoast.h. So let's move forward with v10 (submitted on Sept 10th). Looking at that version, I don't think the function protos that were put in toast_internals.h should be there at all; I think they should be in detoast.h so that they can be used. But I don't like the fact that detoast.h now has to include genam.h; that seems pretty bogus. I think this can be fixed by moving the FetchDatumIteratorData struct definition (but not its typedef) to toast_internals.h. OTOH we've recently seen the TOAST interface (and header files) heavily reworked because of table-AM considerations, so probably this needs even more changes to avoid parts of it becoming heapam-dependant again. create_toast_buffer() doing two pallocs seems a waste. It could be a single one, + buf = (ToastBuffer *) palloc0(MAXALIGN(sizeof(ToastBuffer)) + size); + buf->buf = buf + MAXALIGN(sizeof(ToastBuffer)); (I'm not sure that the MAXALIGNs are strictly necessary there; I think we access the buf as four-byte aligned stuff somewhere in the toast innards, but maybe I'm wrong about that.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
