Horton

Re: Potential ABI breakage in upcoming minor releases

Mats Kindahl <mats@timescale.com>

View thread
From: Mats Kindahl <mats@timescale.com>
To: Marco Slot <marco.slot@gmail.com>
Cc: Aleksander Alekseev <aleksander@timescale.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, Noah Misch <noah@leadboat.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Tom Lane <tgl@sss.pgh.pa.us>, Christoph Berg <myon@debian.org>, Pavan Deolasee <pavan.deolasee@gmail.com>
Date: 2024-11-15T13:27:56Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Undo unintentional ABI break in struct ResultRelInfo.

  2. For inplace update durability, make heap_update() callers wait.

  3. Fix btmarkpos/btrestrpos array key wraparound bug.

  4. Fix calculation of which GENERATED columns need to be updated.

On Fri, Nov 15, 2024 at 12:51 PM Marco Slot <marco.slot@gmail.com> wrote:

> On Fri, Nov 15, 2024 at 9:57 AM Aleksander Alekseev
> <aleksander@timescale.com> wrote:
> > I'm working with the TimescaleDB dev team to fix these issues on the
> > TimescaleDB side.
>
> 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)
>

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