Table AM callback table_complete_speculative()'s succeeded argument is reversed
The 'succeeded' argument seems backwards here:
static void
heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot,
uint32 spekToken, bool succeeded)
{
bool shouldFree = true;
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);/* adjust the tuple's state accordingly */
if (!succeeded)
heap_finish_speculative(relation, &slot->tts_tid);
else
heap_abort_speculative(relation, &slot->tts_tid);if (shouldFree)
pfree(tuple);
}
According to the comments, if "succeeded = true", the insertion is
completed, and otherwise it's killed. It works, because the only caller
is also passing the argument wrong.
Barring objections, I'll push the attached patch to fix that.
- Heikki
Attachments:
reverse-speculative-succeeded-argument.patchtext/x-patch; name=reverse-speculative-succeeded-argument.patchDownload
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index bc47856ad5..00505ec3f4 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -282,7 +282,7 @@ heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot,
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
/* adjust the tuple's state accordingly */
- if (!succeeded)
+ if (succeeded)
heap_finish_speculative(relation, &slot->tts_tid);
else
heap_abort_speculative(relation, &slot->tts_tid);
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 444c0c0574..d545bbce8a 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -556,7 +556,7 @@ ExecInsert(ModifyTableState *mtstate,
/* adjust the tuple's state accordingly */
table_complete_speculative(resultRelationDesc, slot,
- specToken, specConflict);
+ specToken, !specConflict);
/*
* Wake up anyone waiting for our decision. They will re-check
Hi,
On May 14, 2019 4:29:01 AM PDT, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
The 'succeeded' argument seems backwards here:
static void
heapam_tuple_complete_speculative(Relation relation, TupleTableSlot*slot,
uint32 spekToken, bool succeeded)
{
bool shouldFree = true;
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);/* adjust the tuple's state accordingly */
if (!succeeded)
heap_finish_speculative(relation, &slot->tts_tid);
else
heap_abort_speculative(relation, &slot->tts_tid);if (shouldFree)
pfree(tuple);
}According to the comments, if "succeeded = true", the insertion is
completed, and otherwise it's killed. It works, because the only calleris also passing the argument wrong.
Thanks for finding.
Barring objections, I'll push the attached patch to fix that.
Please hold off - your colleagues found this before, and I worked on getting test coverage for the code. It's scheduled for commit together today. Unfortunately nobody looked at the test much...
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi,
On 2019-05-14 07:06:34 -0700, Andres Freund wrote:
On May 14, 2019 4:29:01 AM PDT, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
The 'succeeded' argument seems backwards here:
static void
heapam_tuple_complete_speculative(Relation relation, TupleTableSlot*slot,
uint32 spekToken, bool succeeded)
{
bool shouldFree = true;
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);/* adjust the tuple's state accordingly */
if (!succeeded)
heap_finish_speculative(relation, &slot->tts_tid);
else
heap_abort_speculative(relation, &slot->tts_tid);if (shouldFree)
pfree(tuple);
}According to the comments, if "succeeded = true", the insertion is
completed, and otherwise it's killed. It works, because the only calleris also passing the argument wrong.
Thanks for finding.
Barring objections, I'll push the attached patch to fix that.
Please hold off - your colleagues found this before, and I worked on getting test coverage for the code. It's scheduled for commit together today. Unfortunately nobody looked at the test much...
\
And pushed, as https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=aa4b8c61d2cd57b53be03defb04d59b232a0e150
with the part that wasn't covered by tests now covered by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=08e2edc0767ab6e619970f165cb34d4673105f23
Greetings,
Andres Freund