On Thu, Oct 22, 2015 at 11:59:58PM -0400, Robert Haas wrote:
> On Thu, Oct 15, 2015 at 8:23 PM, Noah Misch <noah@leadboat.com> wrote:
> > Agreed. More specifically, I had in mind for copyParamList() to check the
> > mask while e.g. ExecEvalParamExtern() would either check nothing or merely
> > assert that any mask included the requested parameter. It would be tricky to
> > verify that as safe, so ...
> >
> >> Would it work to define this as "if non-NULL,
> >> params lacking a 1-bit may be safely ignored"? Or some other tweak
> >> that basically says that you don't need to care about this, but you
> >> can if you want to.
> >
> > ... this is a better specification.
>
> Here's an attempt to implement that.
Since that specification permits ParamListInfo consumers to ignore paramMask,
the plpgsql_param_fetch() change from copy-paramlistinfo-fixes.patch is still
formally required.
> @@ -50,6 +51,7 @@ copyParamList(ParamListInfo from)
> retval->parserSetup = NULL;
> retval->parserSetupArg = NULL;
> retval->numParams = from->numParams;
> + retval->paramMask = bms_copy(from->paramMask);
Considering that this function squashes the masked params, I wonder if it
should just store NULL here.
>
> for (i = 0; i < from->numParams; i++)
> {
> @@ -58,6 +60,20 @@ copyParamList(ParamListInfo from)
> int16 typLen;
> bool typByVal;
>
> + /*
> + * Ignore parameters we don't need, to save cycles and space, and
> + * in case the fetch hook might fail.
> + */
> + if (retval->paramMask != NULL &&
> + !bms_is_member(i, retval->paramMask))
The "and in case the fetch hook might fail" in this comment and its clones is
contrary to the above specification. Under that specification, it would be a
bug in the ParamListInfo producer to rely on consumers checking paramMask.
Saving cycles/space would be the spec-approved paramMask use.
Consider adding an XXX comment to the effect that cursors ought to stop using
unshared param lists. The leading comment at setup_unshared_param_list() is a
good home for such an addition.
