WAL log unchanged toasted replica identity key attributes.
authorAmit Kapila <[email protected]>
Mon, 14 Feb 2022 03:15:54 +0000 (08:45 +0530)
committerAmit Kapila <[email protected]>
Mon, 14 Feb 2022 03:15:54 +0000 (08:45 +0530)
Currently, during UPDATE, the unchanged replica identity key attributes
are not logged separately because they are getting logged as part of the
new tuple. But if they are stored externally then the untoasted values are
not getting logged as part of the new tuple and logical replication won't
be able to replicate such UPDATEs. So we need to log such attributes as
part of the old_key_tuple during UPDATE.

Reported-by: Haiying Tang
Author: Dilip Kumar and Amit Kapila
Reviewed-by: Alvaro Herrera, Haiying Tang, Andres Freund
Backpatch-through: 10
Discussion: https://postgr.es/m/OS0PR01MB611342D0A92D4F4BF26C0F47FB229@OS0PR01MB6113.jpnprd01.prod.outlook.com

contrib/test_decoding/expected/toast.out
doc/src/sgml/ref/alter_table.sgml
src/backend/access/heap/heapam.c

index 91a9a1e86d6c630d3b4dd12efe4f75eef090a492..a8e3676f47a3903818572932c734942e4b2af5a6 100644 (file)
@@ -77,7 +77,7 @@ SELECT substr(data, 1, 200) FROM pg_logical_slot_get_changes('regression_slot',
  table public.toasted_key: INSERT: id[integer]:1 toasted_key[text]:'1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123
  COMMIT
  BEGIN
- table public.toasted_key: UPDATE: id[integer]:1 toasted_key[text]:unchanged-toast-datum toasted_col1[text]:unchanged-toast-datum toasted_col2[text]:'987654321098765432109876543210987654321098765432109
+ table public.toasted_key: UPDATE: old-key: toasted_key[text]:'123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678
  COMMIT
  BEGIN
  table public.toasted_key: UPDATE: old-key: toasted_key[text]:'123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678
index 1279c19e81eff2141c6be7bd36cce306949f8a7d..0295d8fbd0c5438b46c42bc6cdf6a37c6eb2c73a 100644 (file)
@@ -736,10 +736,11 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
     <listitem>
      <para>
       This form changes the information which is written to the write-ahead log
-      to identify rows which are updated or deleted.  This option has no effect
-      except when logical replication is in use.
-      In all cases, no old values are logged unless at least one of the columns
-      that would be logged differs between the old and new versions of the row.
+      to identify rows which are updated or deleted.
+      In most cases, the old value of each column is only logged if it differs
+      from the new value; however, if the old value is stored externally, it is
+      always logged regardless of whether it changed.
+      This option has no effect except when logical replication is in use.
      <variablelist>
       <varlistentry>
        <term><literal>DEFAULT</literal></term>
index 144bbcdfe7c8dd2ccf9d798d52a454bd44528951..fcb14f2ac5bacfde9fa3d2620edf015a65c0d071 100644 (file)
@@ -96,9 +96,11 @@ static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf,
                Buffer newbuf, HeapTuple oldtup,
                HeapTuple newtup, HeapTuple old_key_tup,
                bool all_visible_cleared, bool new_all_visible_cleared);
-static Bitmapset *HeapDetermineModifiedColumns(Relation relation,
-                            Bitmapset *interesting_cols,
-                            HeapTuple oldtup, HeapTuple newtup);
+static Bitmapset *HeapDetermineColumnsInfo(Relation relation,
+                         Bitmapset *interesting_cols,
+                         Bitmapset *external_cols,
+                         HeapTuple oldtup, HeapTuple newtup,
+                         bool *has_external);
 static bool heap_acquire_tuplock(Relation relation, ItemPointer tid,
                     LockTupleMode mode, LockWaitPolicy wait_policy,
                     bool *have_tuple_lock);
