Re: [BUGS] BUG #14150: Attempted to delete invisible tuple

[Peter Geoghegan]

On Mon, Jun 13, 2016 at 4:52 PM, Peter Tripp <pe...@chartio.com> wrote:
> Thank you for your assistance in tracking this down Peter G. My apologies
> for the delayed reply, I've was out sick in days following my original post.

So, I've managed to create a test case that triggers this bug
reliably, without involving VACUUM, or foreign keys.

I have a test case involving pgbench and some conflicting jsonb-based
UPSERT statements with large, toastable jsonb datums. I did hand tune
the data distributions and number of clients a little to get the
problem to reproduce. I was attempting to roughly simulate a
customer's problem report, having failed with simpler cases that
involved upserting with TOAST (I approximated the data distributions
involved, and so on).

Here is a backtrace following promoting the "attempted to delete
invisible tuple" elog's elevel to PANIC:

(gdb) bt
#0 0x00007fbdbe2be418 in __GI_raise (sig=sig@entry=6) at
../sysdeps/unix/sysv/linux/raise.c:54
#1 0x00007fbdbe2c001a in __GI_abort () at abort.c:89
#2 0x00000000007dd674 in errfinish (dummy=<optimized out>) at elog.c:557
#3 0x00000000004a4e84 in heap_delete
(relation=relation@entry=0x7fbdbf9aab38, tid=tid@entry=0x156e9bc,
cid=0, crosscheck=crosscheck@entry=0x0, wait=wait@entry=1 '\001',
hufd=hufd@entry=0x7ffdccd28630) at heapam.c:3015
#4 0x00000000004a4f03 in simple_heap_delete
(relation=relation@entry=0x7fbdbf9aab38, tid=0x156e9bc) at
heapam.c:3378
#5 0x00000000004ae224 in toast_delete_datum (value=<optimized out>,
rel=0x7fbdbf9a5250) at tuptoaster.c:1706
#6 0x00000000004aee17 in toast_delete (rel=rel@entry=0x7fbdbf9a5250,
oldtup=oldtup@entry=0x7ffdccd2bf90) at tuptoaster.c:509
#7 0x00000000004a7cbf in heap_abort_speculative
(relation=0x7fbdbf9a5250, tuple=0x15b87b0) at heapam.c:6003
#8 0x00000000005e5624 in ExecInsert (canSetTag=1 '\001',
estate=0x156d0d0, onconflict=ONCONFLICT_UPDATE,
arbiterIndexes=0x15aa460, planSlot=0x156d7d8, slot=0x156d7d8,
mtstate=0x156d320) at nodeModifyTable.c:443
#9 ExecModifyTable (node=0x156d320) at nodeModifyTable.c:1496

Sure enough, the error code path goes through simple_heap_delete(),
and this is related to TOAST. It usually takes no more than 5 seconds
for the test case to show the problem. I'll go work on a proper
diagnosis now.

Thanks for your help, Peter.

--
Peter Geoghegan


