On 11/27/23 11:34, Tomas Vondra wrote: > > > On 11/27/23 08:37, Richard Guo wrote: >> >> On Mon, Nov 27, 2023 at 1:53 PM Soumyadeep Chakraborty >> <soumyadeep2...@gmail.com <mailto:soumyadeep2...@gmail.com>> wrote: >> >> On Sun, Nov 26, 2023 at 9:28 PM Richard Guo <guofengli...@gmail.com >> <mailto:guofengli...@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. >
I did it this way (check in index_insert_cleanup) and pushed. Thanks for the report! regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
