BUG #18252: Assert in CheckOpSlotCompatibility() fails when recursive union filters tuples in non-recursive term

The following bug has been logged on the website:

Bug reference: 18252
Logged by: Alexander Lakhin
Email address: exclusion(at)gmail(dot)com
PostgreSQL version: 16.1
Operating system: Ubuntu 22.04
Description:

The following query:
CREATE TABLE t(i int);
INSERT INTO t VALUES (1), (1);

WITH RECURSIVE rt(i) AS (
SELECT * FROM t
UNION
SELECT * FROM rt
)
SELECT * FROM rt;

triggers an assertion failure with the following call stack:
Core was generated by `postgres: law regression [local] SELECT
'.
Program terminated with signal SIGABRT, Aborted.

warning: Section `.reg-xstate/1408945' in core file too small.
#0 __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140277092927296) at ./nptl/pthread_kill.c:44
44 ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0 __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140277092927296) at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=140277092927296) at
./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=140277092927296, signo=signo(at)entry=6) at
./nptl/pthread_kill.c:89
#3 0x00007f94cdaf0476 in __GI_raise (sig=sig(at)entry=6) at
../sysdeps/posix/raise.c:26
#4 0x00007f94cdad67f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x000056431a46890d in ExceptionalCondition (conditionName=0x56431a5fe928
"op->d.fetch.kind == slot->tts_ops", fileName=0x56431a5fe64a
"execExprInterp.c", lineNumber=2005) at assert.c:66
#6 0x0000564319fbeca1 in CheckOpSlotCompatibility (op=0x56431c3cee30,
slot=0x56431c3ce588) at execExprInterp.c:2005
#7 0x0000564319fbb9d5 in ExecInterpExpr (state=0x56431c3ceda0,
econtext=0x56431c3cf330, isnull=0x7ffd627dddff) at execExprInterp.c:534
#8 0x0000564319fbe8d9 in ExecInterpExprStillValid (state=0x56431c3ceda0,
econtext=0x56431c3cf330, isNull=0x7ffd627dddff) at execExprInterp.c:1870
#9 0x0000564319fc487f in ExecEvalExprSwitchContext (state=0x56431c3ceda0,
econtext=0x56431c3cf330, isNull=0x7ffd627dddff) at
../../../src/include/executor/executor.h:355
#10 0x0000564319fc4909 in ExecQual (state=0x56431c3ceda0,
econtext=0x56431c3cf330) at ../../../src/include/executor/executor.h:424
#11 0x0000564319fc4978 in ExecQualAndReset (state=0x56431c3ceda0,
econtext=0x56431c3cf330) at ../../../src/include/executor/executor.h:441
#12 0x0000564319fc6957 in TupleHashTableMatch (tb=0x56431c3cebb0,
tuple1=0x56431c4032e8, tuple2=0x0) at execGrouping.c:559
#13 0x0000564319fc5240 in tuplehash_insert_hash_internal (tb=0x56431c3cebb0,
key=0x0, hash=2072154383, found=0x7ffd627ddf77) at
../../../src/include/lib/simplehash.h:653
#14 0x0000564319fc559b in tuplehash_insert_hash (tb=0x56431c3cebb0, key=0x0,
hash=2072154383, found=0x7ffd627ddf77) at
../../../src/include/lib/simplehash.h:774
#15 0x0000564319fc67fa in LookupTupleHashEntry_internal
(hashtable=0x56431c3ceb20, slot=0x56431c3ce588, isnew=0x7ffd627de00f,
hash=2072154383) at execGrouping.c:507
#16 0x0000564319fc642e in LookupTupleHashEntry (hashtable=0x56431c3ceb20,
slot=0x56431c3ce588, isnew=0x7ffd627de00f, hash=0x0) at execGrouping.c:322
#17 0x000056431a01bc1e in ExecRecursiveUnion (pstate=0x56431c3cdc38) at
nodeRecursiveunion.c:97
#18 0x0000564319ff3c92 in ExecProcNode (node=0x56431c3cdc38) at
../../../src/include/executor/executor.h:273
#19 0x0000564319ff3dd9 in CteScanNext (node=0x56431c3cf410) at
nodeCtescan.c:103
#20 0x0000564319fda4d3 in ExecScanFetch (node=0x56431c3cf410,
accessMtd=0x564319ff3c94 <CteScanNext>, recheckMtd=0x564319ff3e5d
<CteScanRecheck>) at execScan.c:132
#21 0x0000564319fda54c in ExecScan (node=0x56431c3cf410,
accessMtd=0x564319ff3c94 <CteScanNext>, recheckMtd=0x564319ff3e5d
<CteScanRecheck>) at execScan.c:181
#22 0x0000564319ff3eb6 in ExecCteScan (pstate=0x56431c3cf410) at
nodeCtescan.c:164
#23 0x0000564319fc8f3d in ExecProcNode (node=0x56431c3cf410) at
../../../src/include/executor/executor.h:273
#24 0x0000564319fcbef5 in ExecutePlan (estate=0x56431c3cd968,
planstate=0x56431c3cf410, use_parallel_mode=false, operation=CMD_SELECT,
sendTuples=true, numberTuples=0, direction=ForwardScanDirection,
dest=0x56431c3dfd40,
execute_once=true) at execMain.c:1670
#25 0x0000564319fc9625 in standard_ExecutorRun (queryDesc=0x56431c320c18,
direction=ForwardScanDirection, count=0, execute_once=true) at
execMain.c:365
#26 0x0000564319fc942e in ExecutorRun (queryDesc=0x56431c320c18,
direction=ForwardScanDirection, count=0, execute_once=true) at
execMain.c:309
#27 0x000056431a2863ec in PortalRunSelect (portal=0x56431c371cd8,
forward=true, count=0, dest=0x56431c3dfd40) at pquery.c:924
#28 0x000056431a286014 in PortalRun (portal=0x56431c371cd8,
count=9223372036854775807, isTopLevel=true, run_once=true,
dest=0x56431c3dfd40, altdest=0x56431c3dfd40, qc=0x7ffd627de520) at
pquery.c:768
#29 0x000056431a27edd4 in exec_simple_query (query_string=0x56431c2f67a8
"WITH RECURSIVE rt(i) AS (\n SELECT * FROM t\n UNION\n SELECT *
FROM rt\n)\nSELECT * FROM rt;") at postgres.c:1274
#30 0x000056431a283e42 in PostgresMain (dbname=0x56431c32fc38 "regression",
username=0x56431c2f27d8 "law") at postgres.c:4637
#31 0x000056431a1a4c5e in BackendRun (port=0x56431c3230a0) at
postmaster.c:4464
#32 0x000056431a1a44ea in BackendStartup (port=0x56431c3230a0) at
postmaster.c:4192
#33 0x000056431a1a082f in ServerLoop () at postmaster.c:1782
#34 0x000056431a1a00d9 in PostmasterMain (argc=3, argv=0x56431c2f0710) at
postmaster.c:1466
#35 0x000056431a054518 in main (argc=3, argv=0x56431c2f0710) at main.c:198