--
Sent via pgsql-bugs mailing list (pgsql...@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

[Michael Paquier]

On Tue, Jun 28, 2016 at 10:41 AM, Peter Geoghegan <p...@heroku.com> wrote:
> On Mon, Jun 13, 2016 at 4:52 PM, Peter Tripp <pe...@chartio.com> wrote:
>> Thank you for your assistance in tracking this down Peter G. My apologies
>> for the delayed reply, I've was out sick in days following my original post.
>
> So, I've managed to create a test case that triggers this bug
> reliably, without involving VACUUM, or foreign keys.

Could you post it? I'd be interested in seeing that.
--
Michael

[Peter Geoghegan]

On Mon, Jun 27, 2016 at 8:28 PM, Michael Paquier
<michael...@gmail.com> wrote:
> Could you post it? I'd be interested in seeing that.

Not right now, because it involves mutated customer data. I can
whittle it down later -- it's probably just a matter of having the
sweetspot number of TOAST chunks, or something like that.

What I can provide right away is some pg_xlogdump output (well, SQL
query results against a table loaded with pg_xlogdump output),
including some rough annotations of my own, highlighting the
transaction that raises the error (the one that has an abort record,
since I didn't force a PANIC). That is attached.

--
Peter Geoghegan

[Michael Paquier]

On Wed, Jun 29, 2016 at 8:40 AM, Peter Geoghegan <p...@heroku.com> wrote:
> On Mon, Jun 27, 2016 at 8:28 PM, Michael Paquier
> <michael...@gmail.com> wrote:
>> Could you post it? I'd be interested in seeing that.
>

> Attached is a reasonably minimal testcase, showing many "attempted to
> delete invisible tuple" errors for me quickly and reliably. The
> shellscript expects to be able to invoke pgbench without any custom
> arguments, and runs the attach custom pgbench script in an infinite
> loop. On my laptop, there is a roughly 90% chance that the script's 5
> second pgbench runs will raise the error at least once.

Thanks! I can see the problem with that. Now let's dig into it...

[Oskari Saarenmaa]

29.06.2016, 08:13, Michael Paquier kirjoitti:
> On Wed, Jun 29, 2016 at 8:40 AM, Peter Geoghegan <p...@heroku.com> wrote:
>> On Mon, Jun 27, 2016 at 8:28 PM, Michael Paquier
>> <michael...@gmail.com> wrote:
>>> Could you post it? I'd be interested in seeing that.
>>
>> Attached is a reasonably minimal testcase, showing many "attempted to
>> delete invisible tuple" errors for me quickly and reliably. The
>> shellscript expects to be able to invoke pgbench without any custom
>> arguments, and runs the attach custom pgbench script in an infinite
>> loop. On my laptop, there is a roughly 90% chance that the script's 5
>> second pgbench runs will raise the error at least once.
>
> Thanks! I can see the problem with that. Now let's dig into it...

I ran into this yesterday on 9.5.3 and created a test case before
noticing this thread. My test case just inserts a single
toast-requiring row using ON CONFLICT DO NOTHING and typically fails
immediately:

pgbench -f random.sql -n -T 2 -c 2

random.sql:

-- CREATE TABLE vtest (id INT UNIQUE, blob BYTEA);
TRUNCATE vtest;
INSERT INTO vtest (id, blob) VALUES (1, encrypt(repeat('a',
8192)::bytea, 'x'::bytea, 'aes')) ON CONFLICT DO NOTHING;

ISTM this is caused by toast knowing nothing about speculative
insertion: when two backends have executed a speculative heap_insert
with a conflicting key and the latter one tries to abort after receiving
specConflict there's nothing in tqual.c to say that the toast rows
associated with speculative insertion should be visible to that operation.

/ Oskari

[Peter Geoghegan]

On Tue, Jul 5, 2016 at 11:05 PM, Oskari Saarenmaa <o...@aiven.io> wrote:
> ISTM this is caused by toast knowing nothing about speculative insertion:
> when two backends have executed a speculative heap_insert with a conflicting
> key and the latter one tries to abort after receiving specConflict there's
> nothing in tqual.c to say that the toast rows associated with speculative
> insertion should be visible to that operation.

This analysis may be relevant:

https://www.postgresql.org/message-id/CACjxUsMBEHexQehM6P7ck+7A...@mail.gmail.com


--
Peter Geoghegan

[Oskari Saarenmaa]

06.07.2016, 09:05, Oskari Saarenmaa kirjoitti:
> I ran into this yesterday on 9.5.3 and created a test case before
> noticing this thread. My test case just inserts a single
> toast-requiring row using ON CONFLICT DO NOTHING and typically fails
> immediately:
>
> pgbench -f random.sql -n -T 2 -c 2
>
> random.sql:
>
> -- CREATE TABLE vtest (id INT UNIQUE, blob BYTEA);
> TRUNCATE vtest;
> INSERT INTO vtest (id, blob) VALUES (1, encrypt(repeat('a',
> 8192)::bytea, 'x'::bytea, 'aes')) ON CONFLICT DO NOTHING;
>

> ISTM this is caused by toast knowing nothing about speculative
> insertion: when two backends have executed a speculative heap_insert
> with a conflicting key and the latter one tries to abort after receiving
> specConflict there's nothing in tqual.c to say that the toast rows
> associated with speculative insertion should be visible to that operation.

The attached patch against current master allows heap_abort_speculative
to delete toast rows created by the same command which makes the above
test case and "make check" run without failures. Note that I haven't
touched this code before so I don't know how safe my patch is.

/ Oskari

[Andres Freund]

On 2016-07-05 23:24:13 -0700, Peter Geoghegan wrote:
> On Tue, Jul 5, 2016 at 11:05 PM, Oskari Saarenmaa <o...@aiven.io> wrote:

> > ISTM this is caused by toast knowing nothing about speculative insertion:
> > when two backends have executed a speculative heap_insert with a conflicting
> > key and the latter one tries to abort after receiving specConflict there's
> > nothing in tqual.c to say that the toast rows associated with speculative
> > insertion should be visible to that operation.
>

> This analysis may be relevant:
>
> https://www.postgresql.org/message-id/CACjxUsMBEHexQehM6P7ck+7A...@mail.gmail.com

Not sure what that has to do with the problem?

Andres

[Andres Freund]

Hi,

On 2016-07-06 13:30:57 +0300, Oskari Saarenmaa wrote:
> The attached patch against current master
> allows heap_abort_speculative to delete
> toast rows created by the same command
> which makes the above test case and "make
> check" run without failures. Note that I
> haven't touched this code before so I
> don't know how safe my patch is.

I've not looked closely at this yet, but unfortunately we probably can't
just update the API of HeapTupleSatisfiesUpdate and friends, this needs
to be backpatched to 9.5...

Andres Freund

[Peter Geoghegan]

On Wed, Jul 6, 2016 at 3:30 AM, Oskari Saarenmaa <o...@aiven.io> wrote:
>> ISTM this is caused by toast knowing nothing about speculative
>> insertion: when two backends have executed a speculative heap_insert
>> with a conflicting key and the latter one tries to abort after receiving
>> specConflict there's nothing in tqual.c to say that the toast rows
>> associated with speculative insertion should be visible to that operation.
>
>

> The attached patch against current master allows heap_abort_speculative to
> delete toast rows created by the same command which makes the above test
> case and "make check" run without failures. Note that I haven't touched
> this code before so I don't know how safe my patch is.

I don't really understand your explanation of what this patch does.
Obviously heap_abort_speculative() often has no apparent problem with
any of this; this bug involves a relatively rare race condition
scenario where there *is* a problem. We didn't simply neglect to make
heap_abort_speculative() consider TOAST at all, though.

My sense is that the problem occurs before Postgres throw this
"attempted to delete invisible tuple", and that a better fix is one
that prevents that condition from occurring in the first place
according to the extant visibility rules for
HeapTupleSatisfiesUpdate(). I'm not sure that your fix has not merely
masked the issue.

--
Peter Geoghegan

[Andres Freund]

On 2016-07-06 16:07:38 -0700, Peter Geoghegan wrote:
> On Wed, Jul 6, 2016 at 3:30 AM, Oskari Saarenmaa <o...@aiven.io> wrote:
> >> ISTM this is caused by toast knowing nothing about speculative
> >> insertion: when two backends have executed a speculative heap_insert
> >> with a conflicting key and the latter one tries to abort after receiving
> >> specConflict there's nothing in tqual.c to say that the toast rows
> >> associated with speculative insertion should be visible to that operation.
> >
> >
> > The attached patch against current master allows heap_abort_speculative to
> > delete toast rows created by the same command which makes the above test
> > case and "make check" run without failures. Note that I haven't touched
> > this code before so I don't know how safe my patch is.
>
> I don't really understand your explanation of what this patch does.
> Obviously heap_abort_speculative() often has no apparent problem with
> any of this; this bug involves a relatively rare race condition
> scenario where there *is* a problem.

Did you see oskari's reproducer in
http://archives.postgresql.org/message-id/88248a24-47d1-d575-a63f-2b56a09f82e2%40aiven.io
?

It's not really particularly hard to reproduce with that (pretty basic
interaction). Works like in 80% of the cases I tried, within less than a
sec.

> We didn't simply neglect to make heap_abort_speculative() consider
> TOAST at all, though.

Well, not quite, but nearly. Afaics it currently can only work if the
toasted columns have been inserted by a different command, before the
INSERT ON CONFLICT does anything. I don't see how it can work for newly
inserted toast data. When heap_abort_speculative deletes toast data,
when would it *ever* not fail if the same command executed the toast
data?

Andres

[Peter Geoghegan]

On Wed, Jul 6, 2016 at 4:33 PM, Andres Freund <and...@anarazel.de> wrote:
>
>> We didn't simply neglect to make heap_abort_speculative() consider
>> TOAST at all, though.
>
> Well, not quite, but nearly. Afaics it currently can only work if the
> toasted columns have been inserted by a different command, before the
> INSERT ON CONFLICT does anything. I don't see how it can work for newly
> inserted toast data. When heap_abort_speculative deletes toast data,
> when would it *ever* not fail if the same command executed the toast
> data?

Maybe we should have a testing option that makes ExecInsert() pretend
that the precheck for a conflict indicated that there wasn't one the
first time through, only (or perhaps randomly make this assumption
with a configurable probability). It would perhaps be useful to
artificially increase the number of conflicts during stress testing.
Just something to consider.

--
Peter Geoghegan

[Peter Geoghegan]

On Wed, Jul 6, 2016 at 4:33 PM, Andres Freund <and...@anarazel.de> wrote:
>> We didn't simply neglect to make heap_abort_speculative() consider
>> TOAST at all, though.
>
> Well, not quite, but nearly. Afaics it currently can only work if the
> toasted columns have been inserted by a different command, before the
> INSERT ON CONFLICT does anything. I don't see how it can work for newly
> inserted toast data. When heap_abort_speculative deletes toast data,
> when would it *ever* not fail if the same command executed the toast
> data?

Why would the toasted data ever not be newly inserted in practice? Do
you mean that that would work, where it ever to occur, though in
practice it could not, since this concerns only cases that take the
path with heap_insert()? And so, in simpler words, you believe that
any heap_abort_speculative() call to toast_delete() will cause this
error to be raised?

[Peter Geoghegan]

On Wed, Jul 6, 2016 at 2:40 PM, Andres Freund <and...@anarazel.de> wrote:
>> This analysis may be relevant:
>>
>> https://www.postgresql.org/message-id/CACjxUsMBEHexQehM6P7ck+7A...@mail.gmail.com
>
> Not sure what that has to do with the problem?

As it turns out, probably nothing.

[Andres Freund]

On 2016-07-06 17:14:30 -0700, Peter Geoghegan wrote:
> On Wed, Jul 6, 2016 at 4:33 PM, Andres Freund <and...@anarazel.de> wrote:
> >> We didn't simply neglect to make heap_abort_speculative() consider
> >> TOAST at all, though.
> >
> > Well, not quite, but nearly. Afaics it currently can only work if the
> > toasted columns have been inserted by a different command, before the
> > INSERT ON CONFLICT does anything. I don't see how it can work for newly
> > inserted toast data. When heap_abort_speculative deletes toast data,
> > when would it *ever* not fail if the same command executed the toast
> > data?
>
> Why would the toasted data ever not be newly inserted in practice?

Don't think so, atm.

> And so, in simpler words, you believe that any
> heap_abort_speculative() call to toast_delete() will cause this error
> to be raised?

Looks that way, yes.

[Peter Geoghegan]

On Wed, Jul 6, 2016 at 5:16 PM, Andres Freund <and...@anarazel.de> wrote:
>> And so, in simpler words, you believe that any
>> heap_abort_speculative() call to toast_delete() will cause this error
>> to be raised?
>
> Looks that way, yes.

The reason I doubted that it could be that simple was that it took
this long to hear about this bug. It also took me a little while to
produce a test case. I tended to doubt that all toast_delete() calls
from heap_abort_speculative() are broken, since ISTM that they're not
very rare in practice.

I may have been wrong about that, though.

--
Peter Geoghegan

[Peter Geoghegan]

On Wed, Jul 6, 2016 at 5:22 PM, Peter Geoghegan <p...@heroku.com> wrote:
> The reason I doubted that it could be that simple was that it took
> this long to hear about this bug. It also took me a little while to
> produce a test case. I tended to doubt that all toast_delete() calls
> from heap_abort_speculative() are broken, since ISTM that they're not
> very rare in practice.
>
> I may have been wrong about that, though.

Looks like I was wrong about that. Attached simple patch forces the
implementation to see no conflict when the precheck is attempted in
the first iteration. It artificially forces a conflict. This causes
the regression tests to fail in one or two places, because an error is
raised in the INSERT path that would never have been reached otherwise
(e.g. there is a second almost equivalent unique that ON CONFLICT did
not infer).

These regression test failures are not interesting, though. What is
interesting is that the "attempted to delete invisible tuple" error
can be seen in a single interactive psql session once the patch is
applied. So, there is no race condition as such at all.

--
Peter Geoghegan

[Peter Geoghegan]

On Wed, Jul 6, 2016 at 2:40 PM, Andres Freund <and...@anarazel.de> wrote:
> On 2016-07-06 13:30:57 +0300, Oskari Saarenmaa wrote:

>> The attached patch against current master
>> allows heap_abort_speculative to delete
>> toast rows created by the same command
>> which makes the above test case and "make
>> check" run without failures. Note that I
>> haven't touched this code before so I
>> don't know how safe my patch is.
>

> I've not looked closely at this yet, but unfortunately we probably can't
> just update the API of HeapTupleSatisfiesUpdate and friends, this needs
> to be backpatched to 9.5...

Having studied the problem a bit further, I now think that Oskari has
the right idea, but that his patch needs polishing.

I think that Andres is right that it's not appropriate to modify
HeapTupleSatisfiesUpdate(). I'd add that I'd avoid even using
heap_delete() directly just to suit the esoteric requirements of TOAST
super deletion (a part of the speculative insertion infrastructure). A
more specialized routine like heap_abort_speculative() itself seems
like a better fit (e.g., a recursive call to heap_abort_speculative()
for the TOAST tuple).

There are a few obvious reasons why it wouldn't just work if the
relevant code path was made to call heap_abort_speculative() rather
than simple_heap_delete() from within toast_delete_datum(). However,
heap_abort_speculative() is more or less a stripped down version of
heap_delete(), and simple_heap_delete() is just a heap_delete()
followed by a HTSU_Result test, so it would *almost* just work.

heap_abort_speculative() is concerned with deleting a speculatively
inserted tuple in a way that releases any other waiting xacts
immediately ("super deleting"), as opposed to having them wait until
the end of our (the inserter's) transaction (this avoids "unprincipled
deadlocks" [1], an important design goal for UPSERT). It is prepared
to delete a tuple inserted by the same command, where all existing
heap_delete() callers are not; so heap_abort_speculative() doesn't
even have a call to HeapTupleSatisfiesUpdate(), and so naturally
doesn't require that HeapTupleSatisfiesUpdate() be specially asked to
permit us an "instantaneous tuple deletion", which was Oskari's
approach in his patch.

Long story short, it seems like heap_abort_speculative() is at least
closer to what is required than simple_heap_delete()/heap_delete(),
because it simply doesn't care about HeapTupleSatisfiesUpdate(). In
other words, heap_abort_speculative() was designed to allow an
"instantaneous tuple deletion" from day one, so why not reuse that?

heap_abort_speculative() does ensure that it's modifying a tuple it
finds to be HeapTupleHeaderIsSpeculative(), which would not apply to
its TOAST tuple (it doesn't get that flag), so that needs to be
tweaked. Also, the "HeapTupleHeaderSetXmin(tp.t_data,
InvalidTransactionId)" part (which is what makes a super deletion, uh,
"super") may be heavy handed for TOAST tuples. Still, it looks like it
wouldn't actually break anything to allow it for TOAST tuples, since
HeapTupleSatisfiesToast() *is* prepared for a super deleted TOAST
tuple (if only defensively), and there is nothing special about
VACUUMing TOAST tables, AFAIK.

Maybe heap_abort_speculative() should be refactored to call another
function, and keep only the parts that specifically expect a
HeapTupleHeaderIsSpeculative() tuple. The function that it is made to
call (that consists of the majority of the current
heap_abort_speculative() implementation) could also be called by a
special super deletion variant of toast_delete(). No need to spread
knowledge about speculative insertion any further this way, AFAICT.
The UPSERT commit did modify two HeapTupleSatisfies* routines, but
that didn't include HeapTupleSatisfiesUpdate() (just
HeapTupleSatisfiesDirty(), and the aforementioned defensive code in
HeapTupleSatisfiesToast()).

