MIME-Version: 1.0
References: 
 <CADzfLwW9QczZW-E=McxcjUv0e5VMDctQNETbgao0K-SimVhFPA@mail.gmail.com>
 <202505181556.3n6oiowvntyr@alvherre.pgsql>
 <CADzfLwXKtriMnfCNVGNH2ahwXaByjo-QOMWiDTU-9WZqh+zQ5g@mail.gmail.com>
 <CADzfLwW5bDWSxjHK7mqX8Lewki3+5FBydBC+nVcxg4xMGKscyw@mail.gmail.com>
 <CAMAof6-4xaV3QE2ErYJaJhu6qjFn99sWyo_HQeBhHikZM3GexA@mail.gmail.com>
 <CADzfLwXocKhpW3eFP1oScz+m+1XJ3bpi9QmVpoqC9RX9oyX=UA@mail.gmail.com>
 <CAMAof695VA+mbVRhWCTus=E0WnsMAQyqXxfOTohbcb7VUHSP4g@mail.gmail.com>
 <CAMAof69JSL8MYWG2qRScs3RQDpfcyZT_wFwW4SoAvftW+K_p1g@mail.gmail.com>
 <CADzfLwVMtwjHh8KY9kP=_vcYPqHs=JDzuexO4RFQ2fM8VoqovA@mail.gmail.com>
In-Reply-To: 
 <CADzfLwVMtwjHh8KY9kP=_vcYPqHs=JDzuexO4RFQ2fM8VoqovA@mail.gmail.com>
From: Sergey Sargsyan <sergey.sargsyan.2001@gmail.com>
Date: Wed, 18 Jun 2025 19:33:07 +0300
Message-ID: 
 <CAMAof68L0GO0F0bwuXtLZAjh9k_Hj+o0-8mqfO6iEQyXr4PuVA@mail.gmail.com>
Subject: Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Cc: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>,
	Andres Freund <andres@anarazel.de>, Michael Paquier <michael@paquier.xyz>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Andrey Borodin <amborodin86@gmail.com>,
	Melanie Plageman <melanieplageman@gmail.com>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>
Content-Type: multipart/alternative; boundary="000000000000ba133a0637db2f73"
Archived-At: 
 <https://www.postgresql.org/message-id/CAMAof68L0GO0F0bwuXtLZAjh9k_Hj%2Bo0-8mqfO6iEQyXr4PuVA%40mail.gmail.com>
Precedence: bulk

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

Hi,

Today I encountered a segmentation fault caused by the patch
v20-0007-Add-Datum-storage-support-to-tuplestore.patch. During the merge
phase, I inserted some tuples into the table so that STIR would have data
for the validation phase. The segfault occurred during a call to
tuplestore_end().

The root cause is that a few functions in the tuplestore code still assume
that all stored data is a pointer and thus attempt to pfree it. This
assumption breaks when datumByVal is used, as the data is stored directly
and not as a pointer. In particular, tuplestore_end(), tuplestore_trim(),
and tuplestore_clear() incorrectly try to free such values.

When addressing this, please also ensure that context memory accounting is
handled properly: we should not increment or decrement the remaining
context memory size when cleaning or trimming datumByVal entries, since no
actual memory was allocated for them.

Interestingly, I=E2=80=99m surprised you haven=E2=80=99t hit this segfault =
yourself. Are
you perhaps testing on an older system where INT8OID is passed by
reference? Or is your STIR always empty during the validation phase?

One more point: I noticed you modified the index_create() function
signature. You added the relpersistence parameter, which seems
unnecessary=E2=80=94this can be determined internally by checking if it=E2=
=80=99s an
auxiliary index, in which case the index should be marked as unlogged. You
also added an auxiliaryIndexOfOid argument (do not remember exact naming,
but was used for dependency). It might be cleaner to pass this via the
IndexInfo structure instead. index_create() already has dozens of mouthful
arguments, and external extensions (like pg_squeeze) still rely on the old
signature, so minimizing changes to the function interface would improve
compatibility.

