MIME-Version: 1.0
References: <20230704112358.bkm6r3fxp275m4lu@alvherre.pgsql>
 <2b672382-c0b3-3f44-b72c-86b1077b082b@enterprisedb.com> <CAE-ML+_ecaX4jRQZaRdzdTk4zGPY5qUDB6Y3eV=KQu--k49yeQ@mail.gmail.com>
 <11f78627-a934-1f8b-084e-21ed95f2564c@enterprisedb.com> <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>
In-Reply-To: <CAE-ML+8Nx1X7PL-HHeGq=BbzsXotpzZQKe7QQOtTmiGRS6CmQg@mail.gmail.com>
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 27 Nov 2023 15:37:20 +0800
Message-ID: <CAMbWs4_LLOoWWtnL1Va1qaHHiuvhH2kZY3EDWA7QqycEdRbFzw@mail.gmail.com>
Subject: Re: brininsert optimization opportunity
To: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>, 
	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>
Content-Type: multipart/alternative; boundary="000000000000efa72d060b1d5f79"
Archived-At: <https://www.postgresql.org/message-id/CAMbWs4_LLOoWWtnL1Va1qaHHiuvhH2kZY3EDWA7QqycEdRbFzw%40mail.gmail.com>
Precedence: bulk

--000000000000efa72d060b1d5f79
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Nov 27, 2023 at 1:53=E2=80=AFPM Soumyadeep Chakraborty <
soumyadeep2007@gmail.com> wrote:

> On Sun, Nov 26, 2023 at 9:28=E2=80=AFPM Richard Guo <guofenglinux@gmail.c=
om>
> wrote:
> > It seems that we have an oversight in this commit.  If there is no tupl=
e
> > 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=3D# update brin_summarize set value =3D 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=3D# update brin_summarize set value =3D 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

--000000000000efa72d060b1d5f79
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><div class=3D"gmail_quote"><div=
 dir=3D"ltr" class=3D"gmail_attr">On Mon, Nov 27, 2023 at 1:53=E2=80=AFPM S=
oumyadeep Chakraborty &lt;<a href=3D"mailto:soumyadeep2007@gmail.com">soumy=
adeep2007@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quot=
e" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204)=
;padding-left:1ex">On Sun, Nov 26, 2023 at 9:28=E2=80=AFPM Richard Guo &lt;=
<a href=3D"mailto:guofenglinux@gmail.com" target=3D"_blank">guofenglinux@gm=
ail.com</a>&gt; wrote:<br>
&gt; It seems that we have an oversight in this commit.=C2=A0 If there is n=
o tuple<br>
&gt; that has been inserted, we wouldn&#39;t have an available insert state=
 in<br>
&gt; the clean up phase.=C2=A0 So the Assert in brininsertcleanup() is not =
always<br>
&gt; right.=C2=A0 For example:<br>
&gt;<br>
&gt; regression=3D# update brin_summarize set value =3D brin_summarize.valu=
e;<br>
&gt; server closed the connection unexpectedly<br>
<br>
I wasn&#39;t able to repro the issue on 86b64bafc19c4c60136a4038d2a8d1e6eec=
c59f2.<br>
with UPDATE/INSERT:<br>
<br>
This could be because since c5b7ba4e67aeb5d6f824b74f94114d99ed6e42b7,<br>
we have moved ExecOpenIndices()<br>
from ExecInitModifyTable() to ExecInsert(). Since we never open the<br>
indices if nothing is<br>
inserted, we would never attempt to close them with ExecCloseIndices()<br>
while the ii_AmCache<br>
is NULL (which is what causes this assertion failure).</blockquote><div><br=
></div><div>AFAICS we would also open the indices from ExecUpdate().=C2=A0 =
So if we<br>update the table in a way that no new tuples are inserted, we w=
ill have<br>this issue.=C2=A0 As I showed previously, the query below crash=
es for me on<br>latest master (dc9f8a7983).<br><br>regression=3D# update br=
in_summarize set value =3D brin_summarize.value;<br>server closed the conne=
ction unexpectedly<br><br>There are other code paths that call ExecOpenIndi=
ces(), such as<br>ExecMerge().=C2=A0 I believe it&#39;s not hard to create =
queries that trigger this<br>Assert for those cases.<br><br>Thanks<br>Richa=
rd<br></div></div></div>

--000000000000efa72d060b1d5f79--