What do you think of this outline, Oskari and Andres?

[1] https://wiki.postgresql.org/wiki/Value_locking#.22Unprincipled_Deadlocking.22_and_value_locking

[Oskari Saarenmaa]

07.07.2016, 05:15, Peter Geoghegan kirjoitti:
> Maybe heap_abort_speculative() should be refactored to call another
> function, and keep only the parts that specifically expect a
> HeapTupleHeaderIsSpeculative() tuple. The function that it is made to
> call (that consists of the majority of the current
> heap_abort_speculative() implementation) could also be called by a
> special super deletion variant of toast_delete(). No need to spread
> knowledge about speculative insertion any further this way, AFAICT.
> The UPSERT commit did modify two HeapTupleSatisfies* routines, but
> that didn't include HeapTupleSatisfiesUpdate() (just
> HeapTupleSatisfiesDirty(), and the aforementioned defensive code in
> HeapTupleSatisfiesToast()).
>
> What do you think of this outline, Oskari and Andres?

Something like the attached patch? It still modifies the toast_delete
API, but we could also implement a new speculative-tuple-aware version
of it if that's a concern.

/ Oskari

[Peter Geoghegan]

On Thu, Jul 7, 2016 at 12:43 PM, Oskari Saarenmaa <o...@aiven.io> wrote:
> Something like the attached patch? It still modifies the toast_delete API,
> but we could also implement a new speculative-tuple-aware version of it if
> that's a concern.