Best regards,
Sergey

On Wed, Jun 18, 2025, 1:50=E2=80=AFPM Mihail Nikalayeu <mihailnikalayeu@gma=
il.com>
wrote:

> Hello, Sergey!
>
> > In patch
> v20-0006-Add-STIR-access-method-and-flags-related-to-auxi.patch, within t=
he
> "StirMarkAsSkipInserts" function, a critical section appears to be left
> unclosed. This resulted in an assertion failure during ANALYZE of a table
> containing a leftover STIR index.
> Thanks, good catch. I'll add it to batch fix with the other things.
>
> Best regards,
> Mikhail.
>

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

<div dir=3D"auto"><div dir=3D"auto">Hi,</div><div dir=3D"auto"><br></div>To=
day I encountered a segmentation fault caused by the patch v20-0007-Add-Dat=
um-storage-support-to-tuplestore.patch. During the merge phase, I inserted =
some tuples into the table so that STIR would have data for the validation =
phase. The segfault occurred during a call to tuplestore_end().<div dir=3D"=
auto"><br></div><div dir=3D"auto">The root cause is that a few functions in=
 the tuplestore code still assume that all stored data is a pointer and thu=
s attempt to pfree it. This assumption breaks when datumByVal is used, as t=
he data is stored directly and not as a pointer. In particular, tuplestore_=
end(), tuplestore_trim(), and tuplestore_clear() incorrectly try to free su=
ch values.</div><div dir=3D"auto"><br></div><div dir=3D"auto">When addressi=
ng this, please also ensure that context memory accounting is handled prope=
rly: we should not increment or decrement the remaining context memory size=
 when cleaning or trimming datumByVal entries, since no actual memory was a=
llocated for them.</div><div dir=3D"auto"><br></div><div dir=3D"auto">Inter=
estingly, I=E2=80=99m surprised you haven=E2=80=99t hit this segfault yours=
elf. Are you perhaps testing on an older system where INT8OID is passed by =
reference? Or is your STIR always empty during the validation phase?</div><=
div dir=3D"auto"><br></div><div dir=3D"auto">One more point: I noticed you =
modified the index_create() function signature. You added the relpersistenc=
e parameter, which seems unnecessary=E2=80=94this can be determined interna=
lly by checking if it=E2=80=99s an auxiliary index, in which case the index=
 should be marked as unlogged. You also added an auxiliaryIndexOfOid argume=
nt (do not remember exact naming, but was used for dependency). It might be=
 cleaner to pass this via the IndexInfo structure instead. index_create() a=
lready has dozens of mouthful arguments, and external extensions (like pg_s=
queeze) still rely on the old signature, so minimizing changes to the funct=
ion interface would improve compatibility.</div><div dir=3D"auto"><br></div=
><div dir=3D"auto">Best regards,=C2=A0</div><div dir=3D"auto">Sergey=C2=A0<=
/div></div><br><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_a=
ttr">On Wed, Jun 18, 2025, 1:50=E2=80=AFPM Mihail Nikalayeu &lt;<a href=3D"=
mailto:mihailnikalayeu@gmail.com" target=3D"_blank" rel=3D"noreferrer">miha=
ilnikalayeu@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_qu=
ote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex=
">Hello, Sergey!<br>
<br>
&gt; In patch v20-0006-Add-STIR-access-method-and-flags-related-to-auxi.pat=
ch, within the &quot;StirMarkAsSkipInserts&quot; function, a critical secti=
on appears to be left unclosed. This resulted in an assertion failure durin=
g ANALYZE of a table containing a leftover STIR index.<br>
Thanks, good catch. I&#39;ll add it to batch fix with the other things.<br>
<br>
Best regards,<br>
Mikhail.<br>
</blockquote></div>

--000000000000ba133a0637db2f73--