Re: brininsert optimization opportunity

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
>> <soumyadeep2007(at)gmail(dot)com <mailto:soumyadeep2007(at)gmail(dot)com>> wrote:
>>
>> On Sun, Nov 26, 2023 at 9:28 PM Richard Guo <guofenglinux(at)gmail(dot)com
>> <mailto: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.
>>
>
> 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