This looks good. I like what you've done with the toast_delete() API,
personally.

I would like to hear Andres' opinion on whether or not he feels it
okay to "HeapTupleHeaderSetXmin(tp.t_data, InvalidTransactionId)"
TOAST tuples. If we want to go that way, we should update the
defensive code's comments within HeapTupleSatisfiesToast() to
acknowledge the now very real possibility of that code being strictly
necessary and not just defensive.

It also occurs to me that logical decoding might be affected by the
fact that heap_abort_speculative() is used for TOAST tuples. It looks
fine to me, because while we ignore super deletion from within
DecodeDelete(), it's also true that we're not interested in TOAST
deletion in general from within ReorderBufferCommit(). I'd definitely
like to hear a second opinion on that from Andres, though.

[Amit Kapila]

Agreed. I think that makes sense.

> heap_abort_speculative() does ensure that it's modifying a tuple it
> finds to be HeapTupleHeaderIsSpeculative(), which would not apply to
> its TOAST tuple (it doesn't get that flag), so that needs to be
> tweaked. Also, the "HeapTupleHeaderSetXmin(tp.t_data,
> InvalidTransactionId)" part (which is what makes a super deletion, uh,
> "super") may be heavy handed for TOAST tuples. Still, it looks like it
> wouldn't actually break anything to allow it for TOAST tuples, since
> HeapTupleSatisfiesToast() *is* prepared for a super deleted TOAST
> tuple (if only defensively), and there is nothing special about
> VACUUMing TOAST tables, AFAIK.
>

