Table AM callback table_complete_speculative()'s succeeded argument is reversed

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 caller

is 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 caller

is 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