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

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Heikki Linnakangas
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

reverse-speculative-succeeded-argument.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,


On May 14, 2019 4:29:01 AM PDT, Heikki Linnakangas <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

On 2019-05-14 07:06:34 -0700, Andres Freund wrote:

> On May 14, 2019 4:29:01 AM PDT, Heikki Linnakangas <[hidden email]> 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