> Maybe heap_abort_speculative() should be refactored to call another
> function, and keep only the parts that specifically expect a
> HeapTupleHeaderIsSpeculative() tuple. The function that it is made to
> call (that consists of the majority of the current
> heap_abort_speculative() implementation) could also be called by a
> special super deletion variant of toast_delete(). No need to spread
> knowledge about speculative insertion any further this way, AFAICT.
> The UPSERT commit did modify two HeapTupleSatisfies* routines, but
> that didn't include HeapTupleSatisfiesUpdate() (just
> HeapTupleSatisfiesDirty(), and the aforementioned defensive code in
> HeapTupleSatisfiesToast()).
>

IIUC, then I think you are proposing to have an API (something like
heap_delete_minimal) which will workout well for both heap and toast
tuples with respect to heap_abort_speculative(). I think to solve
this issue, the approach you outlined above seems to be better than
what's being done in Oskari's patch. The advantage of this approach
is that it will save us from touching HeapTupleSatisfiesUpdate and
will do the minimal things (like excluding the replica identity &
replication origin information from WAL) required for deletion of
toast tuples.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

[Peter Geoghegan]

On Sat, Jul 16, 2016 at 10:14 PM, Amit Kapila <amit.k...@gmail.com> wrote:
>> Maybe heap_abort_speculative() should be refactored to call another
>> function, and keep only the parts that specifically expect a
>> HeapTupleHeaderIsSpeculative() tuple. The function that it is made to
>> call (that consists of the majority of the current
>> heap_abort_speculative() implementation) could also be called by a
>> special super deletion variant of toast_delete(). No need to spread
>> knowledge about speculative insertion any further this way, AFAICT.
>> The UPSERT commit did modify two HeapTupleSatisfies* routines, but
>> that didn't include HeapTupleSatisfiesUpdate() (just
>> HeapTupleSatisfiesDirty(), and the aforementioned defensive code in
>> HeapTupleSatisfiesToast()).
>>
>
> IIUC, then I think you are proposing to have an API (something like
> heap_delete_minimal) which will workout well for both heap and toast
> tuples with respect to heap_abort_speculative(). I think to solve
> this issue, the approach you outlined above seems to be better than
> what's being done in Oskari's patch. The advantage of this approach
> is that it will save us from touching HeapTupleSatisfiesUpdate and
> will do the minimal things (like excluding the replica identity &
> replication origin information from WAL) required for deletion of
> toast tuples.

