On Fri, Feb 19, 2021 at 11:12 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > I had an off list discussion with Robert and based on his suggestion > and a poc patch, I have come up with an updated version for handling > the composite type. Basically, the problem was that ExecEvalRow we > are first forming the tuple and then we are calling > HeapTupleHeaderGetDatum and then we again need to deform to find any > compressed data so that can cause huge performance penalty in all > unrelated paths which don't even contain any compressed data. So > Robert's idea was to check for the compressed/external data even > before forming the tuple. I have implemented that and I can see we > are not seeing any performance penalty.
I think that these performance tests aren't really exercising the expanded-record stuff, just the ExecEvalRow changes. We need to test that test case, and I tend to suspect there's going to be a measurable regression. I spent some time looking at how datums get into the expanded record system. There seem to be four possible paths: expanded_record_set_tuple(), make_expanded_record_from_datum(), expanded_record_set_field_internal(), and expanded_record_set_fields(). The first two of these inject an entire tuple, while the latter two work on a field-by-field basis. For that reason, the latter two are not really problematic. I'm not quite sure what the right thing to do is here, but if we wanted to check whether a Datum that we're absorbing is non-pglz-compressed in those places, it would be easy to do. Also, as far as I can see, make_expanded_record_from_datum() is completely unused. So the problem case is where expanded_record_set_tuple() is getting called, and specifically where it's being called with expand_external = true. Any place that it's being called with expand_external = false, there's apparently no problem with the result tuple containing external datums, so probably non-pglz compressed data is OK there too. All of the places that can potentially pass expand_external = true are actually passing !estate->atomic, where estate is a PLpgSQL_execstate. In other words, I think the case where this happens is when we're in a context where the computed value could need to survive across a COMMIT or ROLLBACK, like there may be a procedure running (maybe more than one, each invoking the next via CALL) but there are no queries in progress. We have to expand TOAST references because committing a transaction that deleted data means you might not be able to resolve the old TOAST pointer any more: even if you use a snapshot that can see everything, VACUUM could nuke the deleted rows - or some of them - at any time. To avoid trouble we have to un-externalize before any COMMIT or ROLLBACK occurs. That can suck for performance because we might be fetching a big value that we don't end up using for anything - say if the variable isn't used again - but it beats failing. The argument that we need to force decompression in such cases is considerably more tenuous. It revolves around the possibility that the compression AM itself has been dropped. As long as we have only built-in compression methods, which are undroppable, it seems like we could potentially just decide to do nothing at all about this issue. If the only reason for expanding TOAST pointers inside the expanded-record stuff is to avoid the possibility of one being invalidated by a transaction commit, and if compression methods can't be invalidated by a transaction commit, well then we don't really have a problem. That's not a great solution in terms of our chances of getting this whole patch series committed, but it might at least be enough to unblock the first few patches, and we could document the rest of the issue for later research. What makes me a bit uncomfortable about that approach is that it presupposes that everything that uses expanded records has some other defense against those tuples getting written to disk without first expanding any external datums. And it isn't obvious that this is the case, or at least not to me. For example, PLpgsql's coerce_function_result_tuple()'s code for VARATT_IS_EXTERNAL_EXPANDED() has three cases. The first case passes the tuple through SPI_returntuple() which calls heap_copy_tuple_as_datum() which calls toast_flatten_tuple_to_datum() if required, but the second case calls EOH_flatten_into() and does NOT pass the result through SPI_returntuple(). And ER_flatten_info() has no defense against this case that I can see: sure, it skips the fast path if ER_FLAG_HAVE_EXTERNAL is set, but that doesn't actually do anything to resolve TOAST pointers. Maybe there's no bug there for some reason, but I don't know what that reason might be. We seem to have no test cases either in the main test suite or in the plpgsql test suite where ER_flatten_info gets called with ER_FLAG_HAVE_EXTERNAL is set, which seems a little unfortunate. If there is such a bug here it's independent of this patch, I suppose, but it would still be nice to understand what's going on here better than I do. -- Robert Haas EDB: http://www.enterprisedb.com
