Re: Match table_complete_speculative() code to comment - Mailing list pgsql-hackers

From Andres Freund
Subject Re: Match table_complete_speculative() code to comment
Date
Msg-id 20190514192242.65b7frvb24p2xlun@alap3.anarazel.de
Whole thread Raw
In response to Match table_complete_speculative() code to comment  (Ashwin Agrawal <aagrawal@pivotal.io>)
List pgsql-hackers
Hi,

On 2019-04-30 11:53:38 -0700, Ashwin Agrawal wrote:
> Comment for table_complete_speculative() says
> 
> /*
>  * Complete "speculative insertion" started in the same transaction.
> If
>  * succeeded is true, the tuple is fully inserted, if false, it's
> removed.
>  */
> static inline void
> table_complete_speculative(Relation rel, TupleTableSlot *slot,
>                            uint32 specToken, bool succeeded)
> {
>     rel->rd_tableam->tuple_complete_speculative(rel, slot, specToken,
>                                                 succeeded);
> }
> 
> but code really refers to succeeded as failure. Since that argument is
> passed as specConflict, means conflict happened and hence the tuple
> should be removed. It would be better to fix the code to match the
> comment as in AM layer its better to deal with succeeded to finish the
> insertion and not other way round.
> 
> diff --git a/src/backend/access/heap/heapam_handler.c
> b/src/backend/access/heap/heapam_handler.c
> index 4d179881f27..241639cfc20 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 444c0c05746..d545bbce8a2 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);

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



pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: Adding a test for speculative insert abort case
Next
From: Andres Freund
Date:
Subject: Re: Table AM callback table_complete_speculative()'s succeededargument is reversed