From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: WIP: Faster Expression Processing v4
Date: Thu, 23 Mar 2017 21:26:19 -0400
Message-ID: <30270.1490318779@sss.pgh.pa.us> (raw)
In-Reply-To: <20170324004734.3a2dokbfofiz6ylu@alap3.anarazel.de>
References: <20170314224706.nxvtapenky6eom3z@alap3.anarazel.de>
<13062.1489622943@sss.pgh.pa.us>
<20170320062511.hp5qeurtxrwsvfxr@alap3.anarazel.de>
<31150.1490193666@sss.pgh.pa.us>
<20170322145240.vt2iolwt6vpmx33z@alap3.anarazel.de>
<31922.1490194838@sss.pgh.pa.us>
<28270.1490289299@sss.pgh.pa.us>
<20170323174004.a2ez7kygx5ngqxlp@alap3.anarazel.de>
<8011.1490305255@sss.pgh.pa.us>
<26088.1490315792@sss.pgh.pa.us>
<20170324004734.3a2dokbfofiz6ylu@alap3.anarazel.de>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Andres Freund <andres@anarazel.de> writes:
> On 2017-03-23 20:36:32 -0400, Tom Lane wrote:
>> The problem here is that once we drop the buffer pin, any pointers we may
>> have into on-disk data are dangling pointers --- we're at risk of some
>> other backend taking away that shared buffer. (So I'm afraid that the
>> commentary there is overly optimistic.) So even though those pointers
>> may still be there beyond tts_nvalid, subsequent references to them are
>> very dangerous.
> This applies to the code in master as well, no? An ExecEvalScalarVar()
> followed by an ExecEvalWholeRowVar() would have precisely the same
> effect?
Yeah. The other order would be safe, because ExecEvalScalarVar would do
slot_getattr which would re-extract the value from the newly materialized
tuple. But there definitely seems to be a hazard for the order you
mentioned.
> Do we need to do anything about this in the back-branches,
> given how unlikely this is going to be in practice?
Probably not. As I mentioned, I think this may be only theoretical rather
than real, if you believe that buffer pins would only be associated with
slots holding references to regular tuples. And even if it's not
theoretical, the odds of seeing a failure in the field seem pretty tiny
given that a just-released buffer shouldn't be subject to recycling for
a fair while. But I don't want to leave it like this going forward.
>> So I think that we have got to fix ExecEvalWholeRowVar so that it doesn't
>> clobber the state of the slot.
> That seems like a good plan.
Yeah. I have the stopgap code in my working copy, and will look at
refactoring the tuptoaster code for better performance later.
>> Also, while trying to test the above scenario, I realized that the patch
>> as submitted was being way too cavalier about where it was applying
>> CheckVarSlotCompatibility and so on. The ASSIGN_FOO_VAR steps, for
>> instance, had no protection at all.
> I don't think that's true - the assign checks had copied the code from
> the old ExecBuildProjectionInfo, setting isSimpleVar iff
> (!attr->attisdropped && variable->vartype == attr->atttypid) - we can
> check that for projections in contrast to normal expressions because we
> already know the slot.
Hmm, I see ... but that only works in the cases where the caller of
ExecBuildProjectionInfo supplied a source slot, and a lot of 'em don't.
As the code stands, we are unable to use ASSIGN_FOO_VAR in quite a lot
of places, including everywhere above the relation scan level.
I'd already put in the infrastructure to add ASSIGN_FOO_VAR_FIRST
step types. I could take it back out, but I wonder if it wouldn't be
smarter to keep it and remove the restriction in ExecBuildProjectionInfo.
Or maybe we could have ExecBuildProjectionInfo emit either
ASSIGN_FOO_VAR_FIRST or ASSIGN_FOO_VAR depending on whether it can prove
the reference safe.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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: tgl@sss.pgh.pa.us, andres@anarazel.de, hlinnaka@iki.fi, tomas.vondra@2ndquadrant.com
Subject: Re: WIP: Faster Expression Processing v4
In-Reply-To: <30270.1490318779@sss.pgh.pa.us>
* 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