Message-ID: <0a45e27b-d55d-f46b-5954-2824c9834d2a@enterprisedb.com>
Date: Mon, 27 Nov 2023 16:54:20 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: brininsert optimization opportunity
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Richard Guo <guofenglinux@gmail.com>,
 Soumyadeep Chakraborty <soumyadeep2007@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
 Ashwin Agrawal <ashwinstar@gmail.com>
References: <20230704112358.bkm6r3fxp275m4lu@alvherre.pgsql>
 <CAE-ML+-9K2h2R7zcvQGq8FarxvSVhgAGZkgH57iQCnWQoxOFww@mail.gmail.com>
 <35e7be37-0d35-d8ea-a4d5-eff84aefcc39@enterprisedb.com>
 <CAE-ML+9JVD3vt7JZtmNy8hOFQKtP2jxJtf27=kE=ViGyv36V3w@mail.gmail.com>
 <CAE-ML+_jerNg64mSD5VYy=bOxcRQSr-QfESHvCVFPEwMM_jufQ@mail.gmail.com>
 <CAE-ML+-nBfDXQEfh0Ex0bp=tZdzzs7y5bvmQJg1WC7KpJSZVmQ@mail.gmail.com>
 <CAE-ML+_9Adp8rRfZ9FP4ODtQZLZcMaBvae5ACO9H+-FZdk_v9w@mail.gmail.com>
 <1d68ef1d-41c5-7159-c6c0-b44dad4e3148@enterprisedb.com>
 <CAEze2WhR0zkCTYrs1_-xJErcZkyKLnV-9=cKq43D6EURj6OSgQ@mail.gmail.com>
 <CAE-ML+_1nGVH7CitWieLZNR3W54V+RUGG8RQEon5EdEHucGEFw@mail.gmail.com>
 <12d42882-7860-9b46-7572-3b2fae6c916f@enterprisedb.com>
 <CAMbWs4-w9qC-o9hQox9UHvdVZAYTp8OrPQOKtwbvzWaRejTT=Q@mail.gmail.com>
 <CAE-ML+8Nx1X7PL-HHeGq=BbzsXotpzZQKe7QQOtTmiGRS6CmQg@mail.gmail.com>
 <CAMbWs4_LLOoWWtnL1Va1qaHHiuvhH2kZY3EDWA7QqycEdRbFzw@mail.gmail.com>
 <4b436ddb-39a5-fad5-5486-f8f971dcaaf6@enterprisedb.com>
In-Reply-To: <4b436ddb-39a5-fad5-5486-f8f971dcaaf6@enterprisedb.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/0a45e27b-d55d-f46b-5954-2824c9834d2a%40enterprisedb.com>
Precedence: bulk

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@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.
> 

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