(gdb) frame 6
#6 0x0000564319fbeca1 in CheckOpSlotCompatibility (op=0x56431c3cee30,
slot=0x56431c3ce588)
at execExprInterp.c:2005
2005 Assert(op->d.fetch.kind == slot->tts_ops);
(gdb) p op->d.fetch.kind
$1 = (const TupleTableSlotOps *) 0x56431a7ebcc0 <TTSOpsMinimalTuple>
(gdb) p slot->tts_ops
$2 = (const TupleTableSlotOps * const) 0x56431a7ebd20
<TTSOpsBufferHeapTuple>

With "INSERT INTO t VALUES (1), (2)" the assert is not reached during
processing of the non-recursive term.

Reproduced on REL_12_STABLE .. master.
This check failure can be observed since the introduction of
CheckOpSlotCompatibility (15d8f8312) + f92cd7392.

On Tue, Dec 19, 2023 at 12:30 AM PG Bug reporting form <
noreply(at)postgresql(dot)org> wrote:

> CREATE TABLE t(i int);
> INSERT INTO t VALUES (1), (1);
>
> WITH RECURSIVE rt(i) AS (
> SELECT * FROM t
> UNION
> SELECT * FROM rt
> )
> SELECT * FROM rt;

Nice catch. The TupleHashTable's tableslot is created as type
TTSOpsMinimalTuple. The RecursiveUnion's non-recursive term generates
tuples of type TTSOpsBufferHeapTuple. If the non-recursive term
produces duplicate tuples, we'd find non-empty bucket when inserting the
same key into the hashtable. In this case we need to check to see if
the two tuples match: one is of the hashtable's tableslot
(TTSOpsMinimalTuple), and the other comes from the non-recursive term
(TTSOpsBufferHeapTuple). And the incompatible slot type would fail
CheckOpSlotCompatibility.

I have no idea about how to fix it.

BTW, while reading the codes, I noticed two typos in simplehash.h, one
is in the comment of SH_INSERT, the other is in the comment of
SH_INSERT_HASH. Attached is a trivial patch for that.

Thanks
Richard