@@ -122,7 +124,7 @@ static void MultiXactIdWait(MultiXactId multi, MultiXactStatus status, uint16 in
 static bool ConditionalMultiXactIdWait(MultiXactId multi, MultiXactStatus status,
                           uint16 infomask, Relation rel, int *remaining);
 static XLogRecPtr log_heap_new_cid(Relation relation, HeapTuple tup);
-static HeapTuple ExtractReplicaIdentity(Relation rel, HeapTuple tup, bool key_modified,
+static HeapTuple ExtractReplicaIdentity(Relation rel, HeapTuple tup, bool key_required,
                       bool *copy);
 
 
@@ -3510,6 +3512,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
    bool        all_visible_cleared_new = false;
    bool        checked_lockers;
    bool        locker_remains;
+   bool        id_has_external = false;
    TransactionId xmax_new_tuple,
                xmax_old_tuple;
    uint16      infomask_old_tuple,
@@ -3594,7 +3597,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
    Assert(ItemIdIsNormal(lp));
 
    /*
-    * Fill in enough data in oldtup for HeapDetermineModifiedColumns to work
+    * Fill in enough data in oldtup for HeapDetermineColumnsInfo to work
     * properly.
     */
    oldtup.t_tableOid = RelationGetRelid(relation);
@@ -3620,9 +3623,17 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
        Assert(!(newtup->t_data->t_infomask & HEAP_HASOID));
    }
 
-   /* Determine columns modified by the update. */
-   modified_attrs = HeapDetermineModifiedColumns(relation, interesting_attrs,
-                                                 &oldtup, newtup);
+   /*
+    * Determine columns modified by the update.  Additionally, identify
+    * whether any of the unmodified replica identity key attributes in the
+    * old tuple is externally stored or not.  This is required because for
+    * such attributes the flattened value won't be WAL logged as part of the
+    * new tuple so we must include it as part of the old_key_tuple.  See
+    * ExtractReplicaIdentity.
+    */
+   modified_attrs = HeapDetermineColumnsInfo(relation, interesting_attrs,
+                                             id_attrs, &oldtup,
+                                             newtup, &id_has_external);
 
    /*
     * If we're not updating any "key" column, we can grab a weaker lock type.
@@ -4201,10 +4212,12 @@ l2:
     * Compute replica identity tuple before entering the critical section so
     * we don't PANIC upon a memory allocation failure.
     * ExtractReplicaIdentity() will return NULL if nothing needs to be
-    * logged.
+    * logged.  Pass old key required as true only if the replica identity key
+    * columns are modified or it has external data.
     */
    old_key_tuple = ExtractReplicaIdentity(relation, &oldtup,
-                                          bms_overlap(modified_attrs, id_attrs),
+                                          bms_overlap(modified_attrs, id_attrs) ||
+                                          id_has_external,
                                           &old_key_copied);
 
    /* NO EREPORT(ERROR) from here till changes are logged */
@@ -4360,48 +4373,15 @@ l2:
 }
 
 /*
- * Check if the specified attribute's value is same in both given tuples.
- * Subroutine for HeapDetermineModifiedColumns.
+ * Check if the specified attribute's values are the same.  Subroutine for
+ * HeapDetermineColumnsInfo.
  */
 static bool
