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>
 <CAMAof68L0GO0F0bwuXtLZAjh9k_Hj+o0-8mqfO6iEQyXr4PuVA@mail.gmail.com>
In-Reply-To: 
 <CAMAof68L0GO0F0bwuXtLZAjh9k_Hj+o0-8mqfO6iEQyXr4PuVA@mail.gmail.com>
From: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Date: Wed, 18 Jun 2025 23:15:28 +0200
Message-ID: 
 <CADzfLwUrodAcOggK+3j3LbPLaSXemgHxa-n=LhZTwRAsaakL2g@mail.gmail.com>
Subject: Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements
To: Sergey Sargsyan <sergey.sargsyan.2001@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: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CADzfLwUrodAcOggK%2B3j3LbPLaSXemgHxa-n%3DLhZTwRAsaakL2g%40mail.gmail.com>
Precedence: bulk

Hello, Sergey!

> Today I encountered a segmentation fault caused by the patch v20-0007-Add=
-Datum-storage-support-to-tuplestore.patch. During the merge phase, I inser=
ted some tuples into the table so that STIR would have data for the validat=
ion phase. The segfault occurred during a call to tuplestore_end().
>
> The root cause is that a few functions in the tuplestore code still assum=
e that all stored data is a pointer and thus attempt to pfree it. This assu=
mption breaks when datumByVal is used, as the data is stored directly and n=
ot as a pointer. In particular, tuplestore_end(), tuplestore_trim(), and tu=
plestore_clear() incorrectly try to free such values.
>
> When addressing this, please also ensure that context memory accounting i=
s handled properly: we should not increment or decrement the remaining cont=
ext memory size when cleaning or trimming datumByVal entries, since no actu=
al memory was allocated for them.
>
> Interestingly, I=E2=80=99m surprised you haven=E2=80=99t hit this segfaul=
t yourself. Are you perhaps testing on an older system where INT8OID is pas=
sed by reference? Or is your STIR always empty during the validation phase?

Thanks for pointing that out. It looks like tuplestore_trim and
tuplestore_clear are broken, while tuplestore_end seems to be correct
but fails due to previous heap corruption.
In my case, tuplestore_trim and tuplestore_clear aren't called at all
- that's why the issue wasn't detected. I'm not sure why; perhaps some
recent changes in your codebase are affecting that?

Please run a stress test (if you've already applied the in-place fix
for the tuplestore):
         ninja && meson test --suite setup && meson test
--print-errorlogs --suite pg_amcheck *006*

This will help ensure everything else is working correctly on your system.

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

Yes, that=E2=80=99s probably a good idea. I was trying to keep it simple fr=
om
the perspective of parameters to avoid dealing with some of the tricky
internal logic.
But you're right - it=E2=80=99s better to stick with the old signature.

Best regards,
Mikhail.