Pg18 Recursive Crash

Thanks for the report!

On Mon, Dec 16, 2024 at 09:50:39AM -0800, Paul Ramsey wrote:

Apologies if this is already reported, but there�s a crasher in recursive
queries at the head of the current development that happened to be
exercised by our regression suite. Here is a core-only reproduction.

CREATE TABLE foo (id integer, x integer, y integer);
INSERT INTO foo VALUES (1, 0, 1);
INSERT INTO foo VALUES (2, 1, 2);
INSERT INTO foo VALUES (3, 2, 3);

WITH RECURSIVE path (id, x, y) AS (
SELECT id, x, y FROM foo WHERE id = 1
UNION
SELECT foo.id, foo.x, foo.y
FROM path, foo
WHERE path.y = foo.x
)
SELECT 'crash', id, x, y FROM path;

git-bisect is pointing me to https://postgr.es/c/0f57382. Here is the
trace I'm seeing:

TRAP: failed Assert("op->d.fetch.kind == slot->tts_ops"), File: "../postgresql/src/backend/executor/execExprInterp.c", Line: 2244, PID: 5031
0 postgres 0x000000010112d068 ExceptionalCondition + 108
1 postgres 0x0000000100e54f04 ExecInterpExpr + 604
2 postgres 0x0000000100e5bd50 LookupTupleHashEntry + 116
3 postgres 0x0000000100e8e580 ExecRecursiveUnion + 140
4 postgres 0x0000000100e770c0 CteScanNext + 248
5 postgres 0x0000000100e66e9c ExecScan + 124
6 postgres 0x0000000100e5dc9c standard_ExecutorRun + 304
7 postgres 0x0000000100ffe6dc PortalRunSelect + 236
8 postgres 0x0000000100ffe2f8 PortalRun + 492
9 postgres 0x0000000100ffd298 exec_simple_query + 1276
10 postgres 0x0000000100ffaf24 PostgresMain + 3632
11 postgres 0x0000000100ff631c BackendInitialize + 0
12 postgres 0x0000000100f5d638 PgArchShmemSize + 0
13 postgres 0x0000000100f61518 ServerLoop + 4300
14 postgres 0x0000000100f5fc60 InitProcessGlobals + 0
15 postgres 0x0000000100eb1e7c help + 0
16 dyld 0x000000019ed3b154 start + 2476

--
nathan

On Tue, 17 Dec 2024 at 07:10, Nathan Bossart <nathandbossart@gmail.com> wrote:

On Mon, Dec 16, 2024 at 09:50:39AM -0800, Paul Ramsey wrote:

CREATE TABLE foo (id integer, x integer, y integer);
INSERT INTO foo VALUES (1, 0, 1);
INSERT INTO foo VALUES (2, 1, 2);
INSERT INTO foo VALUES (3, 2, 3);

WITH RECURSIVE path (id, x, y) AS (
SELECT id, x, y FROM foo WHERE id = 1
UNION
SELECT foo.id, foo.x, foo.y
FROM path, foo
WHERE path.y = foo.x
)
SELECT 'crash', id, x, y FROM path;

git-bisect is pointing me to https://postgr.es/c/0f57382. Here is the
trace I'm seeing:

Thanks for the report and bisection. Looking now.

David

On Tue, 17 Dec 2024 at 07:10, Nathan Bossart <nathandbossart@gmail.com> wrote:

git-bisect is pointing me to https://postgr.es/c/0f57382. Here is the
trace I'm seeing:

TRAP: failed Assert("op->d.fetch.kind == slot->tts_ops"), File: "../postgresql/src/backend/executor/execExprInterp.c", Line: 2244, PID: 5031
0 postgres 0x000000010112d068 ExceptionalCondition + 108
1 postgres 0x0000000100e54f04 ExecInterpExpr + 604
2 postgres 0x0000000100e5bd50 LookupTupleHashEntry + 116
3 postgres 0x0000000100e8e580 ExecRecursiveUnion + 140

