Обсуждение: [PATCH] Improve error message when trying to lock virtual tuple.
Hello, When currently trying to lock a virtual tuple the returned error will be a misleading `could not read block 0`. This patch adds a check for the tuple table slot being virtual to produce a clearer error. This can be triggered by extensions returning virtual tuples. While this is of course an error in those extensions the resulting error is very misleading. -- Regards, Sven Klemm
Вложения
Hi, > When currently trying to lock a virtual tuple the returned error > will be a misleading `could not read block 0`. This patch adds a > check for the tuple table slot being virtual to produce a clearer > error. > > This can be triggered by extensions returning virtual tuples. > While this is of course an error in those extensions the resulting > error is very misleading. ``` + /* + * If the slot is virtual, we can't lock it. This should never happen, but + * this will lead to a misleading could not read block error later otherwise. + */ ``` I suggest dropping or rephrasing the "this should never happen" part. If this never happened we didn't need this check. Maybe "If the slot is virtual, we can't lock it. Fail early in order to provide an appropriate error message", or just "If the slot is virtual, we can't lock it". ``` elog(ERROR, "cannot lock virtual tuple"); ``` For some reason I thought that ereport() is the preferred way of throwing errors, but I see elog() used many times in ExecLockRows() so this is probably fine. -- Best regards, Aleksander Alekseev
(now send a copy to -hackers, too) On Mon, 17 Jun 2024 at 17:55, Sven Klemm <sven@timescale.com> wrote: > > Hello, > > When currently trying to lock a virtual tuple the returned error > will be a misleading `could not read block 0`. This patch adds a > check for the tuple table slot being virtual to produce a clearer > error. > > This can be triggered by extensions returning virtual tuples. > While this is of course an error in those extensions the resulting > error is very misleading. I think you're solving the wrong problem here, as I can't think of a place where both virtual tuple slots and tuple locking are allowed at the same time in core code. I mean, in which kind of situation could we get a Relation's table slot which is not lockable by said relation's AM? Assuming the "could not read block 0" error comes from the heap code, why does the assertion in heapam_tuple_lock that checks for a BufferHeapTupleTableSlot not fire before this `block 0` error? If the error is not in the heapam code, could you show an example of the code that breaks with that error code? Kind regards, Matthias van de Meent Neon (https://neon.tech)
On Mon, Jun 17, 2024 at 10:25 PM Matthias van de Meent <boekewurm+postgres@gmail.com> wrote: > I think you're solving the wrong problem here, as I can't think of a > place where both virtual tuple slots and tuple locking are allowed at > the same time in core code. > > I mean, in which kind of situation could we get a Relation's table > slot which is not lockable by said relation's AM? Assuming the "could > not read block 0" error comes from the heap code, why does the > assertion in heapam_tuple_lock that checks for a > BufferHeapTupleTableSlot not fire before this `block 0` error? If the > error is not in the heapam code, could you show an example of the code > that breaks with that error code? In assertion enabled builds this will be stopped much earlier and not return the misleading error message. But most packaged postgres versions don't have assertions enabled and will produce the misleading `could not read block 0` error. I am aware that this not a postgres bug, but i think this error message is an improvement over the current situation. -- Regards, Sven Klemm
On Tue, 18 Jun 2024 at 09:32, Sven Klemm <sven@timescale.com> wrote: > > On Mon, Jun 17, 2024 at 10:25 PM Matthias van de Meent > <boekewurm+postgres@gmail.com> wrote: > > > I think you're solving the wrong problem here, as I can't think of a > > place where both virtual tuple slots and tuple locking are allowed at > > the same time in core code. > > > > I mean, in which kind of situation could we get a Relation's table > > slot which is not lockable by said relation's AM? Assuming the "could > > not read block 0" error comes from the heap code, why does the > > assertion in heapam_tuple_lock that checks for a > > BufferHeapTupleTableSlot not fire before this `block 0` error? If the > > error is not in the heapam code, could you show an example of the code > > that breaks with that error code? > > In assertion enabled builds this will be stopped much earlier and not return > the misleading error message. But most packaged postgres versions don't have > assertions enabled and will produce the misleading `could not read block 0` > error. > I am aware that this not a postgres bug, but i think this error message > is an improvement over the current situation. Extensions shouldn't cause assertions to trigger, IMO, and I don't think that this check in ExecLockRows is a good way to solve that issue. In my opinion, authors should test their extension on assert-enabled PostgreSQL, so that they're certain they're not doing If you're dead-set on having users see less confusing error messages when assertions should have triggered (but are not enabled, and thus don't), I think the better place to add additional checks & error messages is in the actual heapam_tuple_lock method, just after the assertion, rather than in the AM-agnostic ExecLockRows: If or when a tableAM decides that VirtualTableTupleSlot is the slot type they want to use for passing tuples around, then that shouldn't be broken by code in ExecLockRows that was put there to mimick an assert in the heap AM. Kind regards, Matthias van de Meent Neon (https://neon.tech)