Re: Potential ABI breakage in upcoming minor releases

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

> On Fri, Nov 15, 2024 at 9:57 AM Aleksander Alekseev
> <aleksander(at)timescale(dot)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