Re: Assert when executing query on partitioned table

On Thu, Feb 20, 2025 at 6:14 AM Dmitry Koval <d(dot)koval(at)postgrespro(dot)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