Thread: REL_12_STABLE crashing with assertion failure in ExtractReplicaIdentity

Hi,

On 2019-08-16 09:44:15 -0700, Hadi Moshayedi wrote:
> It seems that sometimes when DELETE cascades to referencing tables we fail
> to acquire locks on replica identity index.
> 
> To reproduce, set wal_level to logical, and run 1.sql.
> 
> I can look into this, but I thought first I should send it here in case
> someone who is more familiar with these related functions can solve it
> quickly.

I suspect this "always" has been broken, it's just that we previously
didn't have checks in place that detect these cases. I don't think it's
likely to cause actual harm, due to the locking on the table itself when
dropping indexes etc.  But we still should fix it.

The relevant code is:

        /*
         * If there are indices on the result relation, open them and save
         * descriptors in the result relation info, so that we can add new
         * index entries for the tuples we add/update.  We need not do this
         * for a DELETE, however, since deletion doesn't affect indexes. Also,
         * inside an EvalPlanQual operation, the indexes might be open
         * already, since we share the resultrel state with the original
         * query.
         */
        if (resultRelInfo->ri_RelationDesc->rd_rel->relhasindex &&
            operation != CMD_DELETE &&
            resultRelInfo->ri_IndexRelationDescs == NULL)
            ExecOpenIndices(resultRelInfo,
                            node->onConflictAction != ONCONFLICT_NONE);


I'm not quite sure what the best way to fix this would be however. It
seems like a bad idea to make locking dependent on wal_level, but I'm
also not sure we want to incur the price of locking one more table to
every delete on a table with a primary key?


Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-08-16 09:44:15 -0700, Hadi Moshayedi wrote:
>> It seems that sometimes when DELETE cascades to referencing tables we fail
>> to acquire locks on replica identity index.

> I suspect this "always" has been broken, it's just that we previously
> didn't have checks in place that detect these cases. I don't think it's
> likely to cause actual harm, due to the locking on the table itself when
> dropping indexes etc.  But we still should fix it.

Yeah ... see the discussion leading up to 9c703c169,

https://www.postgresql.org/message-id/flat/19465.1541636036%40sss.pgh.pa.us

We didn't pull the trigger on removing the CMD_DELETE exception here,
but I think the handwriting has been on the wall for some time.

            regards, tom lane

Hi,

On 2019-08-17 01:43:45 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-08-16 09:44:15 -0700, Hadi Moshayedi wrote:
> >> It seems that sometimes when DELETE cascades to referencing tables we fail
> >> to acquire locks on replica identity index.
>
> > I suspect this "always" has been broken, it's just that we previously
> > didn't have checks in place that detect these cases. I don't think it's
> > likely to cause actual harm, due to the locking on the table itself when
> > dropping indexes etc.  But we still should fix it.
>
> Yeah ... see the discussion leading up to 9c703c169,
>
> https://www.postgresql.org/message-id/flat/19465.1541636036%40sss.pgh.pa.us
>
> We didn't pull the trigger on removing the CMD_DELETE exception here,
> but I think the handwriting has been on the wall for some time.

ISTM there's a few different options here:

1a) We build all index infos, unconditionally. As argued in the thread
    you reference, future tableams may eventually require that anyway,
    by doing more proactive index maintenance somehow. Currently there's
    however no support for such AMs via tableam (mostly because I wasn't
    sure how exactly that'd look, and none of the already in-development
    AMs needed it).

2a) We separate acquisition of index locks from ExecOpenIndices(), and
    acquire index locks even for CMD_DELETE. Do so either during
    executor startup, or as part of AcquireExecutorLocks() (the latter
    on the basis that parsing/planning would have required the locks
    already).

There's also corresponding *b) options, where we only do additional work
for CMD_DELETE if wal_level = logical, and the table has a replica
identity requiring use of the index during deleteions. But I think
that's clearly enough a bad idea that we can just dismiss it out of
hand.


