Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uS0Sq-0020qf-8w for pgsql-hackers@arkaria.postgresql.org; Wed, 18 Jun 2025 21:37:00 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1uS0So-005DxX-1O for pgsql-hackers@arkaria.postgresql.org; Wed, 18 Jun 2025 21:36:58 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uS0Sn-005DxP-JP for pgsql-hackers@lists.postgresql.org; Wed, 18 Jun 2025 21:36:58 +0000 Received: from mail-vs1-xe2d.google.com ([2607:f8b0:4864:20::e2d]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1uS0Sm-002lAd-0r for pgsql-hackers@postgresql.org; Wed, 18 Jun 2025 21:36:57 +0000 Received: by mail-vs1-xe2d.google.com with SMTP id ada2fe7eead31-4e2b5ffb932so58254137.0 for ; Wed, 18 Jun 2025 14:36:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1750282614; x=1750887414; darn=postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=aNDi082gHLYZGqaDJOminivR/r3TSe8LUCX7HkuKKMM=; b=b5EESLnbvz/BA30GXR0YDR/WCos9i0cmcIAVXA4pU32H0Jx/he+K2OOojvxzuEUjx0 wvipN4uWDbAJ7swr5uQbQf1qX4wYRM324C6Cd7nWvWV+ucFQLLbR3ykINAyfk+vg4Qlq Qr6UmV+oherGEzflZLz8xLTR/O0eUutFasgm+C52ADzcNNUVZvWTPek/QMDF2jHiVP3/ 1/MLPi3Wtj192Irm/SdIAW9wfs5BRBfiWE6kmX207sNrx9NNmD2SzkM2yVTBooq5n7FD 4EcbKNo3zXu9LLCZTQz3p3V8G29NqnPFDHKwnaqxuwtDPqoCD7uSWOl/0XonCeoSLTE/ 0qVw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750282614; x=1750887414; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=aNDi082gHLYZGqaDJOminivR/r3TSe8LUCX7HkuKKMM=; b=pJaR+X4qy3jbNycJtL1JK0R/ZI5StQNK0AQl9hdMMeJRiVsQ2RHBzIQCKBaWcbO1jX BlJsZk8fjaKQNyJn2Zdt2gowToe9KXClQ+kDlp1IjvSYWY1ziz6iiKQxDbF3o+wEf97O 1JoHXqpnif0cvSq5qmuyhBg9+2XuGaTjTSnBULECpqPi253PJy84wov3ocdrMLXj/R1N 9LAL5dwSlCF5Cgr9R6v3+qMSnZufiE1WV/OwzVjegJd8BSIDGtIAUvXqDhELvMsipMb+ PwCYH+pfK94kposNa40UWspfM5P3i/8smGKT9GWmzGtJImtNCEzaT8mhuKinDf2MELJA caXg== X-Forwarded-Encrypted: i=1; AJvYcCW1ZclTo3ld2pVHqkYcUmJPwosZkq6byGZwseFxVZtf+PCuDkKw5FcJOIbUyiYaoepn+OYQstRWEStVl3mI@postgresql.org X-Gm-Message-State: AOJu0YyaTPPIvOstMmIfp59KoNsKswOGfIp9U7N0Q3p/aj6QnHyOXI4c EdKzrJpGoxuL161T1xxw4Gp3Xgw8p/MIaBv6ERUcoEmx2u9H0qAzAuZnWPNfJm1cxSQo4j9+kxT Pe1wf98dRpbkfd1dAUA6WSqSq9fstgU0= X-Gm-Gg: ASbGncv4wuC1xA9MDCn82R92NCkS9PS+5g6rqfvALY5oIGrfHSGRvkCqcswblCkJyKf b4okdWQvvHdOxdmTsfg+zeW9cW34ktVb8fLFdyOGGFs9te+AqLoh0VF52N2QAMRbtdj4I4+nV2e SwYnxalXpHoxXZx1OdgFt656l73MmvlmwqsPvetixADHcPzw== X-Google-Smtp-Source: AGHT+IGDlmn/aNl0MsHn/QFSPFs/8WS0obD7UylI9YbAhP6sCKEdg5pOZoRqm+lLhCfibM+fLiivhOElSCUsmo0bhxE= X-Received: by 2002:a05:6102:26d2:b0:4e6:f8b0:28da with SMTP id ada2fe7eead31-4e7f630edf4mr14918737137.11.1750282614482; Wed, 18 Jun 2025 14:36:54 -0700 (PDT) MIME-Version: 1.0 References: <202505181556.3n6oiowvntyr@alvherre.pgsql> In-Reply-To: From: Sergey Sargsyan Date: Thu, 19 Jun 2025 00:36:43 +0300 X-Gm-Features: Ac12FXyJf8gZaeDTSxiIBFpD62zQbopYn2rDFtyMdh6X-UY_NSSvueWmz353-sA Message-ID: Subject: Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements To: Mihail Nikalayeu Cc: =?UTF-8?Q?=C3=81lvaro_Herrera?= , Andres Freund , Michael Paquier , PostgreSQL Hackers , Andrey Borodin , Melanie Plageman , Matthias van de Meent Content-Type: multipart/alternative; boundary="0000000000007e3f880637df6d40" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000007e3f880637df6d40 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 =3D 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=E2=80=AFAM Mihail Nikalayeu 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. Th= is > 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 n= o > actual memory was allocated for them. > > > > Interestingly, I=E2=80=99m surprised you haven=E2=80=99t hit this segfa= ult 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 unnecessar= y=E2=80=94 > > this can be determined internally by checking if it=E2=80=99s an auxili= ary > 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 use= d > 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 change= s > to the function interface would improve compatibility. > > Yes, that=E2=80=99s 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=E2=80=99s better to stick with the old signature. > > Best regards, > Mikhail. > --0000000000007e3f880637df6d40 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
My bad, my fork's based on pg15, an= d over there tuplestore_end() does this,

void
tuplestore_end(Tupl= estorestate *state)
{
in= t i;

if (state->myfile)
BufFileClose(state->myfile);
if (state->memtuples)
= {
for (i =3D state->memtupdeleted; i= < state->memtupcount; i++)
pfre= e(state->memtuples[i]);
pfree(state-= >memtuples);
}
pfree(state->readptrs);
pfree(state);
}

=
It lets each tuple go one by one, but in pg18, it j= ust nukes the whole memory context instead.

Therefore, over pg18 patch presents no issues; however,= incorporating `_clean` and `_trim` functions for datum cases is recommende= d to prevent future developers from encountering segmentation faults when u= tilizing the interface. This minor adjustment should ensure expected funct= ionality.

Best reg= ards,=C2=A0
S

On Thu, Jun 19, 2025, 12:16= =E2=80=AFAM Mihail Nikalayeu <mihailnikalayeu@gmail.com> w= rote:
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 in= serted some tuples into the table so that STIR would have data for the vali= dation phase. The segfault occurred during a call to tuplestore_end().
>
> The root cause is that a few functions in the tuplestore code still as= sume that all stored data is a pointer and thus attempt to pfree it. This a= ssumption breaks when datumByVal is used, as the data is stored directly an= d 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 accountin= g is handled properly: we should not increment or decrement the remaining c= ontext memory size when cleaning or trimming datumByVal entries, since no a= ctual memory was allocated for them.
>
> Interestingly, I=E2=80=99m surprised you haven=E2=80=99t hit this segf= ault yourself. Are you perhaps testing on an older system where INT8OID is = passed by reference? Or is your STIR always empty during the validation pha= se?

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; perha= ps 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):
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ninja && meson test --suite setup= && meson test
--print-errorlogs --suite pg_amcheck *006*

This will help ensure everything else is working correctly on your system.<= br>
> One more point: I noticed you modified the index_create() function sig= nature. You added the relpersistence parameter, which seems unnecessary=E2= =80=94
> this can be determined internally by checking if it=E2=80=99s an auxil= iary index, in which case the index should be marked as unlogged. You also = added an
> auxiliaryIndexOfOid argument (do not remember exact naming, but was us= ed for dependency). It might be cleaner to pass this via the IndexInfo stru= cture
> instead. index_create() already has dozens of mouthful arguments, and = external extensions
> (like pg_squeeze) still rely on the old signature, so minimizing chang= es 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.
--0000000000007e3f880637df6d40--