This is caused by me being overly optimistic about getting rid of the
slot deformation step in the ExprState evaluation for hashing. I
must've not fully understood the method of how hash table lookups are
performed for grouping requirements and mistakenly thought it was ok
to use &TTSOpsMinimalTuple for hashing the same as the equality code
is doing in ExecBuildGroupingEqual(). It turns out the ExprState built
by ExecBuildGroupingEqual() always uses slots with minimal tuples due
to how LookupTupleHashEntry_internal() always creates a hash table
entry without a key and then does ExecCopySlotMinimalTuple() to put
the tuple into the hash table after the hash bucket is reserved.
That's not the case for generating the hash value as that uses
whichever slot is passed to LookupTupleHashEntry().

To fix the reported crash, I propose adding a new parameter to
BuildTupleHashTableExt() to allow passing of the TupleTableSlotOps.
Really, I could just set scratch.d.fetch.kind to NULL in
ExecBuildHash32FromAttrs() so that the hashing ExprState is always
built with a deform step, but there are many cases (e.g notSubplan.c)
where a virtual slot is always used, so it would be nice to have some
way to pass the TupleTableSlotOps in for cases where it's known so
that the deform step can be eliminated when it's not needed.

The slightly annoying thing here is that the attached patch passes the
TupleTableSlotOps as NULL in nodeSetOp.c. Per nodeAppend.c line 186,
Append does not go to much effort to setting a fixed
TupleTableSlotOps. Really it could loop over all the child plans and
check if those have fixed slot types of the same type and then fix its
own resulting slot. For nodeSetOps.c use case, since the planner
(currently) injects the flag into the target list, it'll always
project and use a virtual slot type. It's maybe worth coming back and
adjusting nodeAppend.c so it works a bit harder to fix its slot type.
I think that's likely for another patch, however. Tom is also
currently working on nodeSetOps.c to change how all this works so it
no longer uses the flags method to determine the outer and inner
sides.

I plan to push the attached patch soon.

David

David Rowley <dgrowleyml@gmail.com> writes:

The slightly annoying thing here is that the attached patch passes the
TupleTableSlotOps as NULL in nodeSetOp.c. Per nodeAppend.c line 186,
Append does not go to much effort to setting a fixed
TupleTableSlotOps. Really it could loop over all the child plans and
check if those have fixed slot types of the same type and then fix its
own resulting slot. For nodeSetOps.c use case, since the planner
(currently) injects the flag into the target list, it'll always
project and use a virtual slot type. It's maybe worth coming back and
adjusting nodeAppend.c so it works a bit harder to fix its slot type.
I think that's likely for another patch, however. Tom is also
currently working on nodeSetOps.c to change how all this works so it
no longer uses the flags method to determine the outer and inner
sides.

Yeah, I see no point in putting effort into improving the current
nodeSetOp implementation. There might be a reason to change
nodeAppend as you suggest for other use-cases though.

I plan to push the attached patch soon.

I'll presumably need to rebase my nodeSetOp patch when this goes
in. I'll take a look then at whether the new code can be improved
with this additional feature.

regards, tom lane

On Wed, 18 Dec 2024 at 11:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <dgrowleyml@gmail.com> writes:

project and use a virtual slot type. It's maybe worth coming back and
adjusting nodeAppend.c so it works a bit harder to fix its slot type.
I think that's likely for another patch, however. Tom is also
currently working on nodeSetOps.c to change how all this works so it
no longer uses the flags method to determine the outer and inner
sides.

Yeah, I see no point in putting effort into improving the current
nodeSetOp implementation. There might be a reason to change
nodeAppend as you suggest for other use-cases though.

I'll have a look to see what's possible here. Maybe locally adding
some telemetry output to the regression tests to log when an ExprState
performs a deform operation would be good to test with and without the
said patch to see how widespread an improvement the patch would result
in. I expect it might be most useful for partition-wise joins, but
that'll much depend on what operations occur above the Append.

I plan to push the attached patch soon.

I'll presumably need to rebase my nodeSetOp patch when this goes
in. I'll take a look then at whether the new code can be improved
with this additional feature.

