public inbox for [email protected]
help / color / mirror / Atom feedFrom: Sergey Sargsyan <[email protected]>
To: Mihail Nikalayeu <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Andrey Borodin <[email protected]>
Cc: Melanie Plageman <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Subject: Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements
Date: Thu, 19 Jun 2025 00:36:43 +0300
Message-ID: <CAMAof691D4O=3QTuPwJXBYxYpG6s3A=tVhL9vN=T3eeRTMnaig@mail.gmail.com> (raw)
In-Reply-To: <CADzfLwUrodAcOggK+3j3LbPLaSXemgHxa-n=LhZTwRAsaakL2g@mail.gmail.com>
References: <CADzfLwW9QczZW-E=McxcjUv0e5VMDctQNETbgao0K-SimVhFPA@mail.gmail.com>
<[email protected]>
<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>
<CADzfLwUrodAcOggK+3j3LbPLaSXemgHxa-n=LhZTwRAsaakL2g@mail.gmail.com>
My bad, my fork's based on pg15, and over there tuplestore_end() does this,
void
tuplestore_end(Tuplestorestate *state)
{
int i;
if (state->myfile)
BufFileClose(state->myfile);
if (state->memtuples)
{
for (i = state->memtupdeleted; i < state->memtupcount; i++)
pfree(state->memtuples[i]);
pfree(state->memtuples);
}
pfree(state->readptrs);
pfree(state);
}
It lets each tuple go one by one, but in pg18, it just nukes the whole
memory context instead.
Therefore, over pg18 patch presents no issues; however, incorporating
`_clean` and `_trim` functions for datum cases is recommended to prevent
future developers from encountering segmentation faults when utilizing the
interface. This minor adjustment should ensure expected functionality.
Best regards,
S
On Thu, Jun 19, 2025, 12:16 AM Mihail Nikalayeu <[email protected]>
wrote:
> 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 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’m surprised you haven’t 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?
>
> 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
> signature. You added the relpersistence parameter, which seems unnecessary—
> > this can be determined internally by checking if it’s 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.
>
> Yes, that’s probably a good idea. I was trying to keep it simple from
> the perspective of parameters to avoid dealing with some of the tricky
> internal logic.
> But you're right - it’s better to stick with the old signature.
>
> Best regards,
> Mikhail.
>
view thread (64+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements
In-Reply-To: <CAMAof691D4O=3QTuPwJXBYxYpG6s3A=tVhL9vN=T3eeRTMnaig@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox