Re: make tuplestore helper function

On Mon, Nov 8, 2021 at 3:13 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Nov 08, 2021 at 02:52:28PM -0500, Melanie Plageman wrote:
> > On Tue, Nov 2, 2021 at 4:23 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > Several places have a conditional value for the first argument (randomAccess),
> > > but your patch changes the behavior to a constant "true".  I didn't review the
> > > patch beyond that.
> > >
> > > > @@ -740,18 +724,14 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
> > > > -     tupstore =
> > > > -             tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> > > > -                                                       false, work_mem);
> > >
> > > > @@ -2701,42 +2701,13 @@ pg_hba_file_rules(PG_FUNCTION_ARGS)
> > > > -     tuple_store =
> > > > -             tuplestore_begin_heap(rsi->allowedModes & SFRM_Materialize_Random,
> > > > -                                                       false, work_mem);
> > >
> > > > @@ -4799,31 +4797,8 @@ pg_timezone_names(PG_FUNCTION_ARGS)
> > > > -     randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > > > -     tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> > >
> > > > @@ -575,38 +575,12 @@ pg_ls_dir_1arg(PG_FUNCTION_ARGS)
> > > > -     randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > > > -     tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> > >
> > > > @@ -1170,17 +1154,12 @@ pg_cursor(PG_FUNCTION_ARGS)
> > > > -     tupstore =
> > > > -             tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> > > > -                                                       false, work_mem);
> > >
> > > > +++ b/src/backend/utils/fmgr/funcapi.c
> > > > +     tupstore = tuplestore_begin_heap(true, false, maxKBytes);
> >
> > I believe the patch has preserved the same behavior. All of the callers
> > for which I replaced tuplestore_begin_heap() which passed a variable for
> > the randomAccess parameter had set that variable to something which was
> > effectively the same as passing true -- SFRM_Materialize_Random.
>
> I don't think so ?
>
> They callers aren't passing SFRM_Materialize_Random, but rather
> (allowedModes & SFRM_Materialize_Random) != 0
>
> Where allowedModes is determined EXEC_FLAG_BACKWARD.
>
> src/include/executor/executor.h:extern Tuplestorestate *ExecMakeTableFunctionResult(SetExprState *setexpr,
> src/include/executor/executor.h-                                                    ExprContext *econtext,
> src/include/executor/executor.h-                                                    MemoryContext argContext,
> src/include/executor/executor.h-                                                    TupleDesc expectedDesc,
> src/include/executor/executor.h-                                                    bool randomAccess);
>
> src/backend/executor/nodeFunctionscan.c=FunctionNext(FunctionScanState *node)
> src/backend/executor/nodeFunctionscan.c:             ExecMakeTableFunctionResult(node->funcstates[0].setexpr,
> src/backend/executor/nodeFunctionscan.c-                                         node->ss.ps.ps_ExprContext,
> src/backend/executor/nodeFunctionscan.c-                                         node->argcontext,
> src/backend/executor/nodeFunctionscan.c-                                         node->funcstates[0].tupdesc,
> src/backend/executor/nodeFunctionscan.c-                                         node->eflags & EXEC_FLAG_BACKWARD);
>

You are right. I misread it.

So, I've attached a patch where randomAccess is now an additional
parameter (and registered for the next fest).

I was thinking about how to add a test that would have broken when I
passed true for randomAccess to tuplestore_begin_heap() when false was
required. But, I don't fully understand the problem. If backward
accesses to a tuplestore are not allowed but randomAccess is mistakenly
passed as true, would the potential result be potentially wrong results
from accessing the tuplestore results backwards?

- Melanie