Re: brininsert optimization opportunity

On 11/27/23 08:37, Richard Guo wrote:
> 
> On Mon, Nov 27, 2023 at 1:53 PM Soumyadeep Chakraborty
> <soumyadeep2007@gmail.com <mailto:soumyadeep2007@gmail.com>> wrote:
> 
>     On Sun, Nov 26, 2023 at 9:28 PM Richard Guo <guofenglinux@gmail.com
>     <mailto:guofenglinux@gmail.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.
> 

FWIW I can readily reproduce it like this:

  drop table t;
  create table t (a int);
  insert into t values (1);
  create index on t using brin (a);
  update t set a = a;

I however wonder if maybe we should do the check in index_insert_cleanup
and not in the AM callback. That seems simpler / better, because the AM
callbacks then can't make this mistake.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company