On Thu, Nov 18, 2021 at 1:24 PM Justin Pryzby <pry...@telsasoft.com> wrote: > > On Thu, Nov 18, 2021 at 12:59:03PM -0500, Melanie Plageman wrote: > > On Mon, Nov 8, 2021 at 3:13 PM Justin Pryzby <pry...@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 <pry...@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. > > > > > > ... > > > > 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. > ... > > 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? > > No, see here > > src/backend/utils/fmgr/README-The Tuplestore must be created with > randomAccess = true if > src/backend/utils/fmgr/README:SFRM_Materialize_Random is set in allowedModes, > but it can (and preferably > src/backend/utils/fmgr/README-should) be created with randomAccess = false if > not. Callers that can support > src/backend/utils/fmgr/README-both ValuePerCall and Materialize mode will set > SFRM_Materialize_Preferred, > src/backend/utils/fmgr/README-or not, depending on which mode they prefer. > > If you use "randomAccess=true" when it's not needed, the result might be less > efficient. > > Some callers specify "true" when they don't need to. I ran into that here. > https://www.postgresql.org/message-id/21724.1583955...@sss.pgh.pa.us > > Maybe you'd want to add an 0002 patch which changes those to conditional ?
Done in attached v4 > BTW, there should be a newline before MakeFuncResultTuplestore(). Fixed - Melanie
v4-0002-Check-allowedModes-when-creating-tuplestores.patch
Description: Binary data
v4-0001-Add-helper-to-make-tuplestore.patch
Description: Binary data
