Incorrect assertion in ExecBuildAggTrans ? - Mailing list pgsql-hackers

From Andrew Gierth
Subject Incorrect assertion in ExecBuildAggTrans ?
Date
Msg-id 87mty7peb3.fsf@news-spur.riddles.org.uk
Whole thread Raw
List pgsql-hackers
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)



pgsql-hackers by date:

Previous
From: Maxim Orlov
Date:
Subject: Re: [PATCH] Automatic HASH and LIST partition creation
Next
From: Amit Kapila
Date:
Subject: Re: Single transaction in the tablesync worker?