Re: extensible external toast tuple support

On Mon, Jul 1, 2013 at 3:49 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I must be missing something. At that point, yes, you'd like to avoid
>> re-toasting unnecessarily, but ISTM you've already bought the farm.
>> Unless I'm misunderstanding the code as written, you'd just end up
>> writing the indirect pointer back out to disk in that scenario.
>> That's not gonna work...
>
> You didn't misunderstand anything unfortunately. I broke that somewhere
> along the road, probably when factoring things out to
> toast_datum_differs(). Fixed...
> Which shows that we need tests. I've added some using a function in
> regress.c that makes all datums indirect. Together with a triggers that
> allows some testing.

OK, that's nifty.

+ elog(ERROR, "deleteing a tuple indirect datums doesn't make sense");

That's spelled wrong.

+ Assert(VARATT_IS_EXTERNAL_ONDISK(old_value));
+
+ /* fast path for the common case where we have the toast oid available */
+ if (VARATT_IS_EXTERNAL_ONDISK(old_value) &&
+ VARATT_IS_EXTERNAL_ONDISK(new_value))
+ return memcmp((char *) old_value, (char *) new_value,
+ VARSIZE_EXTERNAL(old_value)) != 0;

You've already asserted VARATT_IS_EXTERNAL_ONDISK(old_value), so there
can't be any need to test that condition again.

+ changed = memcmp(VARDATA_ANY(old_data), VARDATA_ANY(new_data),
+ VARSIZE_ANY_EXHDR(old_data)) != 0;
+ if (!changed)
+ *needs_rewrite = true;
+
+ /* heap_tuple_untoast_attr copied data */
+ pfree(old_data);
+ if (new_value != new_data)
+ pfree(new_data);
+ return changed;

So... basically, we always set needs_rewrite to !changed, except when
VARATT_IS_EXTERNAL_ONDISK(new_value). In that latter case we return
after doing memcmp(). That's kind of byzantine and could probably be
simplified by pushing the first if-clause of this function up into the
caller and then making the function's job to simply compare the datums
on the assumption that the new datum is not external on-disk. Then
you wouldn't need this funny three-way return value done as two
Booleans.

But backing up a minute, this is really a significant behavior change
that is independent of the purpose of the rest of this patch. What
you're proposing here is that every time we consider toasting a value
on update, we should first check whether it's byte-for-byte equivalent
to the old value. That may or may not be a good idea - it will suck
if, for example, a user repeatedly updates a very long string by
changing only the last character thereof, but it will win in other
cases. Whether it's a good idea or not, I think it deserves to be a
separate patch. For purposes of this patch, I think you should just
assume that any external-indirect value needs to be retoasted, just as
we currently assume for untoasted values.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company