Re: Caveats from reloption toast_tuple_target

On Tue, Apr 30, 2019 at 02:20:27PM +1200, David Rowley wrote:
> The options seem to be:
> 1. Make the lower limit of toast_tuple_target the same as
> TOAST_TUPLE_THRESHOLD; or
> 2. Require an AccessExclusiveLock when setting toast_tuple_target and
> call create_toast_table() to ensure we get a toast table when the
> setting is reduced to a level that means the target table may require
> a toast relation.

Actually, the patch I sent upthread does call create_toast_table() to
attempt to create a toast table. However it fails lamentably because
it lacks an exclusive lock when setting toast_tuple_target as you
outlined:
create table ab (a char(300));
alter table ab set (toast_tuple_target = 128);
insert into ab select string_agg('', md5(random()::text))
from generate_series(1,10); -- 288 bytes

This fails for the ALTER TABLE step like that:
ERROR: XX000: AccessExclusiveLock required to add toast table.

Now if I upgrade to AccessExclusiveLock then the thing is able to
toast tuples related to the lower threshold set. Here is the stack if
you are interested:
#0 create_toast_table (rel=0x7f8af648d178, toastOid=0,
toastIndexOid=0, reloptions=0, lockmode=8, check=true) at
toasting.c:131
#1 0x00005627da7a4eca in CheckAndCreateToastTable (relOid=16385,
reloptions=0, lockmode=8, check=true) at toasting.c:86
#2 0x00005627da7a4e1e in AlterTableCreateToastTable (relOid=16385,
reloptions=0, lockmode=8) at toasting.c:63
#3 0x00005627da87f479 in ATRewriteCatalogs (wqueue=0x7fffb77cfae8,
lockmode=8) at tablecmds.c:4185

> I sent a patch for #1, but maybe someone thinks we should do #2? The
> reason I've not explored #2 more is that after reading the original
> discussion when this patch was being developed, the main interest
> seemed to be keeping the values inline longer. Moving them out of
> line sooner seems to make less sense since smaller values are less
> likely to compress as well as larger values.

Yes, I have been chewing on the original thread from Simon, and it
seems really that he got interested in larger values when working on
this patch. And anyway, on HEAD we currently allow a toast table to
be created only if the threshold is at least TOAST_TUPLE_THRESHOLD,
so we have an inconsistency between reloptions.c and
needs_toast_table().

There could be an argument for allowing lower thresholds, but let's
see if somebody has a better use-case for it. In this case they would
need to upgrade the lock needed to set toast_tuple_target. I actually
don't have an argument in favor of that, thinking about it more.

Now, can we really increase the minimum value as you and Pavan
propose? For now anything between 128 and TOAST_TUPLE_TARGET gets
silently ignored, but if we increase the threshold as you propose we
could prevent some dumps to be restored, and as storage parameters are
defined as part of a WITH clause in CREATE TABLE, this could break
restores for a lot of users. We could tell pg_dump to enforce any
values between 128 and TOAST_TUPLE_THRESHOLD to be enforced to
TOAST_TUPLE_THRESHOLD, still that's a lot of complication just to take
care of one inconsistency.

Hence, based on that those arguments, there is option #3 to do
nothing. Perhaps the surrounding comments could make the current
behavior less confusing though.
--
Michael