Re: Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

On 2012-11-30 13:57:46 +0100, Andres Freund wrote:
> On 2012-11-30 12:50:06 +0000, Simon Riggs wrote:
> > On 30 November 2012 11:58, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >
> > > We only get the pin right there, I don't see any preexisting pin.
> >
> > Seems easy enough to test with an Assert patch.
> >
> > If the Assert doesn't fail, we apply it as "documentation" of the
> > requirement for a pin.
> >
> > If it fails, we fix the bug.
>
> I think its wrong even if we were holding a pin all the time due the the
> aforementioned PageAddItem reshuffling of line pointers. So that Assert
> wouldn't proof enough.

But a failing Assert obviously proofs something. Stupid me. So here we
go:

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 98b8207..3b61d06 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2662,6 +2662,16 @@ ltrmark:;

buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(tid));

+#ifdef USE_ASSERT_CHECKING
+ if (!BufferIsLocal(buffer))
+ {
+ /* Only pinned by the above ReadBuffer */
+ if (PrivateRefCount[buffer - 1] <= 1)
+ elog(ERROR, "too low local pin count: %d",
+ PrivateRefCount[buffer - 1]);
+ }
+#endif
+
page = BufferGetPage(buffer);
lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid));

CREATE OR REPLACE FUNCTION raise_notice_id()
RETURNS trigger
LANGUAGE plpgsql AS $body$
BEGIN
RAISE NOTICE 'id: %', OLD.id;
RETURN NULL;
END
$body$;

postgres=#
CREATE TABLE crashdummy(id serial primary key, data int);

postgres=#
CREATE TRIGGER crashdummy_after_delete
AFTER DELETE ON crashdummy
FOR EACH ROW EXECUTE PROCEDURE raise_notice_id();

postgres=#
INSERT INTO crashdummy(data) SELECT * FROM generate_series(1, 1000);

postgres=#
DELETE FROM crashdummy WHERE ctid IN (SELECT ctid FROM crashdummy WHERE data < 1000);
ERROR: too low local pin count: 1
Time: 4.515 ms

A plain DELETE without the subselect doesn't trigger the problem though,
thats probably the reason we haven't seen any problems so far.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services