I've pushed the patch now.

David

David Rowley <dgrowleyml@gmail.com> writes:

I've pushed the patch now.

So I tried adapting my patch to not make a copy of the input slot,
and it didn't work: I was still getting assertion failures about
the slot not being a MinimalTupleSlot as expected. On investigation
it appears your patch did not fully adjust BuildTupleHashTableExt
for variable input-slot type. You need the attached as well.

I'm not sure why the existing regression tests didn't catch this.
But it may not be worth searching for a test case, because my patch
will be one ...

regards, tom lane

On Wed, 18 Dec 2024 at 14:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:

So I tried adapting my patch to not make a copy of the input slot,
and it didn't work: I was still getting assertion failures about
the slot not being a MinimalTupleSlot as expected. On investigation
it appears your patch did not fully adjust BuildTupleHashTableExt
for variable input-slot type. You need the attached as well.

Do you have a test case in master that triggers a problem here? Your
patch adjusts code that existed prior to d96d1d515, so I'm confused as
to why your patch is needed now when it wasn't before.

If you're only triggering an issue after patching with your setops
patch, are your changes maybe using FindTupleHashEntry() with an
eqcomp that isn't compatible with the 'slot' parameter you're passing
to that function?

David

David Rowley <dgrowleyml@gmail.com> writes:

On Wed, 18 Dec 2024 at 14:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:

it appears your patch did not fully adjust BuildTupleHashTableExt
for variable input-slot type. You need the attached as well.

Do you have a test case in master that triggers a problem here?

No, that's what I didn't want to spend time looking for ;-).
But here is a WIP modification of my 0001 patch from the SetOp
improvement thread -- the difference is to lobotomize
setop_load_hash_tuple to just return the input slot. Without
the execGrouping.c changes, it gets assertion failures in the core
regression tests. (My intention had been to remove
setop_load_hash_tuple and then adjust build_hash_table to
pass a non-null inputOps if the two inputs have the same tuple
slot type. But I got stuck on this step.)

regards, tom lane

On Wed, Dec 18, 2024 at 7:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <dgrowleyml@gmail.com> writes:

The slightly annoying thing here is that the attached patch passes the
TupleTableSlotOps as NULL in nodeSetOp.c. Per nodeAppend.c line 186,
Append does not go to much effort to setting a fixed
TupleTableSlotOps. Really it could loop over all the child plans and
check if those have fixed slot types of the same type and then fix its
own resulting slot. For nodeSetOps.c use case, since the planner
(currently) injects the flag into the target list, it'll always
project and use a virtual slot type. It's maybe worth coming back and
adjusting nodeAppend.c so it works a bit harder to fix its slot type.
I think that's likely for another patch, however. Tom is also
currently working on nodeSetOps.c to change how all this works so it
no longer uses the flags method to determine the outer and inner
sides.

Yeah, I see no point in putting effort into improving the current
nodeSetOp implementation. There might be a reason to change
nodeAppend as you suggest for other use-cases though.

Should we be concerned about passing a NULL TupleTableSlotOps in
nodeRecursiveUnion.c? The query below triggers the same assert
failure: the slot is expected to be TTSOpsMinimalTuple, but it is
TTSOpsBufferHeapTuple.

create table t (a int);
insert into t values (1), (1);

with recursive cte (a) as (select a from t union select a from cte)
select a from cte;

Thanks
Richard

On Wed, 18 Dec 2024 at 22:29, Richard Guo <guofenglinux@gmail.com> wrote:

Should we be concerned about passing a NULL TupleTableSlotOps in
nodeRecursiveUnion.c?

I made it pass NULL on purpose because the slot type on the recursive
union can be different on the inner and outer sides. Do you see issues
with that?

The query below triggers the same assert
failure: the slot is expected to be TTSOpsMinimalTuple, but it is
TTSOpsBufferHeapTuple.

create table t (a int);
insert into t values (1), (1);

with recursive cte (a) as (select a from t union select a from cte)
select a from cte;

