Fix race condition in TupleDescCompactAttr assert code

5983a4cff added CompactAttribute as an abbreviated alternative to
FormData_pg_attribute to allow more cache-friendly processing in tasks
related to TupleDescs.  That commit contained some assert-only code to
check that the CompactAttribute had been populated correctly, however,
the method used to do that checking caused the TupleDesc's
CompactAttribute to be zeroed before it was repopulated and compared to
the snapshot taken before the memset call.  This caused issues as the type
cache caches TupleDescs in shared memory which can be used by multiple
backend processes at the same time.  There was a window of time between
the zero and repopulation of the CompactAttribute where another process
would mistakenly think that the CompactAttribute is invalid due to the
memset.

To fix this, instead of taking a snapshot of the CompactAttribute and
calling populate_compact_attribute() and comparing the snapshot to the
freshly populated TupleDesc's CompactAttribute, refactor things so we
can just populate a temporary CompactAttribute on the stack.  This way
we don't touch the TupleDesc's memory.

Reported-by: Alexander Lakhin, SQLsmith
Discussion: https://postgr.es/m/ca3a256a-5d12-42db-aabe-a75a030d9fb9@gmail.com

diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 9fec6e33865b7741347bc847972bba3a6fe7a18c..8cff6236e19c7a74f6ed9290f3d8b103d8d4f73f 100644 (file)
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -57,17 +57,13 @@ ResourceOwnerForgetTupleDesc(ResourceOwner owner, TupleDesc tupdesc)
 }
 
 /*
- * populate_compact_attribute
- *             Fill in the corresponding CompactAttribute element from the
- *             Form_pg_attribute for the given attribute number.  This must be called
- *             whenever a change is made to a Form_pg_attribute in the TupleDesc.
+ * populate_compact_attribute_internal
+ *             Helper function for populate_compact_attribute()
  */
-void
-populate_compact_attribute(TupleDesc tupdesc, int attnum)
+static inline void
+populate_compact_attribute_internal(Form_pg_attribute src,
+                                                                       CompactAttribute *dst)
 {
-       Form_pg_attribute src = TupleDescAttr(tupdesc, attnum);
-       CompactAttribute *dst = &tupdesc->compact_attrs[attnum];
-
        memset(dst, 0, sizeof(CompactAttribute));
 
        dst->attcacheoff = -1;
@@ -101,6 +97,63 @@ populate_compact_attribute(TupleDesc tupdesc, int attnum)
        }
 }
 
+/*
+ * populate_compact_attribute
+ *             Fill in the corresponding CompactAttribute element from the
+ *             Form_pg_attribute for the given attribute number.  This must be called
+ *             whenever a change is made to a Form_pg_attribute in the TupleDesc.
+ */
+void
+populate_compact_attribute(TupleDesc tupdesc, int attnum)
+{
+       Form_pg_attribute src = TupleDescAttr(tupdesc, attnum);
+       CompactAttribute *dst;
+
+       /*
+        * Don't use TupleDescCompactAttr to prevent infinite recursion in assert
+        * builds.
+        */
+       dst = &tupdesc->compact_attrs[attnum];
+
+       populate_compact_attribute_internal(src, dst);
+}
+
+#ifdef USE_ASSERT_CHECKING
+/*
+ * verify_compact_attribute
+ *             In Assert enabled builds, we verify that the CompactAttribute is
+ *             populated correctly.  This helps find bugs in places such as ALTER
+ *             TABLE where code makes changes to the FormData_pg_attribute but
+ *             forgets to call populate_compact_attribute().
+ *
+ * This is used in TupleDescCompactAttr(), but declared here to allow access
+ * to populate_compact_attribute_internal().
+ */
+void
+verify_compact_attribute(TupleDesc tupdesc, int attnum)
+{
+       CompactAttribute *cattr = &tupdesc->compact_attrs[attnum];
+       Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum);
+       CompactAttribute tmp;
+
+       /*
+        * Populate the temporary CompactAttribute from the corresponding
+        * Form_pg_attribute
+        */
+       populate_compact_attribute_internal(attr, &tmp);
+
+       /*
+        * Make the attcacheoff match since it's been reset to -1 by
+        * populate_compact_attribute_internal.
+        */
+       tmp.attcacheoff = cattr->attcacheoff;
+
+       /* Check the freshly populated CompactAttribute matches the TupleDesc's */
+       Assert(memcmp(&tmp, cattr, sizeof(CompactAttribute)) == 0);
+}
+#endif
+
+
 /*
  * CreateTemplateTupleDesc
  *             This function allocates an empty tuple descriptor structure.

diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index e61a4affa461cbba585328f01c72e86bb554c8e3..05ccd2465fc0acc30f486a85b163bd1772babc82 100644 (file)
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -158,6 +158,10 @@ TupleDescAttr(TupleDesc tupdesc, int i)
 
 #undef TupleDescAttrAddress
 
+#ifdef USE_ASSERT_CHECKING
+extern void verify_compact_attribute(TupleDesc, int attnum);
+#endif
+
 /*
  * Accessor for the i'th CompactAttribute element of tupdesc.
  */
@@ -165,30 +169,11 @@ static inline CompactAttribute *
 TupleDescCompactAttr(TupleDesc tupdesc, int i)
 {
        CompactAttribute *cattr = &tupdesc->compact_attrs[i];
-#ifdef USE_ASSERT_CHECKING
-       CompactAttribute snapshot;
 
-       /*
-        * In Assert enabled builds we verify that the CompactAttribute is
-        * populated correctly.  This helps find bugs in places such as ALTER
-        * TABLE where code makes changes to the FormData_pg_attribute but forgets
-        * to call populate_compact_attribute.
-        */
-
-       /*
-        * Take a snapshot of how the CompactAttribute is now before calling
-        * populate_compact_attribute to make it up-to-date with the
-        * FormData_pg_attribute.
-        */
-       memcpy(&snapshot, cattr, sizeof(CompactAttribute));
-
-       populate_compact_attribute(tupdesc, i);
-
-       /* reset attcacheoff back to what it was */
-       cattr->attcacheoff = snapshot.attcacheoff;
+#ifdef USE_ASSERT_CHECKING
 
-       /* Ensure the snapshot matches the freshly populated CompactAttribute */
-       Assert(memcmp(&snapshot, cattr, sizeof(CompactAttribute)) == 0);
+       /* Check that the CompactAttribute is correctly populated */
+       verify_compact_attribute(tupdesc, i);
 #endif
 
        return cattr;