Re: extensible external toast tuple support

On Mon, Jul 1, 2013 at 3:49 PM, Andres Freund <andres@2ndquadrant.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