On Wed, Nov 6, 2019 at 4:01 AM Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > This patch seems sound as far as the API restructuring goes.
Thanks. And thanks for weighing in. > If I may summarize the remaining discussion: This patch adds a field > toast_max_chunk_size to TableAmRoutine, to take the place of the > hardcoded TOAST_MAX_CHUNK_SIZE. The heapam_methods implementation then > sets this to TOAST_MAX_CHUNK_SIZE, thus preserving existing behavior. > Other table AMs can set this to some other value that they find > suitable. Currently, TOAST_MAX_CHUNK_SIZE is computed based on > heap-specific values and assumptions, so it's likely that other AMs > won't want to use that value. (Side note: Maybe rename > TOAST_MAX_CHUNK_SIZE then.) Yeah. > The concern was raised that while > TOAST_MAX_CHUNK_SIZE is stored in pg_control, values chosen by other > table AMs won't be, and so they won't have any safe-guards against > starting a server with incompatible disk layout. Then, various ways to > detect or check the TOAST chunk size at run time were discussed, but > none seemed satisfactory. Yeah. I've been nervous about trying to proceed with this patch because Andres seemed confident there was a better approach than what I did here, but as I wrote about back on September 12th, it doesn't seem like his idea will work. I'm not clear whether I'm being stupid and there's a way to salvage his idea, or whether he just made a mistake. One possible approach would be to move more of the logic below the tableam layer. For example, toast_fetch_datum() could do this: toastrel = table_open(toast_pointer.va_toastrelid, AccessShareLock); call_a_new_tableam_method_here(toast_rel, &toast_pointer); table_close(toastrel, AccessShareLock); ...and then it becomes the tableam's job to handle everything that needs to be done in the middle. That might be better than what I've got now; it's certainly more flexible. It does mean that an AM that just wants to reuse the existing logic with a different chunk size has got to repeat some code, but it's probably <~150 lines, so that's perhaps not a catastrophe. Alternatively, we could (a) stick with the current approach, (b) use the current approach but make the table AM member a callback rather than a constant, or (c) something else entirely. I don't want to give up on making the TOAST infrastructure pluggable; requiring every AM to use the heap as its TOAST implementation seems too constraining. > I think AMs are probably going to need a general mechanism to store > pg_control-like data somewhere. There are going to be chunk sizes, > block sizes, segment sizes, and so on. This one is just a particular > case of that. That's an interesting point. I don't know for sure to what extent we need that; I think that the toast chunk size is actually not very interesting to vary, and the fact that we technically allow it to be varied seems like it isn't buying us much. I think as much as possible we should allow settings that actually need to be varied to differ table-by-table, not require a recompile or re-initdb. But if we are going to have some that do require that, then what you're talking about here would certainly make that easier to secure. > This particular patch doesn't need to be held up by that, though. > Providing that mechanism can be a separate subproject of pluggable storage. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
