Re: [HACKERS] Custom compression methods

On Fri, Feb 19, 2021 at 4:21 PM Robert Haas <robertmhaas(at)gmail(dot)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