-heap_tuple_attr_equals(TupleDesc tupdesc, int attrnum,
-                      HeapTuple tup1, HeapTuple tup2)
+heap_attr_equals(TupleDesc tupdesc, int attrnum, Datum value1, Datum value2,
+               bool isnull1, bool isnull2)
 {
-   Datum       value1,
-               value2;
-   bool        isnull1,
-               isnull2;
    Form_pg_attribute att;
 
-   /*
-    * If it's a whole-tuple reference, say "not equal".  It's not really
-    * worth supporting this case, since it could only succeed after a no-op
-    * update, which is hardly a case worth optimizing for.
-    */
-   if (attrnum == 0)
-       return false;
-
-   /*
-    * Likewise, automatically say "not equal" for any system attribute other
-    * than OID and tableOID; we cannot expect these to be consistent in a HOT
-    * chain, or even to be set correctly yet in the new tuple.
-    */
-   if (attrnum < 0)
-   {
-       if (attrnum != ObjectIdAttributeNumber &&
-           attrnum != TableOidAttributeNumber)
-           return false;
-   }
-
-   /*
-    * Extract the corresponding values.  XXX this is pretty inefficient if
-    * there are many indexed columns.  Should HeapDetermineModifiedColumns do
-    * a single heap_deform_tuple call on each tuple, instead?  But that
-    * doesn't work for system columns ...
-    */
-   value1 = heap_getattr(tup1, attrnum, tupdesc, &isnull1);
-   value2 = heap_getattr(tup2, attrnum, tupdesc, &isnull2);
-
    /*
     * If one value is NULL and other is not, then they are certainly not
     * equal
@@ -4443,24 +4423,97 @@ heap_tuple_attr_equals(TupleDesc tupdesc, int attrnum,
  * Given an updated tuple, determine (and return into the output bitmapset),
  * from those listed as interesting, the set of columns that changed.
  *
- * The input bitmapset is destructively modified; that is OK since this is
- * invoked at most once in heap_update.
+ * has_external indicates if any of the unmodified attributes (from those
+ * listed as interesting) of the old tuple is a member of external_cols and is
+ * stored externally.
+ *
+ * The input interesting_cols bitmapset is destructively modified; that is OK
+ * since this is invoked at most once in heap_update.
  */
 static Bitmapset *
-HeapDetermineModifiedColumns(Relation relation, Bitmapset *interesting_cols,
-                            HeapTuple oldtup, HeapTuple newtup)
+HeapDetermineColumnsInfo(Relation relation,
+                        Bitmapset *interesting_cols,
+                        Bitmapset *external_cols,
+                        HeapTuple oldtup, HeapTuple newtup,
+                        bool *has_external)
 {
-   int         attnum;
+   int         attrnum;
    Bitmapset  *modified = NULL;
+   TupleDesc   tupdesc = RelationGetDescr(relation);
 
-   while ((attnum = bms_first_member(interesting_cols)) >= 0)
+   while ((attrnum = bms_first_member(interesting_cols)) >= 0)
    {
-       attnum += FirstLowInvalidHeapAttributeNumber;
+       Datum       value1,
+                   value2;
+       bool        isnull1,
+                   isnull2;
+
+       attrnum += FirstLowInvalidHeapAttributeNumber;
+
+       /*
+        * If it's a whole-tuple reference, say "not equal".  It's not really
+        * worth supporting this case, since it could only succeed after a
+        * no-op update, which is hardly a case worth optimizing for.
+        */
+       if (attrnum == 0)
+       {
+           modified = bms_add_member(modified,
+                                     attrnum -
+                                     FirstLowInvalidHeapAttributeNumber);
+           continue;
+       }
 
-       if (!heap_tuple_attr_equals(RelationGetDescr(relation),
-                                   attnum, oldtup, newtup))
+       /*
+        * Likewise, automatically say "not equal" for any system attribute
+        * other than OID and tableOID; we cannot expect these to be consistent
+        * in a HOT chain, or even to be set correctly yet in the new tuple.
+        */
+       if (attrnum < 0)
+       {
+           if (attrnum != ObjectIdAttributeNumber &&
+               attrnum != TableOidAttributeNumber)
+           {
+               modified = bms_add_member(modified,
+                                         attrnum -
+                                         FirstLowInvalidHeapAttributeNumber);
+               continue;
+           }
+       }
+
+       /*
+        * Extract the corresponding values.  XXX this is pretty inefficient
+        * if there are many indexed columns.  Should we do a single
+        * heap_deform_tuple call on each tuple, instead?   But that doesn't
+        * work for system columns ...
+        */
+       value1 = heap_getattr(oldtup, attrnum, tupdesc, &isnull1);
+       value2 = heap_getattr(newtup, attrnum, tupdesc, &isnull2);
+
+       if (!heap_attr_equals(tupdesc, attrnum, value1,
+                             value2, isnull1, isnull2))
+       {
            modified = bms_add_member(modified,
-                                     attnum - FirstLowInvalidHeapAttributeNumber);
+                                     attrnum -
+                                     FirstLowInvalidHeapAttributeNumber);
+           continue;
+       }
+
+       /*
+        * No need to check attributes that can't be stored externally. Note
+        * that system attributes can't be stored externally.
+        */
+       if (attrnum < 0 || isnull1 ||
+           TupleDescAttr(tupdesc, attrnum - 1)->attlen != -1)
+           continue;
+
+       /*
+        * Check if the old tuple's attribute is stored externally and is a
+        * member of external_cols.
+        */
+       if (VARATT_IS_EXTERNAL((struct varlena *) DatumGetPointer(value1)) &&
+           bms_is_member(attrnum - FirstLowInvalidHeapAttributeNumber,
+                         external_cols))
+           *has_external = true;
    }
 
    return modified;
@@ -7964,9 +8017,12 @@ log_heap_new_cid(Relation relation, HeapTuple tup)
  *
  * Returns NULL if there's no need to log an identity or if there's no suitable
  * key in the Relation relation.
+ *
+ * Pass key_required true if any replica identity columns changed value, or if
+ * any of them have any external data.  Delete must always pass true.
  */
 static HeapTuple
-ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed, bool *copy)
+ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_required, bool *copy)
 {
    TupleDesc   desc = RelationGetDescr(relation);
    Oid         replidindex;
@@ -8000,8 +8056,8 @@ ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed, bool *
        return tp;
    }
 
-   /* if the key hasn't changed and we're only logging the key, we're done */
-   if (!key_changed)
+   /* if the key isn't required and we're only logging the key, we're done */
+   if (!key_required)
        return NULL;
 
    /* find the replica identity index */