Re: Match table_complete_speculative() code to comment

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