Date: Fri, 4 Oct 2019 21:45:35 -0700
From: Andres Freund <andres@anarazel.de>
To: Peter Geoghegan <pg@bowt.ie>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16036: Segmentation fault while doing an update
Message-ID: <20191005044535.onxnohvdhp5ihmd3@alap3.anarazel.de>
References: <4E1FB33C-7280-487B-B3D1-D3C247AA9E3D@anarazel.de>
 <FBA5344D-9356-49BA-9B47-C3705C2E9481@gmail.com>
 <A1695349-530C-4871-AED5-0262A9BC9920@anarazel.de>
 <746398E3-F4E1-4F14-A686-3394F27011DC@gmail.com>
 <20191004032020.se46diwqas43e3tw@alap3.anarazel.de>
 <11540.1570159561@sss.pgh.pa.us>
 <20191004043021.ylfbxcjqyypphfft@alap3.anarazel.de>
 <20191004192848.s3cgkt7n7g3cbvja@alap3.anarazel.de>
 <20191004214300.pqe7fruaemklz5qn@alap3.anarazel.de>
 <20191004222437.45qmglpto43pd3jb@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191004222437.45qmglpto43pd3jb@alap3.anarazel.de>
Precedence: bulk

Hi,

On 2019-10-04 15:24:37 -0700, Andres Freund wrote:
> On 2019-10-04 14:43:00 -0700, Andres Freund wrote:
> > > I think there's a good case to be made to backpatch the tests further
> > > than 12, but I'm not sure about it? They do pass (with one error message
> > > about a failure to delete changed to a failure to update, we didn't use
> > > to be able to discern) back to 9.6, requiring
> > 
> > I did push the tests back to 9.6.
> 
> There's a few ordering violations in the tests, e.g.:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2019-10-04%2021%3A51%3A13

I've now disabled that portion of the test. It look to me like this
might be a subtle upsert bug.  What happens is that in the
isolationtester sequence (all operating on one row, and all already in a
transaction):
    "s1_upd_a_data" "s3_upd_a_data" "s2_upsert_a_data" "s1_upd_a_data" "s1_c" "s3_del_a" "s3_c" "s2_c"

sometimes the s2_upsert_a_data gets to update the row before the
s3_upd_a_data. That does *not* happen with a plain update, even though
there's at that point always a target row to operate on (i.e. this
happens before s3_del_a is reached). I think the cause for that might be
that the check_exclusion_or_unique_constraint() check done for upsert
(via ExecCheckIndexConstraints() in ExecInsert()) does a
XactLockTableWait() without, like heapam.c would, ordering via a tuple
lock.

Since it's clearly not 12 specific, and not really related to triggers /
EPQ, I think disabling it for now is a sensible choice.


>  key-a          val-a-s1-ups1-ups1
>  step s1_c: COMMIT;
> -s3: NOTICE:  upd: text key-a = text key-a: t
> -s3: NOTICE:  upk: text val-a-s1-ups1-ups1 <> text mismatch: t
> -s3: NOTICE:  trigger: name rep_b_u; when: BEFORE; lev: ROWs; op: UPDATE; old: (key-a,val-a-s1-ups1-ups1) new: (key-a,val-a-s1-ups1-ups1-ups3)
> -s3: NOTICE:  trigger: name rep_a_u; when: AFTER; lev: ROWs; op: UPDATE; old: (key-a,val-a-s1-ups1-ups1) new: (key-a,val-a-s1-ups1-ups1-ups3)
> -step s3_upd_a_data: <... completed>
> +s2: NOTICE:  upd: text key-a = text key-a: t
> +s2: NOTICE:  upk: text val-a-s1-ups1-ups1 <> text mismatch: t
> +s2: NOTICE:  trigger: name rep_b_u; when: BEFORE; lev: ROWs; op: UPDATE; old: (key-a,val-a-s1-ups1-ups1) new: (key-a,val-a-s1-ups1-ups1-upserts2)
> +s2: NOTICE:  trigger: name rep_a_u; when: AFTER; lev: ROWs; op: UPDATE; old: (key-a,val-a-s1-ups1-ups1) new: (key-a,val-a-s1-ups1-ups1-upserts2)
> +step s2_upsert_a_data: <... completed>
>  key            data           
> 
> I was under the assumption that it'd be deterministic who gets to
> continue with a speculative insertion, but that ain't so.

This was a preliminary theory, a wrong one: Speculative insertions
aren't actually involved, see above...

Greetings,

Andres Freund