Subject: Re: Potential ABI breakage in upcoming minor releases

From: Noah Misch <[email-masked]>
Date: Thu, 14 Nov 2024 14:41:08 -0800

On Thu, Nov 14, 2024 at 09:33:32PM +0100, Alvaro Herrera wrote:
> On 2024-Nov-14, Tom Lane wrote:
> > Alvaro Herrera <[email-masked]> writes:
> > > But timescale did crash:
> > 
> > So what is timescale doing differently?

> src/ts_catalog/catalog.c-extern TSDLLEXPORT ResultRelInfo *
> src/ts_catalog/catalog.c-ts_catalog_open_indexes(Relation heapRel)
> src/ts_catalog/catalog.c-{
> src/ts_catalog/catalog.c-   ResultRelInfo *resultRelInfo;
> src/ts_catalog/catalog.c-
> src/ts_catalog/catalog.c:   resultRelInfo = makeNode(ResultRelInfo);
> src/ts_catalog/catalog.c-   resultRelInfo->ri_RangeTableIndex = 0; /* dummy */
> src/ts_catalog/catalog.c-   resultRelInfo->ri_RelationDesc = heapRel;
> src/ts_catalog/catalog.c-   resultRelInfo->ri_TrigDesc = NULL; /* we don't fire triggers */
> src/ts_catalog/catalog.c-
> src/ts_catalog/catalog.c-   ExecOpenIndices(resultRelInfo, false);

This is a copy of PostgreSQL's CatalogOpenIndexes().  Assuming timescaledb
uses it like PostgreSQL uses CatalogOpenIndexes(), it's fine.  Specifically,
CatalogOpenIndexes() is fine since PostgreSQL does not pass the ResultRelInfo
to ModifyTable.

> Oh, hmm, there's this bit which uses struct assignment into a stack
> element:

Yes, stack allocation of a ResultRelInfo sounds relevant to the crash.  It
qualifies as a "very unusual code pattern" in the postgr.es/c/e54a42a sense.

I'm hearing the only confirmed impact on non-assert builds is the need to
recompile timescaledb.  (It's unknown whether recompiling will suffice for
timescaledb.  For assert builds, six PGXN extensions need recompilation.)  I
don't see us issuing another set of back branch releases for the purpose of
making a v16.4-built timescaledb avoid a rebuild.  The target audience would
be someone who can get a new PostgreSQL build but can't get a new timescaledb
build.  That feels like a small audience.  What's missing in that analysis?

Going forward, for struct field additions, I plan to make my own patches more
ABI-breakage-averse than the postgr.es/c/e54a42a standard.

---

From: Gregory Smith <[email-masked]>
Date: Thu, 14 Nov 2024 19:01:42 -0500

On Thu, Nov 14, 2024 at 5:41 PM Noah Misch <[email-masked]> wrote:

> timescaledb.  For assert builds, six PGXN extensions need recompilation.)
>

That matches what our build and test teams are seeing.

We dug into the two lines of impacted Citus code, they are just touching
columnar metadata.  We dragged Marco into a late night session to double
check that with the Citus columnar regression tests and look for red flags
in the code.  In an assert Citus built against 16.4 running against
PostgreSQL 16.5, he hit the assert warnings, but the tests pass and there's
no signs or suspicion of a functional impact:

CREATE TABLE columnar_table_1 (a int) USING columnar;
INSERT INTO columnar_table_1 VALUES (1);
+WARNING:  problem in alloc set Stripe Write Memory Context: detected write
past chunk end in block 0x563ee43a4f10, chunk 0x563ee43a6240
+WARNING:  problem in alloc set Stripe Write Memory Context: detected write
past chunk end in block 0x563ee4369bb0, chunk 0x563ee436acb0
+WARNING:  problem in alloc set Stripe Write Memory Context: detected write
past chunk end in block 0x563ee4369bb0, chunk 0x563ee436b3c8

Thanks to everyone who's jumped in to investigate here.  With the PL/Perl
CVE at an 8.8, sorting out how to get that fix to everyone and navigate the
breakage is very important.

--
Greg Smith, Crunchy Data
Director of Open Source Strategy

---

From: Aleksander Alekseev <[email-masked]>
Date: Fri, 15 Nov 2024 11:56:45 +0300

Hi,

> CatalogOpenIndexes() is fine since PostgreSQL does not pass the ResultRelInfo

Yes. Initially I thought this was done for compatibility reasons with
different PG versions, but this doesn't seem to be the case since
CatalogOpenIndexes() was in the core for a long time.

> qualifies as a "very unusual code pattern" in the postgr.es/c/e54a42a sense.

Yes, this is another issue.

