BUG #18722: Processing arrays with plpgsql raises errors

PG Bug reporting form <noreply@postgresql.org> writes:

The following script:
CREATE FUNCTION make_ia() RETURNS int[] LANGUAGE plpgsql AS
'declare x int[]; begin x := array[0]; return x; end';
CREATE FUNCTION ia_eq(int[], int[]) RETURNS boolean LANGUAGE plpgsql AS
'begin return array_eq($1, $2); end';
CREATE OPERATOR = (procedure = ia_eq, leftarg = int[], rightarg = int[]);
SELECT NULLIF(make_ia(), array[1]::int[]);

fails with:
ERROR: cache lookup failed for type 2139062143

Nice catch! What is happening here is that make_ia returns a
read/write pointer to an expanded array object. The EEOP_NULLIF
code passes that pointer straight on to the equality function.
Which in this case is a plpgsql function that will suppose it
can take ownership of the expanded object, resulting in said
object being freed before return. (Neither function has done
anything wrong.) The problem is that EEOP_NULLIF then returns
the original Datum pointer, which is now pointing at garbage.
The different failures you get depending on what is done next
with the Datum are not too surprising.

What EEOP_NULLIF needs to do is pass a read-only pointer to the
equality function, so that the object is not modified and remains
available to return if we want to do so.

Attached is a quick WIP patch to handle that. It is missing a test
case, but the real omission is that llvm_compile_expr()'s EEOP_NULLIF
handling also needs to be fixed, and I'm pretty unsure how to do that.

I'm wondering now if any of our other conditional expressions have
similar bugs ...

regards, tom lane

I wrote:

What EEOP_NULLIF needs to do is pass a read-only pointer to the
equality function, so that the object is not modified and remains
available to return if we want to do so.

Attached is a quick WIP patch to handle that. It is missing a test
case, but the real omission is that llvm_compile_expr()'s EEOP_NULLIF
handling also needs to be fixed, and I'm pretty unsure how to do that.

Here's a fleshed-out patch with a test case and JIT support. This
is about the first time I've messed with LLVM, so I wouldn't mind
some review of what I did in llvmjit_expr.c. In particular, do I
correctly understand that "l_funcvalue(b, v_fcinfo, 0)" produces
a reference to a copy of the initial value of args[0].value?
It seems to work in testing --- the original R/W pointer is returned
out of the expression --- but if we returned the R/O pointer instead
it'd be mighty difficult to detect the resulting inefficiency.

I'm wondering now if any of our other conditional expressions have
similar bugs ...

I was amused to discover that case.sql already had infrastructure
suitable for testing this, thanks to bug #14472.

regards, tom lane

On Sun, 24 Nov 2024 at 22:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Here's a fleshed-out patch with a test case and JIT support. This
is about the first time I've messed with LLVM, so I wouldn't mind
some review of what I did in llvmjit_expr.c. In particular, do I
correctly understand that "l_funcvalue(b, v_fcinfo, 0)" produces
a reference to a copy of the initial value of args[0].value?

I don't know about that, but I wonder if this bug could be fixed by
having ExecInitExprRec() insert a EEOP_MAKE_READONLY step. Then it
wouldn't be necessary to make any changes to the expression evaluation
code.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

On Sun, 24 Nov 2024 at 22:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Here's a fleshed-out patch with a test case and JIT support. This
is about the first time I've messed with LLVM, so I wouldn't mind
some review of what I did in llvmjit_expr.c. In particular, do I
correctly understand that "l_funcvalue(b, v_fcinfo, 0)" produces
a reference to a copy of the initial value of args[0].value?

I don't know about that, but I wonder if this bug could be fixed by
having ExecInitExprRec() insert a EEOP_MAKE_READONLY step. Then it
wouldn't be necessary to make any changes to the expression evaluation
code.

That would entirely destroy one of the primary performance benefits of
the expanded-object infrastructure. The idea is that if you have
fconsumer(fproducer(...), ...)
and fproducer returns a read-write pointer to an object it's built,
then fconsumer should be able to take ownership of the object and
use it as a local variable (possibly modifying it) without incurring
any object-copying overhead.

This works in any context where an intermediate expression value
has a single consumer, which is most. If there are multiple
consumers then we need to insert MAKE_READONLY steps for all
(or all but one) of them. I overlooked EEOP_NULLIF as such
a case, but I don't think there are so many more cases as to
justify throwing away the concept altogether.

regards, tom lane

On Mon, 25 Nov 2024 at 15:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

I don't know about that, but I wonder if this bug could be fixed by
having ExecInitExprRec() insert a EEOP_MAKE_READONLY step. Then it
wouldn't be necessary to make any changes to the expression evaluation
code.

That would entirely destroy one of the primary performance benefits of
the expanded-object infrastructure. The idea is that if you have
fconsumer(fproducer(...), ...)
and fproducer returns a read-write pointer to an object it's built,
then fconsumer should be able to take ownership of the object and
use it as a local variable (possibly modifying it) without incurring
any object-copying overhead.

This works in any context where an intermediate expression value
has a single consumer, which is most. If there are multiple
consumers then we need to insert MAKE_READONLY steps for all
(or all but one) of them. I overlooked EEOP_NULLIF as such
a case, but I don't think there are so many more cases as to
justify throwing away the concept altogether.

I didn't mean do it in all cases, I just meant the NullIfExpr case
identified here. My point was that instead of modifying the evaluation
code for EEOP_NULLIF to make it call
MakeExpandedObjectReadOnlyInternal(), it would be easier to insert a
EEOP_MAKE_READONLY step for the first argument of the EEOP_NULLIF
step.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

I didn't mean do it in all cases, I just meant the NullIfExpr case
identified here. My point was that instead of modifying the evaluation
code for EEOP_NULLIF to make it call
MakeExpandedObjectReadOnlyInternal(), it would be easier to insert a
EEOP_MAKE_READONLY step for the first argument of the EEOP_NULLIF
step.

But then the NULLIF step would only have access to the R/O pointer,
no? We do want to pass on a R/W pointer to the output, if we got
one, to handle cases like
fconsumer(NULLIF(fproducer(...), ...), ...)
Admittedly that's a pretty edgy edge-case, but still we're leaving
money on the table if we don't do it. So I think we have to deal
with the issue within NULLIF.

regards, tom lane

On Mon, 25 Nov 2024 at 19:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

I didn't mean do it in all cases, I just meant the NullIfExpr case
identified here. My point was that instead of modifying the evaluation
code for EEOP_NULLIF to make it call
MakeExpandedObjectReadOnlyInternal(), it would be easier to insert a
EEOP_MAKE_READONLY step for the first argument of the EEOP_NULLIF
step.

But then the NULLIF step would only have access to the R/O pointer,
no? We do want to pass on a R/W pointer to the output, if we got
one, to handle cases like
fconsumer(NULLIF(fproducer(...), ...), ...)
Admittedly that's a pretty edgy edge-case, but still we're leaving
money on the table if we don't do it. So I think we have to deal
with the issue within NULLIF.

OK, that makes sense.

Regards,
Dean