Right, but Oskari's latest revision of the patch is a response to this
feedback, which I'm happy with.

At the moment, fixing the bug is blocking on proper review of that
second revision. I've outlined already that I have a couple of
non-specific concerns about how it could be buggy, neither of which
are actionable by Oskari. It needs further scrutiny from other
hackers, particularly Andres, but looks correct to me.

--
Peter Geoghegan

[Amit Kapila]

On Mon, Jul 18, 2016 at 5:42 AM, Peter Geoghegan <p...@heroku.com> wrote:
> On Sat, Jul 16, 2016 at 10:14 PM, Amit Kapila <amit.k...@gmail.com> wrote:
>>> Maybe heap_abort_speculative() should be refactored to call another
>>> function, and keep only the parts that specifically expect a
>>> HeapTupleHeaderIsSpeculative() tuple. The function that it is made to
>>> call (that consists of the majority of the current
>>> heap_abort_speculative() implementation) could also be called by a
>>> special super deletion variant of toast_delete(). No need to spread
>>> knowledge about speculative insertion any further this way, AFAICT.
>>> The UPSERT commit did modify two HeapTupleSatisfies* routines, but
>>> that didn't include HeapTupleSatisfiesUpdate() (just
>>> HeapTupleSatisfiesDirty(), and the aforementioned defensive code in
>>> HeapTupleSatisfiesToast()).
>>>
>>
>> IIUC, then I think you are proposing to have an API (something like
>> heap_delete_minimal) which will workout well for both heap and toast
>> tuples with respect to heap_abort_speculative(). I think to solve
>> this issue, the approach you outlined above seems to be better than
>> what's being done in Oskari's patch. The advantage of this approach
>> is that it will save us from touching HeapTupleSatisfiesUpdate and
>> will do the minimal things (like excluding the replica identity &
>> replication origin information from WAL) required for deletion of
>> toast tuples.
>
> Right, but Oskari's latest revision of the patch is a response to this
> feedback, which I'm happy with.
>

I have somehow missed that e-mail. I don't know why, but I could not
see any of the Oskari's e-mail in this thread in my gmail inbox. I
have to check that via postgresql.org [1].


[1] - https://www.postgresql.org/list/pgsql-bugs

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

[Peter Geoghegan]

I bet it ended up in spam. I've had to white list mailing list posts explicitly in the past year.

--
Peter Geoghegan

[Oskari Saarenmaa]

