Date: Thu, 14 Nov 2024 14:41:08 -0800
From: Noah Misch <noah@leadboat.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Christoph Berg <myon@debian.org>,
	Pavan Deolasee <pavan.deolasee@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Potential ABI breakage in upcoming minor releases
Message-ID: <20241114224108.d0.nmisch@google.com>
References: <1976928.1731614974@sss.pgh.pa.us>
 <202411142033.u6za5a6ylj2k@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202411142033.u6za5a6ylj2k@alvherre.pgsql>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: <https://www.postgresql.org/message-id/20241114224108.d0.nmisch%40google.com>
Precedence: bulk

On Thu, Nov 14, 2024 at 09:33:32PM +0100, Alvaro Herrera wrote:
> On 2024-Nov-14, Tom Lane wrote:
> > Alvaro Herrera <alvherre@alvh.no-ip.org> 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.