Обсуждение: Match table_complete_speculative() code to comment
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); /* * Wake up anyone waiting for our decision. They will re-check - Ashwin and Melanie
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