18.07.2016, 05:40, Peter Geoghegan kirjoitti:
> I bet it ended up in spam. I've had to white list mailing list posts
> explicitly in the past year.

This is probably because the mailing list breaks the DKIM signature of
messages and the DMARC policy in my domain says mails which fail both
DKIM and SPF validation should be rejected.

The DMARC FAQ [1] lists a few options for making DMARC work with mailing
lists. These issues were discussed last year on -hackers [2].

/ Oskari

[1]
https://dmarc.org/wiki/FAQ#I_operate_a_mailing_list_and_I_want_to_interoperate_with_DMARC.2C_what_should_I_do.3F

[2]
https://www.postgresql.org/message-id/flat/CACjxUsPCjAFU81izZ0VcmK78EtEQ4_EjgCJK402WwwXvEZRhZA%40mail.gmail.com

[Peter Geoghegan]

On Sun, Jul 17, 2016 at 5:12 PM, Peter Geoghegan <p...@heroku.com> wrote:
> At the moment, fixing the bug is blocking on proper review of that
> second revision. I've outlined already that I have a couple of
> non-specific concerns about how it could be buggy, neither of which
> are actionable by Oskari. It needs further scrutiny from other
> hackers, particularly Andres, but looks correct to me.

Any idea of when you'll get around to this, Andres? I don't want to
miss the next point release.

--
Peter Geoghegan

[Andres Freund]

On 2016-07-07 22:43:59 +0300, Oskari Saarenmaa wrote:
> Something like the attached patch? It
> still modifies the toast_delete API, but
> we could also implement a new
> speculative-tuple-aware version of it if
> that's a concern.

Maybe I'm missing something, but toasted columns aren't actually marked
as speculative right now, no? Cf.

HeapTuple
toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
int options)
..
/*
* Ignore the INSERT_SPECULATIVE option. Speculative insertions/super
* deletions just normally insert/delete the toast values. It seems
* easiest to deal with that here, instead on, potentially, multiple
* callers.
*/
options &= ~HEAP_INSERT_SPECULATIVE;

so going through heap_abort_speculative()/heap_delete_speculative()
doesn't look right to me. We could change thing so toasted rows also
get inserted speculatively - but I don't see much of a point.

> +void
> +heap_delete_speculative(Relation relation, ItemPointer tid, bool is_toast)
> +{

If we go this way, it seems easier to get is_toast from
IsToastRelation(relation), rather than hand it through painstakingly.

Greetings,

Andres Freund

[Peter Geoghegan]

On Wed, Jul 27, 2016 at 4:53 PM, Andres Freund <and...@anarazel.de> wrote:
> Maybe I'm missing something, but toasted columns aren't actually marked
> as speculative right now, no? Cf.

> so going through heap_abort_speculative()/heap_delete_speculative()
> doesn't look right to me. We could change thing so toasted rows also
> get inserted speculatively - but I don't see much of a point.

That was my understanding also. And like you, I see zero point in
making sure that TOAST tuples are in fact speculatively inserted. On
the other hand, I also fail to see any consequence of "super deleting"
TOAST tuples created as part of speculative insertion, since
HeapTupleSatisfiesToast() is already prepared for that possibility (if
only defensively).

In other words, while I suggested that there was plenty to be reused
in heap_abort_speculative(), some things clearly had to be a little
different -- as few as possible, though. I didn't think it was worth
bothering teaching heap_abort_speculative()/heap_delete_speculative()
to give additional special treatment to these TOAST tuples just
because they weren't actually speculatively inserted. In short:
perhaps we should always "HeapTupleHeaderSetXmin(tp.t_data,
InvalidTransactionId)", even for TOAST tuples, *just* to be
consistent.

I don't think it's that important that we actually go this way. (Note
also that Oskari might have changed the wording of comments within
HeapTupleSatisfiesToast() to indicate that a super-deleted TOAST tuple
is definitely possible, and not just something to be paranoid about).

>> +void
>> +heap_delete_speculative(Relation relation, ItemPointer tid, bool is_toast)
>> +{
>
> If we go this way, it seems easier to get is_toast from
> IsToastRelation(relation), rather than hand it through painstakingly.

+1

--
Peter Geoghegan

[Andres Freund]

On 2016-07-27 16:53:47 -0700, Andres Freund wrote:
> On 2016-07-07 22:43:59 +0300, Oskari Saarenmaa wrote:
> > Something like the attached patch? It
> > still modifies the toast_delete API, but
> > we could also implement a new
> > speculative-tuple-aware version of it if
> > that's a concern.
>

> Maybe I'm missing something, but toasted columns aren't actually marked
> as speculative right now, no? Cf.
>

> HeapTuple
> toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
> int options)
> ..
> /*
> * Ignore the INSERT_SPECULATIVE option. Speculative insertions/super
> * deletions just normally insert/delete the toast values. It seems
> * easiest to deal with that here, instead on, potentially, multiple
> * callers.
> */
> options &= ~HEAP_INSERT_SPECULATIVE;
>