On 19.12.23 12:28, Richard Guo wrote:
> BTW, while reading the codes, I noticed two typos in simplehash.h, one
> is in the comment of SH_INSERT, the other is in the comment of
> SH_INSERT_HASH.  Attached is a trivial patch for that.

/*
- * Insert the key key into the hash-table, set *found to true if the key
- * already exists, false otherwise. Returns the hash-table entry in either
- * case.
+ * Insert the key into the hash-table, set *found to true if the key
already
+ * exists, false otherwise. Returns the hash-table entry in either case.
*/
SH_SCOPE SH_ELEMENT_TYPE *
SH_INSERT(SH_TYPE * tb, SH_KEY_TYPE key, bool *found)

I suppose the original intention might have been "Insert the key passed
in the parameter named key ..." or "Insert the key `key` ..." or
something like that. But even if so, that's clearly confusing, so I am
in favor of your patch, just pointing this out in case someone wants to
protest.

On 26.12.23 15:42, Peter Eisentraut wrote:
> On 19.12.23 12:28, Richard Guo wrote:
>> BTW, while reading the codes, I noticed two typos in simplehash.h, one
>> is in the comment of SH_INSERT, the other is in the comment of
>> SH_INSERT_HASH.  Attached is a trivial patch for that.
>
>  /*
> - * Insert the key key into the hash-table, set *found to true if the key
> - * already exists, false otherwise. Returns the hash-table entry in either
> - * case.
> + * Insert the key into the hash-table, set *found to true if the key
> already
> + * exists, false otherwise. Returns the hash-table entry in either case.
>   */
>  SH_SCOPE       SH_ELEMENT_TYPE *
>  SH_INSERT(SH_TYPE * tb, SH_KEY_TYPE key, bool *found)
>
>
> I suppose the original intention might have been "Insert the key passed
> in the parameter named key ..." or "Insert the key `key` ..." or
> something like that.  But even if so, that's clearly confusing, so I am
> in favor of your patch, just pointing this out in case someone wants to
> protest.

hearing none, committed

On Tue, Jan 2, 2024 at 5:22 PM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> On 26.12.23 15:42, Peter Eisentraut wrote:
> > On 19.12.23 12:28, Richard Guo wrote:
> >> BTW, while reading the codes, I noticed two typos in simplehash.h, one
> >> is in the comment of SH_INSERT, the other is in the comment of
> >> SH_INSERT_HASH. Attached is a trivial patch for that.
> >
> > /*
> > - * Insert the key key into the hash-table, set *found to true if the key
> > - * already exists, false otherwise. Returns the hash-table entry in
> either
> > - * case.
> > + * Insert the key into the hash-table, set *found to true if the key
> > already
> > + * exists, false otherwise. Returns the hash-table entry in either case.
> > */
> > SH_SCOPE SH_ELEMENT_TYPE *
> > SH_INSERT(SH_TYPE * tb, SH_KEY_TYPE key, bool *found)
> >
> >
> > I suppose the original intention might have been "Insert the key passed
> > in the parameter named key ..." or "Insert the key `key` ..." or
> > something like that. But even if so, that's clearly confusing, so I am
> > in favor of your patch, just pointing this out in case someone wants to
> > protest.
>
> hearing none, committed

Thanks for pushing!

Thanks
Richard

On Tue, Dec 19, 2023 at 7:28 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:

> On Tue, Dec 19, 2023 at 12:30 AM PG Bug reporting form <
> noreply(at)postgresql(dot)org> wrote:
>
>> CREATE TABLE t(i int);
>> INSERT INTO t VALUES (1), (1);
>>
>> WITH RECURSIVE rt(i) AS (
>> SELECT * FROM t
>> UNION
>> SELECT * FROM rt
>> )
>> SELECT * FROM rt;
>
>
> Nice catch. The TupleHashTable's tableslot is created as type
> TTSOpsMinimalTuple. The RecursiveUnion's non-recursive term generates
> tuples of type TTSOpsBufferHeapTuple. If the non-recursive term
> produces duplicate tuples, we'd find non-empty bucket when inserting the
> same key into the hashtable. In this case we need to check to see if
> the two tuples match: one is of the hashtable's tableslot
> (TTSOpsMinimalTuple), and the other comes from the non-recursive term
> (TTSOpsBufferHeapTuple). And the incompatible slot type would fail
> CheckOpSlotCompatibility.
>

This Assert was added in 15d8f8312:

commit 15d8f83128e15de97de61430d0b9569f5ebecc26
Author: Andres Freund <andres(at)anarazel(dot)de>
Date: Thu Nov 15 22:00:30 2018 -0800

Verify that expected slot types match returned slot types.

So loop in Andres to have a look.

Thanks
Richard