3)  Remove the CheckRelationLockedByMe() assert from
    ExtractReplicaIdentity(), at least for 12. I don't think this is an
    all that convicing option, but it'd reduce churn relatively late in
    beta.

4)  Add a index_open(RowExclusiveLock) to ExtractReplicaIdentity(). That
    seems very unconvincing to me, because we'd do so for every row.


I think there's some appeal in going towards 2), because batching lock
acquisition into a more central place has the chance to yield some
speedups on its own, but more importantly would allow for batched
operations one day. Centralizing lock acquisition also seems like it
might make things easier to understand than today, where a lot of
different parts of the system acquire the locks, even just for
execution.  But it also seems to be likely too invasive for 12 - making
me think that 1a) is the way to go for now.

Comments?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-08-17 01:43:45 -0400, Tom Lane wrote:
>> Yeah ... see the discussion leading up to 9c703c169,
>> https://www.postgresql.org/message-id/flat/19465.1541636036%40sss.pgh.pa.us
>> We didn't pull the trigger on removing the CMD_DELETE exception here,
>> but I think the handwriting has been on the wall for some time.

> ISTM there's a few different options here:

> 1a) We build all index infos, unconditionally. As argued in the thread
>     you reference, future tableams may eventually require that anyway,
>     by doing more proactive index maintenance somehow. Currently there's
>     however no support for such AMs via tableam (mostly because I wasn't
>     sure how exactly that'd look, and none of the already in-development
>     AMs needed it).
> 2a) We separate acquisition of index locks from ExecOpenIndices(), and
>     acquire index locks even for CMD_DELETE. Do so either during
>     executor startup, or as part of AcquireExecutorLocks() (the latter
>     on the basis that parsing/planning would have required the locks
>     already).
> There's also corresponding *b) options, where we only do additional work
> for CMD_DELETE if wal_level = logical, and the table has a replica
> identity requiring use of the index during deleteions. But I think
> that's clearly enough a bad idea that we can just dismiss it out of
> hand.
> 3)  Remove the CheckRelationLockedByMe() assert from
>     ExtractReplicaIdentity(), at least for 12. I don't think this is an
>     all that convicing option, but it'd reduce churn relatively late in
>     beta.
> 4)  Add a index_open(RowExclusiveLock) to ExtractReplicaIdentity(). That
>     seems very unconvincing to me, because we'd do so for every row.

As far as 4) goes, I think the code in ExtractReplicaIdentity is pretty
duff anyway, because it doesn't bother to check for the defined failure
return for RelationIdGetRelation.  But if we're concerned about the
cost of recalculating this stuff per-row, couldn't we cache it a little
better?  It should be safe to assume the set of index columns isn't
changing intra-query.

... in fact, isn't all the infrastructure for that present already?
Why is this code looking directly at the index at all, rather than
using the relcache's rd_idattr bitmap?

I suspect we'll have to do 1a) eventually anyway, but this particular
problem seems like it has a better solution.  Will try to produce a
patch in a bit.

            regards, tom lane

I wrote:
> As far as 4) goes, I think the code in ExtractReplicaIdentity is pretty
> duff anyway, because it doesn't bother to check for the defined failure
> return for RelationIdGetRelation.  But if we're concerned about the
> cost of recalculating this stuff per-row, couldn't we cache it a little
> better?  It should be safe to assume the set of index columns isn't
> changing intra-query.
> ... in fact, isn't all the infrastructure for that present already?
> Why is this code looking directly at the index at all, rather than
> using the relcache's rd_idattr bitmap?

Here's a proposed patch along those lines.  It fixes Hadi's original
crash case and passes check-world.

I'm a bit suspicious of the exclusion for idattrs being empty, but
if I remove that, some of the contrib/test_decoding test results
change.  Anybody want to comment on that?  If that's actually an
expected situation, why is there an elog(DEBUG) in that path?

            regards, tom lane

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index cb811d3..8a3c7dc 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -7594,18 +7594,20 @@ 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.
+ *
+ * *copy is set to true if the returned tuple is a modified copy rather than
+ * the same tuple that was passed in.
  */
 static HeapTuple
-ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed, bool *copy)
+ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed,
+                       bool *copy)
 {
     TupleDesc    desc = RelationGetDescr(relation);
-    Oid            replidindex;
-    Relation    idx_rel;
     char        replident = relation->rd_rel->relreplident;
-    HeapTuple    key_tuple = NULL;
+    Bitmapset  *idattrs;
+    HeapTuple    key_tuple;
     bool        nulls[MaxHeapAttributeNumber];
     Datum        values[MaxHeapAttributeNumber];
-    int            natt;

     *copy = false;

@@ -7624,7 +7626,7 @@ ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed, bool *
         if (HeapTupleHasExternal(tp))
         {
             *copy = true;
-            tp = toast_flatten_tuple(tp, RelationGetDescr(relation));
+            tp = toast_flatten_tuple(tp, desc);
         }
         return tp;
     }
@@ -7633,41 +7635,37 @@ ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed, bool *
     if (!key_changed)
         return NULL;

-    /* find the replica identity index */
-    replidindex = RelationGetReplicaIndex(relation);
-    if (!OidIsValid(replidindex))
+    /* find out the replica identity columns */
+    idattrs = RelationGetIndexAttrBitmap(relation,
+                                         INDEX_ATTR_BITMAP_IDENTITY_KEY);
+
+    if (bms_is_empty(idattrs))
     {
         elog(DEBUG4, "could not find configured replica identity for table \"%s\"",
              RelationGetRelationName(relation));
         return NULL;
     }

-    idx_rel = RelationIdGetRelation(replidindex);
-
-    Assert(CheckRelationLockedByMe(idx_rel, AccessShareLock, true));
-
-    /* deform tuple, so we have fast access to columns */
-    heap_deform_tuple(tp, desc, values, nulls);
-
-    /* set all columns to NULL, regardless of whether they actually are */
-    memset(nulls, 1, sizeof(nulls));
-
     /*
-     * Now set all columns contained in the index to NOT NULL, they cannot
-     * currently be NULL.
+     * Construct a new tuple containing only the replica identity columns,
+     * with nulls elsewhere.  While we're at it, assert that the replica
+     * identity columns aren't null.
      */
