On Thu, Oct 22, 2015 at 11:59:58PM -0400, Robert Haas wrote:
> On Thu, Oct 15, 2015 at 8:23 PM, Noah Misch <n...@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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to