Re: ERROR: found unexpected null value in index

Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Jul 9, 2019 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm.  So maybe we need to teach it to ignore tuples that are not the tips
>> of their HOT chains?

> An approach like that was the first thing that I thought of, but I'll
> have to study the problem some more before coming up with a firm
> opinion.

I experimented with the attached.  It solves the reported problem and
passes check-world.  I made it just ignore any tuple for which
HeapTupleHeaderIsHotUpdated is true.  It might seem like there's a risk
of ignoring valid data, if the end tuple of a HOT chain is dead due to
a transaction abort, but since HeapTupleHeaderIsHotUpdated checks for
xmax validity I judge that the risk of that is small enough to be
acceptable.

A bigger problem with this is that in the tableam world, this seems
like a complete disaster modularity-wise.  I think it won't actually
fail --- non-heap AMs are probably not returning
BufferHeapTupleTableSlots, and even if they are, they shouldn't be
marking tuples HOT_UPDATED unless that concept applies to them.
But it sure seems like this leaves get_actual_variable_range() knowing
way more than it ought to about storage-level concerns.

Should we try to transpose some of this logic to below the AM API,
and if so, what should that look like?

            regards, tom lane

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index d7e3f09..b51cbed 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -5232,8 +5232,6 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
                                    InvalidOid,    /* no reg proc for this */
                                    (Datum) 0);    /* constant */

-            have_data = true;
-
             /* If min is requested ... */
             if (min)
             {
@@ -5262,15 +5260,33 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
                  * get_actual_variable_range() call will not have to visit
                  * that heap entry.  In this way we avoid repetitive work when
                  * this function is used a lot during planning.
+                 *
+                 * However, all is not perfect, because if the index was
+                 * recently created it may have entries pointing to broken HOT
+                 * chains that contain tuples that don't match the index entry
+                 * but aren't yet known-dead either.  We need to ignore such
+                 * tuples, since they might not be representative of the index
+                 * extremal values, indeed could even contain NULLs.  We
+                 * approximate this by ignoring any hot-updated tuples we see
+                 * and continuing to scan.
                  */
                 index_scan = index_beginscan(heapRel, indexRel,
                                              &SnapshotNonVacuumable,
                                              1, 0);
                 index_rescan(index_scan, scankeys, 1, NULL, 0);

-                /* Fetch first tuple in sortop's direction */
-                if (index_getnext_slot(index_scan, indexscandir, slot))
+                /* Fetch first/next tuple in sortop's direction */
+                while (index_getnext_slot(index_scan, indexscandir, slot))
                 {
+                    /* Ignore hot-updated tuples, per comment above */
+                    if (TTS_IS_BUFFERTUPLE(slot))
+                    {
+                        BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot;
+
+                        if (HeapTupleHeaderIsHotUpdated(bslot->base.tupdata.t_data))
+                            continue;
+                    }
+
                     /* Extract the index column values from the slot */
                     FormIndexDatum(indexInfo, slot, estate,
                                    values, isnull);
@@ -5284,24 +5300,40 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
                     MemoryContextSwitchTo(oldcontext);
                     *min = datumCopy(values[0], typByVal, typLen);
                     MemoryContextSwitchTo(tmpcontext);
+                    have_data = true;
+                    break;
                 }
-                else
-                    have_data = false;

                 index_endscan(index_scan);
             }
+            else
+            {
+                /* If min not requested, assume index has data */
+                have_data = true;
+            }

             /* If max is requested, and we didn't find the index is empty */
             if (max && have_data)
             {
+                have_data = false;
+
                 index_scan = index_beginscan(heapRel, indexRel,
                                              &SnapshotNonVacuumable,
                                              1, 0);
                 index_rescan(index_scan, scankeys, 1, NULL, 0);

-                /* Fetch first tuple in reverse direction */
-                if (index_getnext_slot(index_scan, -indexscandir, slot))
+                /* Fetch first/next tuple in reverse direction */
+                while (index_getnext_slot(index_scan, -indexscandir, slot))
                 {
+                    /* Ignore hot-updated tuples, per comment above */
+                    if (TTS_IS_BUFFERTUPLE(slot))
+                    {
+                        BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot;
+
+                        if (HeapTupleHeaderIsHotUpdated(bslot->base.tupdata.t_data))
+                            continue;
+                    }
+
                     /* Extract the index column values from the slot */
                     FormIndexDatum(indexInfo, slot, estate,
                                    values, isnull);
@@ -5315,9 +5347,9 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
                     MemoryContextSwitchTo(oldcontext);
                     *max = datumCopy(values[0], typByVal, typLen);
                     MemoryContextSwitchTo(tmpcontext);
+                    have_data = true;
+                    break;
                 }
-                else
-                    have_data = false;

                 index_endscan(index_scan);
             }