-    for (natt = 0; natt < IndexRelationGetNumberOfKeyAttributes(idx_rel); natt++)
-    {
-        int            attno = idx_rel->rd_index->indkey.values[natt];
+    heap_deform_tuple(tp, desc, values, nulls);

-        if (attno < 0)
-            elog(ERROR, "system column in index");
-        nulls[attno - 1] = false;
+    for (int i = 0; i < desc->natts; i++)
+    {
+        if (bms_is_member(i + 1 - FirstLowInvalidHeapAttributeNumber,
+                          idattrs))
+            Assert(!nulls[i]);
+        else
+            nulls[i] = true;
     }

     key_tuple = heap_form_tuple(desc, values, nulls);
     *copy = true;
-    RelationClose(idx_rel);
+
+    bms_free(idattrs);

     /*
      * If the tuple, which by here only contains indexed columns, still has
@@ -7680,7 +7678,7 @@ ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed, bool *
     {
         HeapTuple    oldtup = key_tuple;

-        key_tuple = toast_flatten_tuple(oldtup, RelationGetDescr(relation));
+        key_tuple = toast_flatten_tuple(oldtup, desc);
         heap_freetuple(oldtup);
     }

On Mon, Sep 2, 2019 at 6:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > As far as 4) goes, I think the code in ExtractReplicaIdentity is pretty
> > duff anyway, because it doesn't bother to check for the defined failure
> > return for RelationIdGetRelation.  But if we're concerned about the
> > cost of recalculating this stuff per-row, couldn't we cache it a little
> > better?  It should be safe to assume the set of index columns isn't
> > changing intra-query.
> > ... in fact, isn't all the infrastructure for that present already?
> > Why is this code looking directly at the index at all, rather than
> > using the relcache's rd_idattr bitmap?
>
> Here's a proposed patch along those lines.  It fixes Hadi's original
> crash case and passes check-world.

Agree that this patch would be a better solution for Hadi's report,
although I also agree that the situation with index locking for DELETE
isn't perfect.

> I'm a bit suspicious of the exclusion for idattrs being empty, but
> if I remove that, some of the contrib/test_decoding test results
> change.  Anybody want to comment on that?  If that's actually an
> expected situation, why is there an elog(DEBUG) in that path?

ISTM that the exclusion case may occur with the table's replica
identity being REPLICA_IDENTITY_DEFAULT and there being no primary
index defined, in which case nothing needs to get logged.

The elog(DEBUG) may just be a remnant from the days when this was
being developed.  I couldn't find any notes on it though in the
archives [1] though.

Thanks,
Amit

[1] https://www.postgresql.org/message-id/flat/20131204155510.GO24801%40awork2.anarazel.de

Amit Langote <amitlangote09@gmail.com> writes:
> On Mon, Sep 2, 2019 at 6:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Here's a proposed patch along those lines.  It fixes Hadi's original
>> crash case and passes check-world.

> Agree that this patch would be a better solution for Hadi's report,
> although I also agree that the situation with index locking for DELETE
> isn't perfect.

Thanks for checking!

>> I'm a bit suspicious of the exclusion for idattrs being empty, but
>> if I remove that, some of the contrib/test_decoding test results
>> change.  Anybody want to comment on that?  If that's actually an
>> expected situation, why is there an elog(DEBUG) in that path?

> ISTM that the exclusion case may occur with the table's replica
> identity being REPLICA_IDENTITY_DEFAULT and there being no primary
> index defined, in which case nothing needs to get logged.

Looking more closely, the case is unreachable in the heap_update
path because key_changed will necessarily be false if the idattrs
set is empty.  But it is reachable in heap_delete because that
just passes key_changed = constant true, whether or not there's
any defined replica identity.  In view of that, I think
we should just remove the elog(DEBUG) ... and maybe add a comment
explaining this.

            regards, tom lane

Hi,

On 2019-09-01 16:50:00 -0400, Tom Lane wrote:
> As far as 4) goes, I think the code in ExtractReplicaIdentity is pretty
> duff anyway, because it doesn't bother to check for the defined failure
> return for RelationIdGetRelation.  But if we're concerned about the
> cost of recalculating this stuff per-row, couldn't we cache it a little
> better?  It should be safe to assume the set of index columns isn't
> changing intra-query.

I agree that it ought to be more efficent - but also about as equally
safe? I.e. if the previous code wasn't safe, the new code wouldn't be
safe either? As in, we're "just" avoiding the assert, but not increasing
safety?


> ... in fact, isn't all the infrastructure for that present already?
> Why is this code looking directly at the index at all, rather than
> using the relcache's rd_idattr bitmap?

No idea, that's too long ago :(


> I'm a bit suspicious of the exclusion for idattrs being empty, but
> if I remove that, some of the contrib/test_decoding test results
> change.  Anybody want to comment on that?  If that's actually an
> expected situation, why is there an elog(DEBUG) in that path?

I think Amit's explanation here is probably accurate.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> I agree that it ought to be more efficent - but also about as equally
> safe? I.e. if the previous code wasn't safe, the new code wouldn't be
> safe either? As in, we're "just" avoiding the assert, but not increasing
> safety?

Well, the point is that the old code risks performing a relcache load
without holding any lock on the relation.  In practice, given that
we do hold a lock on the parent table, it's probably safe ... but
it's at best bad practice.  It's not too hard to imagine future
optimizations that would allow this to result in a corrupt relcache entry.

I don't believe that there's any equivalent risk in the modified code.

            regards, tom lane