Re: Concurrency bug in UPDATE of partition-key
От | Amit Kapila |
---|---|
Тема | Re: Concurrency bug in UPDATE of partition-key |
Дата | |
Msg-id | CAA4eK1LkSscAckj8mh8_ogME98CQ5f6c_sRUp6R5K60LENeKXA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Concurrency bug in UPDATE of partition-key (Amit Khandekar <amitdkhan.pg@gmail.com>) |
Ответы |
Re: Concurrency bug in UPDATE of partition-key
|
Список | pgsql-hackers |
On Tue, Jun 19, 2018 at 9:33 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: > > Could not add RAISE statement, because the isolation test does not > seem to print those statements in the output. Instead, I have dumped > some rows in a new table through the trigger function, and did a > select from that table. Attached is v3 patch. > Comments ----------------- 1. + /* + * If the tuple was concurrently updated and the caller of this + * function requested for the updated tuple, skip the trigger + * execution. + */ + if (newSlot != NULL && epqslot != NULL) + return false; There is a possibility of memory leak due to above change. Basically, GetTupleForTrigger returns a newly allocated tuple. We should free the triggertuple by calling heap_freetuple(trigtuple). 2. ExecBRDeleteTriggers(..) { .. + /* If requested, pass back the concurrently updated tuple, if any. */ + if (epqslot != NULL) + *epqslot = newSlot; + if (trigtuple == NULL) return false; + + /* + * If the tuple was concurrently updated and the caller of this + * function requested for the updated tuple, skip the trigger + * execution. + */ + if (newSlot != NULL && epqslot != NULL) + return false; .. } Can't we merge the first change into second, something like: if (newSlot != NULL && epqslot != NULL) { *epqslot = newSlot; return false; } 3. ExecBRDeleteTriggers(..) { - TupleTableSlot *newSlot; int i; Assert(HeapTupleIsValid(fdw_trigtuple) ^ ItemPointerIsValid(tupleid)); if (fdw_trigtuple == NULL) { + TupleTableSlot *newSlot; + .. } This change is okay on its own, but I think it has nothing to do with this patch. If you don't mind, can we remove it? 4. +/* ---------- + * ExecBRDeleteTriggers() + * + * Called to execute BEFORE ROW DELETE triggers. + * + * Returns false in following scenarios : + * 1. Trigger function returned NULL. + * 2. The tuple was concurrently deleted OR it was concurrently updated and the + * new tuple didn't pass EvalPlanQual() test. + * 3. The tuple was concurrently updated and the new tuple passed the + * EvalPlanQual() test, BUT epqslot parameter is not NULL. In such case, this + * function skips the trigger function execution, because the caller has + * indicated that it wants to further process the updated tuple. The updated + * tuple slot is passed back through epqslot. + * + * In all other cases, returns true. + * ---------- + */ bool ExecBRDeleteTriggers(EState *estate, EPQState *epqstate, The above comments appear unrelated to this function, example, this function is not at all aware of concurrent updates. I think if you want to add comments to this function, we can add them as a separate patch or if you want to add them as part of this patch at least make them succinct. 5. + /* + * If this is part of an UPDATE of partition-key, the + * epq tuple will contain the changes from this + * transaction over and above the updates done by the + * other transaction. The caller should now use this + * tuple as its NEW tuple, rather than the earlier NEW + * tuple. + */ I think we can simplify this comment as "If requested, pass back the updated tuple if any.", something similar to what you have at some other place in the patch. 6. +# Session A is moving a row into another partition, but is waiting for +# another session B that is updating the original row. The row that ends up +# in the new partition should contain the changes made by session B. You have named sessions as s1 and s2, but description uses A and B, it will be better to use s1 and s2 respectively. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Ashutosh BapatДата:
Сообщение: Re: Adding tests for inheritance trees with temporary tables
Следующее
От: Jeevan ChalkeДата:
Сообщение: Re: Server crashed with TRAP: FailedAssertion("!(parallel_workers >0)" when partitionwise_aggregate true.