Subject: Re: Potential ABI breakage in upcoming minor releases

From: Alvaro Herrera <[email-masked]>
Date: Thu, 14 Nov 2024 21:06:59 +0100

On 2024-Nov-14, Christoph Berg wrote:

> It didn't actually crash, true.

But timescale did crash:

https://pgdgbuild.dus.dg-i.net/job/timescaledb-autopkgtest/9/architecture=amd64,distribution=sid/console

11:59:51 @@ -34,6 +40,7 @@
11:59:51  CREATE INDEX ON _hyper_1_3_chunk(temp2);
11:59:51  -- INSERT only will not trigger vacuum on indexes for PG13.3+
11:59:51  UPDATE vacuum_test SET time = time + '1s'::interval, temp1 = random(), temp2 = random();
11:59:51 --- we should see two parallel workers for each chunk
11:59:51 -VACUUM (PARALLEL 3) vacuum_test;
11:59:51 -DROP TABLE vacuum_test;
11:59:51 +server closed the connection unexpectedly
11:59:51 +	This probably means the server terminated abnormally
11:59:51 +	before or while processing the request.
11:59:51 +connection to server was lost

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
(Ijon Tichy en Viajes, Stanislaw Lem)

---

From: Tom Lane <[email-masked]>
Date: Thu, 14 Nov 2024 15:09:34 -0500

> But timescale did crash:

So what is timescale doing differently?

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 14 Nov 2024 21:33:32 +0100

> So what is timescale doing differently?

No idea.  Maybe a stacktrace from a core would tell us more; but the
jenkins task doesn't seem to have server logs or anything else to gives
us more clues.

There are three makeNode(ResultRelInfo), but they don't look anything
out of the ordinary:

C perhan: timescaledb 0$ git grep -C5 makeNode.ResultRelInfo
src/copy.c-  *
src/copy.c-  * WARNING. The dummy rangetable index is decremented by 1 (unchecked)
src/copy.c-  * inside `ExecConstraints` so unless you want to have a overflow, keep it
src/copy.c-  * above zero. See `rt_fetch` in parsetree.h.
src/copy.c-  */
src/copy.c: resultRelInfo = makeNode(ResultRelInfo);
src/copy.c-
src/copy.c-#if PG16_LT
src/copy.c- ExecInitRangeTable(estate, pstate->p_rtable);
src/copy.c-#else
src/copy.c- Assert(pstate->p_rteperminfos != NULL);
--
src/nodes/chunk_dispatch/chunk_insert_state.c-create_chunk_result_relation_info(const ChunkDispatch *dispatch, Relation rel)
src/nodes/chunk_dispatch/chunk_insert_state.c-{
src/nodes/chunk_dispatch/chunk_insert_state.c-  ResultRelInfo *rri;
src/nodes/chunk_dispatch/chunk_insert_state.c-  ResultRelInfo *rri_orig = dispatch->hypertable_result_rel_info;
src/nodes/chunk_dispatch/chunk_insert_state.c-  Index hyper_rti = rri_orig->ri_RangeTableIndex;
src/nodes/chunk_dispatch/chunk_insert_state.c:  rri = makeNode(ResultRelInfo);
src/nodes/chunk_dispatch/chunk_insert_state.c-
src/nodes/chunk_dispatch/chunk_insert_state.c-  InitResultRelInfo(rri, rel, hyper_rti, NULL, dispatch->estate->es_instrument);
src/nodes/chunk_dispatch/chunk_insert_state.c-
src/nodes/chunk_dispatch/chunk_insert_state.c-  /* Copy options from the main table's (hypertable's) result relation info */
src/nodes/chunk_dispatch/chunk_insert_state.c-  rri->ri_WithCheckOptions = rri_orig->ri_WithCheckOptions;
--
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);

I didn't catch any other allocations that would depend on the size of
ResultRelInfo in obvious ways.

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

tsl/src/compression/compression.c-1559- if (decompressor->indexstate->ri_NumIndices > 0)
tsl/src/compression/compression.c-1560- {
tsl/src/compression/compression.c:1561:     ResultRelInfo indexstate_copy = *decompressor->indexstate;
tsl/src/compression/compression.c-1562-     Relation single_index_relation;

I find this rather suspect.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La espina, desde que nace, ya pincha" (Proverbio africano)

---

From: Aleksander Alekseev <[email-masked]>
Date: Thu, 14 Nov 2024 23:44:11 +0300

Hi Alvaro,

> [...]

ExecOpenIndices() will not fill the ri_needLockTagTuple field for the
caller and it will contain garbage.

Since you assumed this is the correct usage, maybe it should?

-- 
Best regards,
Aleksander Alekseev