Thread

Incorrect assertion in ExecBuildAggTrans ?

Andrew Gierth <andrew@tao11.riddles.org.uk> — 2020-12-21T12:02:16Z

ExecBuildAggTrans has this sequence:

                if (pertrans->deserialfn.fn_strict)
                    scratch.opcode = EEOP_AGG_STRICT_DESERIALIZE;
                else
                    scratch.opcode = EEOP_AGG_DESERIALIZE;

                scratch.d.agg_deserialize.fcinfo_data = ds_fcinfo;
                scratch.d.agg_deserialize.jumpnull = -1;    /* adjust later */
                scratch.resvalue = &trans_fcinfo->args[argno + 1].value;
                scratch.resnull = &trans_fcinfo->args[argno + 1].isnull;

                ExprEvalPushStep(state, &scratch);
                adjust_bailout = lappend_int(adjust_bailout,
                                             state->steps_len - 1);

but later on, where adjust_bailout is processed, we see this (note that
EEOP_AGG_DESERIALIZE is not checked for):

            if (as->opcode == EEOP_JUMP_IF_NOT_TRUE)
            {
                Assert(as->d.jump.jumpdone == -1);
                as->d.jump.jumpdone = state->steps_len;
            }
            else if (as->opcode == EEOP_AGG_STRICT_INPUT_CHECK_ARGS ||
                     as->opcode == EEOP_AGG_STRICT_INPUT_CHECK_NULLS)
            {
                Assert(as->d.agg_strict_input_check.jumpnull == -1);
                as->d.agg_strict_input_check.jumpnull = state->steps_len;
            }
            else if (as->opcode == EEOP_AGG_STRICT_DESERIALIZE)
            {
                Assert(as->d.agg_deserialize.jumpnull == -1);
                as->d.agg_deserialize.jumpnull = state->steps_len;
            }
            else
                Assert(false);

Seems clear to me that the assertion is wrong, and that even though a
non-strict DESERIALIZE opcode might not need jumpnull filled in, the
code added it to adjust_bailout anyway, so we crash out here on an
asserts build. This may have been overlooked because all the builtin
deserialize functions appear to be strict, but postgis has at least one
non-strict one and can hit this.

This could be fixed either by fixing the assert, or by not adding
non-strict deserialize opcodes to adjust_bailout; anyone have any
preferences?

-- 
Andrew (irc:RhodiumToad)