I'm working with the TimescaleDB dev team to fix these issues on the
TimescaleDB side.

-- 
Best regards,
Aleksander Alekseev

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 15 Nov 2024 12:31:05 +0100

> build.  That feels like a small audience.  What's missing in that analysis?

I agree with your conclusion that no rewrap is needed.  I previously
said otherwise, based on claims that there were multiple extensions
causing crashes.  If the one crash we know about is because timescaledb
is using an unusual coding pattern, they can fix that more easily than
we can.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos.  Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)

---

From: Marco Slot <[email-masked]>
Date: Fri, 15 Nov 2024 12:50:35 +0100

> I'm working with the TimescaleDB dev team to fix these issues on the

I looked a bit at this out of interest. I see an assert failure in the
lines below when running tests with TimescaleDB compiled against 17.0
with 17.1 installed. Without the assertion it would anyway segfault
below.

    /*
     * Usually, mt_lastResultIndex matches the target rel.  If it happens not
     * to, we can get the index the hard way with an integer division.
     */
    whichrel = mtstate->mt_lastResultIndex;
    if (resultRelInfo != mtstate->resultRelInfo + whichrel)
    {
        whichrel = resultRelInfo - mtstate->resultRelInfo;
        Assert(whichrel >= 0 && whichrel < mtstate->mt_nrels);
    }

    updateColnos = (List *) list_nth(node->updateColnosLists, whichrel);

The problem here is that because TimescaleDB compiled against 17.0
assumes a struct size of 376 (on my laptop) while PostgreSQL allocated
the array with a struct size of 384, so the pointer math no longer
holds and the whichrel value becomes nonsense. (1736263376 for
whatever reason)

cheers,
Marco

---

From: Aleksander Alekseev <[email-masked]>
Date: Fri, 15 Nov 2024 15:03:21 +0300

Hi Macro,

> holds and the whichrel value becomes nonsense. (1736263376 for

Thanks for reporting. Yes, the code assumed fixed
sizeof(ResultRelInfo) within a given PG major release branch which
turned out not to be the case. We will investigate whether it can be
easily fixed on TimescaleDB side.

-- 
Best regards,
Aleksander Alekseev

---

From: Mats Kindahl <[email-masked]>
Date: Fri, 15 Nov 2024 14:21:40 +0100

> CatalogOpenIndexes() is fine since PostgreSQL does not pass the

This seems to be old code, so I have replaced it with CatalogOpenIndexes
(and friends) and it seems to work fine.

> qualifies as a "very unusual code pattern" in the postgr.es/c/e54a42a

This is a lesson for us to make sure that we have better checks for these
kinds of situations. We should be able to deal with the extension by
allocating, e.g., twice the needed memory for the struct (and make sure to
zero it out). Then there is no risk of stepping on other object's data.

> timescaledb.  For assert builds, six PGXN extensions need recompilation.)

I re-compiled and ran with the same version on the server and extension and
that seems to work fine. Nothing that stands out. (We have a few other
things that behave strange, so might have to take that back.)

> I don't see us issuing another set of back branch releases for the purpose
> of

-- 
Best wishes,
Mats Kindahl, Timescale

---

From: Mats Kindahl <[email-masked]>
Date: Fri, 15 Nov 2024 14:27:56 +0100

> holds and the whichrel value becomes nonsense. (1736263376 for

That was one of the failures that we had, but we also have a copy of the
ExecModifyTable() function and this line would in that case be a problem:

607
608             /* Preload local variables */
609             resultRelInfo = node->resultRelInfo + node->
mt_lastResultIndex;
610             subplanstate = outerPlanState(node);
611

This stores several resultRelInfo as an array (it seems), which trivially
will break if you pass it back and forth between extension and server code
(where sizeof(ResultRelInfo) is different). For this case, a List might be
better since it will just contain pointers and the extension will in that
case just not read the added fields.
-- 
Best wishes,
Mats Kindahl, Timescale

---

From: Christoph Berg <[email-masked]>
Date: Fri, 15 Nov 2024 14:45:42 +0100

Re: Noah Misch
> timescaledb.  For assert builds, six PGXN extensions need recompilation.)

Which 6 extensions are these?

I re-ran the tests on all the extensions on apt.pg.o and didn't see
any failures (timescale and age are already rebuilt).

Christoph

---

From: Noah Misch <[email-masked]>
Date: Fri, 15 Nov 2024 06:32:17 -0800

> any failures (timescale and age are already rebuilt).

I counted wrong, it's the five here:
https://postgr.es/m/[email-masked]

They are AGE and the four others you found not to be part of apt.pg.o.
(timescaledb is not part of PGXN, so it's not one of the five.)