Re: brininsert optimization opportunity

On Mon, Nov 27, 2023 at 1:53 PM Soumyadeep Chakraborty <
soumyadeep2007(at)gmail(dot)com> wrote:

> On Sun, Nov 26, 2023 at 9:28 PM Richard Guo <guofenglinux(at)gmail(dot)com>
> wrote:
> > It seems that we have an oversight in this commit. If there is no tuple
> > that has been inserted, we wouldn't have an available insert state in
> > the clean up phase. So the Assert in brininsertcleanup() is not always
> > right. For example:
> >
> > regression=# update brin_summarize set value = brin_summarize.value;
> > server closed the connection unexpectedly
>
> I wasn't able to repro the issue on
> 86b64bafc19c4c60136a4038d2a8d1e6eecc59f2.
> with UPDATE/INSERT:
>
> This could be because since c5b7ba4e67aeb5d6f824b74f94114d99ed6e42b7,
> we have moved ExecOpenIndices()
> from ExecInitModifyTable() to ExecInsert(). Since we never open the
> indices if nothing is
> inserted, we would never attempt to close them with ExecCloseIndices()
> while the ii_AmCache
> is NULL (which is what causes this assertion failure).

AFAICS we would also open the indices from ExecUpdate(). So if we
update the table in a way that no new tuples are inserted, we will have
this issue. As I showed previously, the query below crashes for me on
latest master (dc9f8a7983).

regression=# update brin_summarize set value = brin_summarize.value;
server closed the connection unexpectedly

There are other code paths that call ExecOpenIndices(), such as
ExecMerge(). I believe it's not hard to create queries that trigger this
Assert for those cases.

Thanks
Richard