That's a good find. I tested as far back as REL_13_STABLE and it's
failing the same Assert there too.

Maybe we need to backpatch passing NULL instead of &TTSOpsMinimalTuple
to ExecBuildGroupingEqual() in BuildTupleHashTableExt(). Something
like the attached patch.

As far as I see it, there's no downside to this as
ExecComputeSlotInfo() will return true anyway for the
EEOP_INNER_FETCHSOME step in ExecBuildGroupingEqual() due to minimal
tuples needing deformation anyway. For master, we could just do what
Tom proposed above so that any callers that always use virtual slots
can benefit from the elimination of the EEOP_INNER_FETCHSOME step.

David

On Wed, 18 Dec 2024 at 23:45, David Rowley <dgrowleyml@gmail.com> wrote:

Maybe we need to backpatch passing NULL instead of &TTSOpsMinimalTuple
to ExecBuildGroupingEqual() in BuildTupleHashTableExt(). Something
like the attached patch.

I've attached a more formal patch for this and I've also now done a
bit more research and experimentation as to why we didn't notice this
for so long. It looks like the non-recursive part of the UNION must
use TTSOpsBufferHeapTuple and there must be duplicates. So that
basically means you need to select all columns, otherwise, there'd be
projection and the slot would be virtual. That boils down to, you need
a table without a primary key or any unique constraints, otherwise,
you can't get the duplicate value that's required to trigger the
Assert failure. I hope the proposed commit message is enough to
explain this in enough detail.

Of course, there may maybe some other path to trigger this using one
of the other users of BuildTupleHashTableExt().

I propose to quickly do a master-only follow-up commit to use the
inputOps instead of NULL in BuildTupleHashTableExt (Basically Tom's
patch from [1]/messages/by-id/2543667.1734483723@sss.pgh.pa.us)

Sound ok?

David

[1]: /messages/by-id/2543667.1734483723@sss.pgh.pa.us

David Rowley <dgrowleyml@gmail.com> writes:

On Wed, 18 Dec 2024 at 23:45, David Rowley <dgrowleyml@gmail.com> wrote:

Maybe we need to backpatch passing NULL instead of &TTSOpsMinimalTuple
to ExecBuildGroupingEqual() in BuildTupleHashTableExt(). Something
like the attached patch.

I've attached a more formal patch for this and I've also now done a
bit more research and experimentation as to why we didn't notice this
for so long.

I suspect that another key reason for the lack of reports is that
it's an assertion failure only, with no consequences in production
builds. So ordinary users issuing such a query wouldn't notice.

I propose to quickly do a master-only follow-up commit to use the
inputOps instead of NULL in BuildTupleHashTableExt (Basically Tom's
patch from [1])

LGTM.

regards, tom lane

On Wed, Dec 18, 2024 at 7:45 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Wed, 18 Dec 2024 at 22:29, Richard Guo <guofenglinux@gmail.com> wrote:

Should we be concerned about passing a NULL TupleTableSlotOps in
nodeRecursiveUnion.c?

I made it pass NULL on purpose because the slot type on the recursive
union can be different on the inner and outer sides. Do you see issues
with that?

I see. I didn't notice any real issues with that; I was just flagged
by the XXX comment there, which raises the question of whether it's
worth working harder to determine the inputOps.

Thanks
Richard

On Thu, Dec 19, 2024 at 8:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <dgrowleyml@gmail.com> writes:

I propose to quickly do a master-only follow-up commit to use the
inputOps instead of NULL in BuildTupleHashTableExt (Basically Tom's
patch from [1])

LGTM.

+1.

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> writes:

I see. I didn't notice any real issues with that; I was just flagged
by the XXX comment there, which raises the question of whether it's
worth working harder to determine the inputOps.

I was intending to add some code to my nodeSetop patch to see if
both input plan nodes use the same fixed slot type, and if so
pass that rather than NULL to BuildTupleHashTableExt. Perhaps
nodeRecursiveunion could do the same thing (in which case we
probably ought to abstract that out to a subroutine).

regards, tom lane