On Fri, Feb 19, 2021 at 4:21 PM Robert Haas <robertmh...@gmail.com> wrote: > 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.
Andres just pointed out to me the error of my thinking here: ER_flatten_into can *never* encounter a case with both ER_FLAG_FVALUE_VALID and ER_FLAG_HAVE_EXTERNAL, because ER_get_flat_size has to get called first, and will de-toast external values as it goes. So there actually is justification for coerce_function_result_tuple() to skip the call to SPI_returntuple(). Given that, one might wonder why the test in ER_flatten_into() even cares about ER_FLAG_HAVE_EXTERNAL in the first place... I suppose it's just a harmless oversight. -- Robert Haas EDB: http://www.enterprisedb.com