> so going through heap_abort_speculative()/heap_delete_speculative()
> doesn't look right to me. We could change thing so toasted rows also
> get inserted speculatively - but I don't see much of a point.
>

> > +void
> > +heap_delete_speculative(Relation relation, ItemPointer tid, bool is_toast)
> > +{
>
> If we go this way, it seems easier to get is_toast from
> IsToastRelation(relation), rather than hand it through painstakingly.

Oh, and this desperately needs a minimal test. Shouldn't be hard to do
with isolationtester.

[Peter Geoghegan]

On Wed, Jul 27, 2016 at 5:47 PM, Andres Freund <and...@anarazel.de> wrote:
> Oh, and this desperately needs a minimal test. Shouldn't be hard to do
> with isolationtester.

Uh, really?

--
Peter Geoghegan

[Andres Freund]

On 2016-07-27 17:48:48 -0700, Peter Geoghegan wrote:
> On Wed, Jul 27, 2016 at 5:47 PM, Andres Freund <and...@anarazel.de> wrote:
> > Oh, and this desperately needs a minimal test. Shouldn't be hard to do
> > with isolationtester.
>
> Uh, really?

What really? That it needs a test, or that it's easy to do?

[Peter Geoghegan]

On Wed, Jul 27, 2016 at 6:04 PM, Andres Freund <and...@anarazel.de> wrote:
> That it needs a test, or that it's easy to do?

That it's easy to write one.

--
Peter Geoghegan

[Peter Geoghegan]

On Wed, Jul 27, 2016 at 6:12 PM, Peter Geoghegan <p...@heroku.com> wrote:
> On Wed, Jul 27, 2016 at 6:04 PM, Andres Freund <and...@anarazel.de> wrote:
>> That it needs a test, or that it's easy to do?
>
> That it's easy to write one.

I'll be more concrete: I don't see what choke point is available to
make control yield after the pre-check determines there is no
conflict, but before index tuple insertion determines that there is in
fact a conflict (to reliably trigger a failed specualtive
insertion/super deletion).

[Andres Freund]

On 2016-07-29 17:37:21 -0700, Peter Geoghegan wrote:
> On Wed, Jul 27, 2016 at 6:12 PM, Peter Geoghegan <p...@heroku.com> wrote:
> > On Wed, Jul 27, 2016 at 6:04 PM, Andres Freund <and...@anarazel.de> wrote:
> >> That it needs a test, or that it's easy to do?
> >
> > That it's easy to write one.
>
> I'll be more concrete: I don't see what choke point is available to
> make control yield after the pre-check determines there is no
> conflict, but before index tuple insertion determines that there is in
> fact a conflict (to reliably trigger a failed specualtive
> insertion/super deletion).

An expression index over a function acquiring a lock looks like it
should do the trick.

Are you looking in writing an updated patch? It seems we're on one page
of the rough direction.

Andres

[Peter Geoghegan]

On Sun, Jul 31, 2016 at 3:31 PM, Andres Freund <and...@anarazel.de> wrote:
> An expression index over a function acquiring a lock looks like it
> should do the trick.

Wouldn't even that hinge upon the function knowing if it was called in
the precheck path rather than the insertion path? I think that that
might be possible, but offhand it seems messy.

> Are you looking in writing an updated patch? It seems we're on one page
> of the rough direction.

I could do that. I'll look into whether or not what you describe is
really practicable in the next couple of days. It's not obvious to me
that it is right now, although I do certainly agree that it's worth a
shot.

I think that Oskari's patch was basically solid, concerns already listed aside.

--
Peter Geoghegan

[Oskari Saarenmaa]

01.08.2016, 01:31, Andres Freund kirjoitti:
> On 2016-07-29 17:37:21 -0700, Peter Geoghegan wrote:
>> On Wed, Jul 27, 2016 at 6:12 PM, Peter Geoghegan <p...@heroku.com> wrote:
>>> On Wed, Jul 27, 2016 at 6:04 PM, Andres Freund <and...@anarazel.de> wrote:
>>>> That it needs a test, or that it's easy to do?
>>>
>>> That it's easy to write one.
>>
>> I'll be more concrete: I don't see what choke point is available to
>> make control yield after the pre-check determines there is no
>> conflict, but before index tuple insertion determines that there is in
>> fact a conflict (to reliably trigger a failed specualtive
>> insertion/super deletion).
>

> An expression index over a function acquiring a lock looks like it
> should do the trick.
>

> Are you looking in writing an updated patch? It seems we're on one page
> of the rough direction.

Thanks for the review and the locking index idea for a test case.
Attached a further simplified patch to fix the issue plus an isolation
test case for it.

Cheers,
Oskari