From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: pgsql-hackers@postgresql.org, Andres Freund <andres@anarazel.de>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Subject: Re: make tuplestore helper function
Date: Thu, 18 Nov 2021 12:24:30 -0600
Message-ID: <20211118182430.GO17618@telsasoft.com> (raw)
In-Reply-To: <CAAKRu_b2rySeOC=zZDd-++N6AXTuL-uABpPKQgO8f2NV4j5dPw@mail.gmail.com>
References: <CAAKRu_azyd1Z3W_r7Ou4sorTjRCs+PxeHw1CWJeXKofkE6TuZg@mail.gmail.com>
<20211102202353.GG17618@telsasoft.com>
<CAAKRu_a-ZZTez9yn4EMDAQQgt1UFpFcTVr=MDxf-b75tZ9q8UQ@mail.gmail.com>
<20211108201338.GU17618@telsasoft.com>
<CAAKRu_b2rySeOC=zZDd-++N6AXTuL-uABpPKQgO8f2NV4j5dPw@mail.gmail.com>
On Thu, Nov 18, 2021 at 12:59:03PM -0500, Melanie Plageman wrote:
> 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.
> > > >
...
> > > 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.1583955158@sss.pgh.pa.us
Maybe you'd want to add an 0002 patch which changes those to conditional ?
BTW, there should be a newline before MakeFuncResultTuplestore().
--
Justin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, melanieplageman@gmail.com, andres@anarazel.de, alvherre@alvh.no-ip.org
Subject: Re: make tuplestore helper function
In-Reply-To: <20211118182430.GO17618@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox