On Thu, Feb 20, 2025 at 6:14 AM Dmitry Koval <d.ko...@postgrespro.ru> wrote:
> I got an Assert when executing an "INSERT ... ON CONFLICT ... UPDATE
> ..." query on partitioned table. Managed to reproduce this situation.
I was able to reproduce the assert with your instructions.
> I suggest replace Assert with an error message, see
> [v1-0001-draft-of-fix.patch]. This is not a final fix as I am confused
> by the comment for Assert: "we don't support an UPDATE of INSERT ON
> CONFLICT for a partitioned table".
> (Why "don't support an UPDATE"?
> It's not forbidden by syntax or errors ...)
The assert was introduced commit f16241 [0]. Specifically it was added as a
result of this discussion [1]
> On Wed, Nov 29, 2017 at 7:51 AM, Amit Kapila
<amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Tue, Nov 28, 2017 at 5:58 PM, amul sul <sulamul(at)gmail(dot)com>
wrote:
>>> On Sat, Nov 25, 2017 at 11:39 AM, Amit Kapila
<amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>> On Thu, Nov 23, 2017 at 5:18 PM, amul sul <sulamul(at)gmail(dot)com>
wrote:
>>>>> On Sat, Nov 11, 2017 at 1:05 AM, Robert Haas
<robertmhaas(at)gmail(dot)com> wrote:
>>>>>> On Wed, Sep 27, 2017 at 7:07 AM, amul sul <sulamul(at)gmail(dot)com>
wrote:
>>>>>>>
>>>> 1.
>>>> @@ -1480,6 +1493,10 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
>>>> ereport(ERROR,
>>>> (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
>>>> errmsg("could not serialize access due to concurrent update")));
>>>> + if
(!BlockNumberIsValid(BlockIdGetBlockNumber(&((hufd.ctid).ip_blkid))))
>>>> + ereport(ERROR,
>>>> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>>>> + errmsg("tuple to be updated was already moved to an another
>>>> partition due to concurrent update")));
>>>>
>>>> Why do you think we need this check in the OnConflictUpdate path? I
>>>> think we don't it here because we are going to relinquish this version
>>>> of the tuple and will start again and might fetch some other row
>>>> version. Also, we don't support Insert .. On Conflict Update with
>>>> partitioned tables, see[1], which is also an indication that at the
>>>> very least we don't need it now.
>>>>
>>> Agreed, even though this case will never going to be anytime soon
>>> shouldn't we have a check for invalid block id? IMHO, we should have
>>> this check and error report or assert, thoughts?
>>>
>>
>> I feel adding code which can't be hit (even if it is error handling)
>> is not a good idea. I think having an Assert should be okay, but
>> please write comments to explain the reason for adding an Assert.
>>
>
> Agree, updated in the attached patch.
So if I understand correctly, at the time the assert was added, we in
fact did not support UPDATE of INSERT ON CONFLICT for a partitioned
table. However, since then we've added support but nobody removed or
altered the assert.
I'm not very familiar with this code, but from that discussion it
sounds like your solution of converting the assert to an error is
correct.
I don't fully understand why an error is needed though. This specific
case will return false which will signal the caller to retry the
insert. Just as an experiment I tried deleting the assert (attached
patch) and running your test. Everything behaved as expected and
nothing blew up. It also seems to pass a CI run just fine [2]. It also
passes `make check && make check-world` locally.
Hopefully someone from the original thread can shed some light on
whether an error is needed or not.
[0]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=f16241bef7cc271bff60e23de2f827a10e50dde8
[1]
https://www.postgresql.org/message-id/flat/CAAJ_b95PkwojoYfz0bzXU8OokcTVGzN6vYGCNVUukeUDrnF3dw%40mail.gmail.com
[2] https://github.com/jkosh44/postgres/runs/38431219796
Thanks,
Joseph Koshakow
From 3959bc8d53b29d3b7ba79d9cf923f9da65d43d5b Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <kosh...@gmail.com>
Date: Sat, 8 Mar 2025 15:39:47 -0500
Subject: [PATCH v1] Remove assert for update on conflict
This commit removes an assert that is no longer valid. The assert was
making an assumption that INSERT .. ON CONFLIT UPDATE .. was not
allowed on partitioned tables. This assumption is no longer valid and
the assert is possible to be triggered.
---
src/backend/executor/nodeModifyTable.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index b0fe50075a..f33a273cfa 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2786,14 +2786,6 @@ ExecOnConflictUpdate(ModifyTableContext *context,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
- /*
- * As long as we don't support an UPDATE of INSERT ON CONFLICT for
- * a partitioned table we shouldn't reach to a case where tuple to
- * be lock is moved to another partition due to concurrent update
- * of the partition key.
- */
- Assert(!ItemPointerIndicatesMovedPartitions(&tmfd.ctid));
-
/*
* Tell caller to try again from the very start.
*
--
2.43.0
