Thread

1. Re: Remembering bug #6123

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T13:11:23Z

   Tom Lane  wrote:
   > "Kevin Grittner"  writes:
    
   >> Going back through the patches we had to make to 9.0 to move to
   >> PostgreSQL triggers, I noticed that I let the issues raised as bug
   >> #6123 lie untouched during the 9.2 development cycle. In my view,
   >> the best suggestion for a solution was proposed by Florian here:
   > 
   >> http://archives.postgresql.org/pgsql-hackers/2011-08/msg00388.php
   > 
   > Do you mean this:
   > 
   >     After every BEFORE trigger invocation, if the trigger returned
   >     non-NULL, check if latest row version is still the same as when
   >     the trigger started. If not, complain.
    
   That is the consice statement of it, yes.
    
   > While that sounds relatively safe, if possibly performance-
   > impacting, it's not apparent to me how it fixes the problem you
   > complained of.  The triggers you were using were modifying rows
   > other than the one being targeted by the triggering action, so a
   > test like the above would not notice that they'd done anything.
    
   My initial use-case was that a BEFORE DELETE trigger was deleting
   related "child" rows, and the BEFORE DELETE trigger at the child
   level was updating counts on the original (parent) row.  The proposed
   change would cause an error to be thrown when the parent level
   returned a non-NULL value from its BEFORE DELETE trigger.  That would
   prevent the silent corruption of the data, so it's a big step forward
   in my view; but it's not the behavior we most want in our shop for
   this particular case.  In the messages later in the thread, Florian
   pointed out that this pattern would allow us to get the desired
   behavior:
    
   | BEFORE DELETE ON :
   |   DELETE FROM  WHERE parent_id = OLD.id;
   |   IF FOUND THEN
   |     -- Removing children might have modified our row,
   |     -- so returning non-NULL is not an option
   |     DELETE FROM  WHERE id = OLD.id;
   |     RETURN NULL;
   |   ELSE
   |     -- No children removed, so our row should be unmodified
   |     RETURN OLD;
   |   END IF;
    
   The advantage of Florian's approach is that it changes the default
   behavior to something very safe, while allowing arbitrarily complex
   behavior through correspondingly more complex code.
    
   -Kevin
   
   

2. Re: Remembering bug #6123

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-12T15:36:00Z

   "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
   > Tom Lane  wrote:
   >> While that sounds relatively safe, if possibly performance-
   >> impacting, it's not apparent to me how it fixes the problem you
   >> complained of.  The triggers you were using were modifying rows
   >> other than the one being targeted by the triggering action, so a
   >> test like the above would not notice that they'd done anything.
    
   > My initial use-case was that a BEFORE DELETE trigger was deleting
   > related "child" rows, and the BEFORE DELETE trigger at the child
   > level was updating counts on the original (parent) row.  The proposed
   > change would cause an error to be thrown when the parent level
   > returned a non-NULL value from its BEFORE DELETE trigger.
   
   Ah, I see.
   
   I suggest that the current behavior was designed for the case of
   independent concurrent updates, and you have not made a good argument
   for changing that.  What does make sense to me, in light of these
   examples, is to complain if a BEFORE trigger modifies the row "itself"
   (including indirect updates).  IOW, at conclusion of trigger firing
   (I see no need to do it for each trigger), check to see if the target
   row has been outdated *by the current transaction*, and throw error if
   not.
   
   And, if you accept the statement of the fix like that, then actually
   there is no performance hit because there is no need to introduce new
   tests.  All we have to do is start treating HeapTupleSelfUpdated result
   from heap_update or heap_delete as an error case instead of an
   okay-do-nothing case.  There doesn't even need to be an explicit check
   that this was caused by a trigger, because AFAICS there isn't any other
   possibility.
   
   			regards, tom lane
   
   

3. Re: Remembering bug #6123

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T15:42:29Z

   Tom Lane <tgl@sss.pgh.pa.us> wrote:
    
   > I suggest that the current behavior was designed for the case of
   > independent concurrent updates, and you have not made a good
   > argument for changing that.  What does make sense to me, in light
   > of these examples, is to complain if a BEFORE trigger modifies the
   > row "itself" (including indirect updates).  IOW, at conclusion of
   > trigger firing (I see no need to do it for each trigger), check to
   > see if the target row has been outdated *by the current
   > transaction*, and throw error if not.
   > 
   > And, if you accept the statement of the fix like that, then
   > actually there is no performance hit because there is no need to
   > introduce new tests.  All we have to do is start treating
   > HeapTupleSelfUpdated result from heap_update or heap_delete as an
   > error case instead of an okay-do-nothing case.  There doesn't even
   > need to be an explicit check that this was caused by a trigger,
   > because AFAICS there isn't any other possibility.
    
   I think that's pretty much what my previously posted patches did. 
   Here's a slightly modified one, based on Florian's feedback.  Is
   this what you had in mind, or am I misunderstanding?
    
   -Kevin
   
   

4. Re: Remembering bug #6123

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-12T16:06:10Z

   "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
   > Tom Lane <tgl@sss.pgh.pa.us> wrote:
   >> ...  All we have to do is start treating
   >> HeapTupleSelfUpdated result from heap_update or heap_delete as an
   >> error case instead of an okay-do-nothing case.  There doesn't even
   >> need to be an explicit check that this was caused by a trigger,
   >> because AFAICS there isn't any other possibility.
    
   > I think that's pretty much what my previously posted patches did. 
   > Here's a slightly modified one, based on Florian's feedback.  Is
   > this what you had in mind, or am I misunderstanding?
    
   Actually, I was just about to follow up with a comment that that was too
   simplistic: it would break the current behavior of join updates/deletes
   that join to the same target row more than once.  (There might be an
   argument for restricting those, but this discussion isn't about that.)
   So what we need to do is check whether the outdate was done by a later
   CommandId than current.  I see that your patch is attempting to deal
   with these issues by testing GetCurrentCommandId(false) !=
   estate->es_output_cid, but that seems completely wrong to me, as what it
   does is complain if *any* additional command has been executed in the
   transaction, regardless of what changed the target tuple.  It ought to
   be comparing the tuple's xmax to es_output_cid.  And the comment needs
   to cover why it's worrying about that.
   
   Also, what's the point of testing update_ctid?  I don't see that it
   matters whether the outdate was a delete or an update.
   
   			regards, tom lane
   
   

5. Re: Remembering bug #6123

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T16:16:39Z

   Tom Lane <tgl@sss.pgh.pa.us> wrote:
    
   > So what we need to do is check whether the outdate was done by a
   > later CommandId than current.  I see that your patch is attempting
   > to deal with these issues by testing GetCurrentCommandId(false) !=
   > estate->es_output_cid, but that seems completely wrong to me, as
   > what it does is complain if *any* additional command has been
   > executed in the transaction, regardless of what changed the target
   > tuple.  It ought to be comparing the tuple's xmax to
   > es_output_cid.  And the comment needs to cover why it's worrying
   > about that.
    
   OK.  I'll rework based on your comments.
    
   > Also, what's the point of testing update_ctid?  I don't see that
   > it matters whether the outdate was a delete or an update.
    
   The update_ctid code was a carry-over from my old, slightly
   different approach, which I failed to change as I should have.  I'll
   fix that along with the other.
    
   Thanks,
    
   -Kevin
   
   

6. Re: Remembering bug #6123

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-12T16:30:35Z

   "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
   > Tom Lane <tgl@sss.pgh.pa.us> wrote:
   >> Also, what's the point of testing update_ctid?  I don't see that
   >> it matters whether the outdate was a delete or an update.
    
   > The update_ctid code was a carry-over from my old, slightly
   > different approach, which I failed to change as I should have.  I'll
   > fix that along with the other.
   
   Actually, on reflection there might be a reason for checking
   update_ctid, with a view to allowing "harmless" cases.  I see
   these cases:
   
   * UPDATE finds a trigger already updated the row: must throw error
   since we can't apply the update.
   
   * UPDATE finds a trigger already deleted the row: arguably, we could
   let the deletion stand and ignore the update action.
   
   * DELETE finds a trigger already updated the row: must throw error
   since we can't apply the delete.
   
   * DELETE finds a trigger already deleted the row: arguably, there's
   no reason to complain.
   
   Don't know if that was your reasoning as well.  But if it is, then again
   the comment needs to cover that.
   
   			regards, tom lane
   
   

7. Re: Remembering bug #6123

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-12T16:33:26Z

   I wrote:
   >> ... It ought to be comparing the tuple's xmax to
   >> es_output_cid.
   
   Sigh, need to go find some caffeine.  Obviously, it needs to check the
   tuple's cmax, not xmax.  And that means the patch will be a bit more
   invasive than this, because heap_update and heap_delete don't return
   that information at present.
   
   			regards, tom lane
   
   

8. Re: Remembering bug #6123

   Florian G. Pflug <fgp@phlo.org> — 2012-01-12T17:04:28Z

   On Jan12, 2012, at 17:30 , Tom Lane wrote:
   > Actually, on reflection there might be a reason for checking
   > update_ctid, with a view to allowing "harmless" cases.  I see
   > these cases:
   > 
   > * UPDATE finds a trigger already updated the row: must throw error
   > since we can't apply the update.
   > 
   > * UPDATE finds a trigger already deleted the row: arguably, we could
   > let the deletion stand and ignore the update action.
   
   I've argued against that in the past, and I still think it's a bad idea.
   The BEFORE UPDATE trigger might have done some actions which aren't valid
   now that the row has been deleted. If it actually *is* safe to let a
   self-delete take precent, the BEFORE UPDATE trigger ought to check whether
   the row still exists, and return NULL if it doesn't.
   
   > * DELETE finds a trigger already updated the row: must throw error
   > since we can't apply the delete.
   > 
   > * DELETE finds a trigger already deleted the row: arguably, there's
   > no reason to complain.
   
   I'm not convinced that is a a good idea, either. If we do that, there will
   essentially be two BEFORE DELETE trigger invocations for a single deleted
   row, which seems wrong. And again - if a BEFORE DELETE trigger *does* deal
   with this case nicely, it simply has to check whether the row still exists
   before returning non-NULL.
   
   Also, without these exceptions, the behaviour (post-patch) is simply
   explain by
   
     Either don't cause recursive same-row updates from BEFORE trigger,
     or have your trigger return NULL.
   
   and except for the case of multiple BEFORE triggers on the same table
   you can always assume that
   
     A BEFORE trigger's view of a tuple modifications always reflects the
     actual modification that will eventually happen (or an error is
     thrown)
   
   Adding a lists of "buts" and "ifs" to these rules has a higher chance of
   adding confusion and causing bugs than to actually help people, IMHO.
   
   Especially since (judged from the number of years the current behaviour
   as stood undisputed) these cases seems to arise quite infrequently in
   practice.
   
   best regards,
   Florian Pflug
   
   
   

9. Re: Remembering bug #6123

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-12T18:32:47Z

   Florian Pflug <fgp@phlo.org> writes:
   > On Jan12, 2012, at 17:30 , Tom Lane wrote:
   >> Actually, on reflection there might be a reason for checking
   >> update_ctid, with a view to allowing "harmless" cases.
   
   > I've argued against that in the past, and I still think it's a bad idea.
   
   Well, the main argument for it in my mind is that we could avoid
   breaking existing behavior in cases where it's not clearly essential
   to do so.  Perhaps that's less compelling than keeping the behavior
   simple, since it's true that the specific cases we could preserve the old
   behavior in are pretty infrequent.  I'm not wedded to the idea, but
   would like to hear a few other peoples' opinions.
   
   			regards, tom lane
   
   

10. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T19:23:53Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
    > Florian Pflug <fgp@phlo.org> writes:
    >> On Jan12, 2012, at 17:30 , Tom Lane wrote:
    >>> Actually, on reflection there might be a reason for checking
    >>> update_ctid, with a view to allowing "harmless" cases.
    > 
    >> I've argued against that in the past, and I still think it's a
    >> bad idea.
    > 
    > Well, the main argument for it in my mind is that we could avoid
    > breaking existing behavior in cases where it's not clearly
    > essential to do so.  Perhaps that's less compelling than keeping
    > the behavior simple, since it's true that the specific cases we
    > could preserve the old behavior in are pretty infrequent.  I'm not
    > wedded to the idea, but would like to hear a few other peoples'
    > opinions.
     
    FWIW, I started from the position that the "harmless" cases should
    be quietly handled.  But Florian made what, to me, were persuasive
    arguments against that.  A DELETE trigger might fire twice for one
    delete, mucking up data integrity, for example.  His proposal seemed
    to make my use case hard to handle, but then he pointed out how
    triggers could be coded to allow that -- it's just that you would
    need to go out of your way to do so, reducing the chance of falling
    into bad behavior accidentally.
     
    So count me as a convert to Florian's POV, even if I didn't succeed
    in ripping out all the code from my former POV from the v3 patch.
     
    -Kevin
    
    

11. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T20:24:55Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
     
    > it needs to check the tuple's cmax [...]  And that means the patch
    > will be a bit more invasive than this, because heap_update and
    > heap_delete don't return that information at present.
     
    I'm thinking that I could keep the test for:
     
      GetCurrentCommandId(false) != estate->es_output_cid
     
    as a "first pass".  If that's true, I could use EvalPlanQualFetch()
    to find the last version of the tuple, and generate the error if the
    tuple's cmax != estate->es_output_cid.  I think, although I'm not
    entirely sure, that EvalPlanQualFetch needs a little work to support
    this usage.
     
    Attached is a patch based on these thoughts.  Is it on the right
    track?  I suspect I haven't got everything covered, but thought a
    reality check was in order at this point.
     
    It does pass regression tests, including the new ones I added.
     
    -Kevin
    
    

12. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T20:29:13Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
     
    > Attached is a patch based on these thoughts.
     
    OK, really attached this time.
     
    -Kevin
    
    

13. Re: Remembering bug #6123

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-12T20:31:37Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    > Tom Lane <tgl@sss.pgh.pa.us> wrote:
    >> it needs to check the tuple's cmax [...]  And that means the patch
    >> will be a bit more invasive than this, because heap_update and
    >> heap_delete don't return that information at present.
     
    > I'm thinking that I could keep the test for:
     
    >   GetCurrentCommandId(false) != estate->es_output_cid
     
    > as a "first pass".  If that's true, I could use EvalPlanQualFetch()
    > to find the last version of the tuple, and generate the error if the
    > tuple's cmax != estate->es_output_cid.  I think, although I'm not
    > entirely sure, that EvalPlanQualFetch needs a little work to support
    > this usage.
     
    > Attached is a patch based on these thoughts.  Is it on the right
    > track?  I suspect I haven't got everything covered, but thought a
    > reality check was in order at this point.
    
    You forgot to attach the patch, but the approach seems totally Rube
    Goldberg to me anyway.  Why not just fix heap_update/heap_delete to
    return the additional information?  It's not like we don't whack their
    parameter lists around regularly.
    
    Rather than having three output parameters to support the case, I'm
    a bit inclined to merge them into a single-purpose struct type.
    But that's mostly cosmetic.
    
    			regards, tom lane
    
    

14. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T20:36:02Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
     
    > You forgot to attach the patch, but the approach seems totally
    > Rube Goldberg to me anyway.  Why not just fix heap_update/
    > heap_delete to return the additional information?  It's not like
    > we don't whack their parameter lists around regularly.
     
    OK.
     
    > Rather than having three output parameters to support the case,
    > I'm a bit inclined to merge them into a single-purpose struct
    > type.  But that's mostly cosmetic.
     
    OK.
     
    Working on v5.
     
    -Kevin
    
    

15. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T22:31:51Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
     
    > You forgot to attach the patch, but the approach seems totally
    > Rube Goldberg to me anyway.  Why not just fix heap_update/
    > heap_delete to return the additional information?  It's not like
    > we don't whack their parameter lists around regularly.
    > 
    > Rather than having three output parameters to support the case,
    > I'm a bit inclined to merge them into a single-purpose struct
    > type.  But that's mostly cosmetic.
     
    OK, I got rid of the parrots and candles and added a structure to
    hold the data returned only on failure.
     
    Am I getting closer?
     
    -Kevin
    
    

16. Re: Remembering bug #6123

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-12T22:44:16Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    > OK, I got rid of the parrots and candles and added a structure to
    > hold the data returned only on failure.
     
    > Am I getting closer?
    
    Hmm, I would think you'd get assertion failures from calling
    HeapTupleHeaderGetCmax when xmax isn't the current transaction.
    (But I'm not sure that the regression tests really exercise such
    cases ... did you try the isolation tests with this?)  I was thinking
    we should probably define the cmax as being returned only in SelfUpdated
    cases.
    
    You failed to update assorted relevant comments, too.  But I can take
    it from here.
    
    			regards, tom lane
    
    

17. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T22:51:04Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
    > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
     
    >> Am I getting closer?
    > 
    > Hmm, I would think you'd get assertion failures from calling
    > HeapTupleHeaderGetCmax when xmax isn't the current transaction.
    > (But I'm not sure that the regression tests really exercise such
    > cases ... did you try the isolation tests with this?)
     
    I didn't go farther than a `make check`, but I'm doing a more
    thorough set of tests now.
     
    > I was thinking we should probably define the cmax as being
    > returned only in SelfUpdated cases.
     
    I thought about that, but didn't see how it could be other than
    self-updated.  If you do, I guess I missed something.
     
    > You failed to update assorted relevant comments, too.  But I can
    > take it from here.
     
    I can take another pass to polish it if you'd like.  I didn't expect
    this to be the last version I posted; I was afraid I might still
    have some restructuring to do.
     
    -Kevin
    
    

18. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T22:53:23Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
    >>  I was thinking we should probably define the cmax as being
    >> returned only in SelfUpdated cases.
    >  
    > I thought about that, but didn't see how it could be other than
    > self-updated.  If you do, I guess I missed something.
     
    Oh, I see it now.  Oops.
     
    I can fix that and the comments if you like.
     
    -Kevin
    
    

19. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-12T23:29:44Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
     
    > Hmm, I would think you'd get assertion failures from calling
    > HeapTupleHeaderGetCmax when xmax isn't the current transaction.
    > (But I'm not sure that the regression tests really exercise such
    > cases ... did you try the isolation tests with this?)  I was
    > thinking we should probably define the cmax as being returned only
    > in SelfUpdated cases.
     
    You were right.  One of the isolation tests failed an assertion;
    things pass with the attached change, setting the cmax
    conditionally.  Some comments updated.  I hope this is helpful.  I
    can't take more time right now, because we're getting major snowfall
    and I've got to leave now to get home safely.
     
    -Kevin
    
    

20. Re: Remembering bug #6123

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-13T17:32:28Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    > You were right.  One of the isolation tests failed an assertion;
    > things pass with the attached change, setting the cmax
    > conditionally.  Some comments updated.  I hope this is helpful.
    
    I worked over this patch a bit, mostly cosmetically; updated version
    attached.  However, we're not there yet.  I have a couple of remaining
    concerns:
    
    1. I think the error message needs more thought.  I believe it's
    possible to trigger this error not only with BEFORE triggers, but also
    with a volatile function used in the query that reaches around and
    updates row(s) of the target table.  Now, I don't have the slightest
    qualms about breaking any apps that do anything so dirty, but should
    we try to generalize the message text to cope with that?
    
    2. The HINT needs work too, as it seems pretty useless as it stands.
    I'd have expected some short reference to the technique of re-executing
    the update in the trigger and then returning NULL.  (BTW, does this
    patch require any documentation changes, and if so where?)
    
    3. I modified heap_lock_tuple to also return a HeapUpdateFailureData,
    principally on the grounds that its API should be largely parallel to
    heap_update, but having done that I can't help wondering whether its
    callers are okay with skipping self-updated tuples.  I see that you
    changed EvalPlanQualFetch, but I'm not entirely sure that that is right;
    shouldn't it continue to ignore rows modified by the current command
    itself?  And you did not touch the other two callers, which definitely
    have got issues.  Here is an example, which is basically the reference
    count case refactored into a single self-referencing table, so that we
    can hit both parent and child rows in one operation:
    
    create table test (
        id int primary key,
        parent int references test,
        data text,
        nchildren int not null default 0
    );
    
    create function test_ins_func()
      returns trigger language plpgsql as
    $$
    begin
      if new.parent is not null then
        update test set nchildren = nchildren + 1 where id = new.parent;
      end if;
      return new;
    end;
    $$;
    create trigger test_ins_trig before insert on test
      for each row execute procedure test_ins_func();
    
    create function test_del_func()
      returns trigger language plpgsql as
    $$
    begin
      if old.parent is not null then
        update test set nchildren = nchildren - 1 where id = old.parent;
      end if;
      return old;
    end;
    $$;
    create trigger test_del_trig before delete on test
      for each row execute procedure test_del_func();
    
    insert into test values (1, null, 'root');
    insert into test values (2, 1, 'root child A');
    insert into test values (3, 1, 'root child B');
    insert into test values (4, 2, 'grandchild 1');
    insert into test values (5, 3, 'grandchild 2');
    
    update test set data = 'root!' where id = 1;
    
    select * from test;
    
    delete from test;
    
    select * from test;
    
    drop table test;
    drop function test_ins_func();
    drop function test_del_func();
    
    When you run this, with or without the current patch, you get:
    
     id | parent |     data     | nchildren 
    ----+--------+--------------+-----------
      2 |      1 | root child A |         1
      4 |      2 | grandchild 1 |         0
      3 |      1 | root child B |         1
      5 |      3 | grandchild 2 |         0
      1 |        | root!        |         2
    (5 rows)
    
    DELETE 4
     id | parent | data  | nchildren 
    ----+--------+-------+-----------
      1 |        | root! |         0
    (1 row)
    
    the reason being that when the outer delete arrives at the last (root!)
    row, GetTupleForTrigger fails because the tuple is already self-updated,
    and we treat that as canceling the outer delete action.
    
    I'm not sure what to do about this.  If we throw an error there, there
    will be no way that the trigger can override the error because it will
    never get to run.  Possibly we could plow ahead with the expectation
    of throwing an error later if the trigger doesn't cancel the
    update/delete, but is it safe to do so if we don't hold lock on the
    tuple?  In any case that idea doesn't help with the remaining caller,
    ExecLockRows.
    
    			regards, tom lane
    
    

21. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-13T18:59:03Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
     
    > I worked over this patch a bit, mostly cosmetically; updated
    > version attached.
     
    Thanks!
     
    > However, we're not there yet.  I have a couple of remaining
    > concerns:
    > 
    > 1. I think the error message needs more thought.  I believe it's
    > possible to trigger this error not only with BEFORE triggers, but
    > also with a volatile function used in the query that reaches
    > around and updates row(s) of the target table.  Now, I don't have
    > the slightest qualms about breaking any apps that do anything so
    > dirty, but should we try to generalize the message text to cope
    > with that?
     
    I hadn't thought of that, but it does seem possible.  Maybe after
    dealing with the other points, I'll work on a test case to show that
    behavior.
     
    I'm also fine with generating an error for such dirty tricks, and I
    agree that if that's indeed possible we should make the message
    general enough to cover that case.  Nothing comes to mind at the
    moment, but I'll think on it.
     
    > 2. The HINT needs work too, as it seems pretty useless as it
    > stands.  I'd have expected some short reference to the technique
    > of re-executing the update in the trigger and then returning NULL.
     
    In the previous (rather long) thread on the topic, it seemed that
    most cases where people have hit this, the problem was easily solved
    by moving the offending code to the AFTER trigger.  The technique of
    re-issuing the command didn't turn up until much later.  I would bet
    it will be the right thing to do 20% of the time when people get
    such an error.  I don't want to leave the 80% solution out of the
    hint, but if you don't think it's too verbose, I guess it would be a
    good thing to mention the 20% solution, too.
     
    > (BTW, does this patch require any documentation changes, and if so
    > where?)
     
    I've been wondering that.  Perhaps a paragraph or two with an
    example on this page?:
     
    http://www.postgresql.org/docs/devel/static/trigger-definition.html
     
    > 3. I modified heap_lock_tuple to also return a
    > HeapUpdateFailureData, principally on the grounds that its API
    > should be largely parallel to heap_update, but having done that I
    > can't help wondering whether its callers are okay with skipping
    > self-updated tuples.  I see that you changed EvalPlanQualFetch,
    > but I'm not entirely sure that that is right; shouldn't it
    > continue to ignore rows modified by the current command itself?
     
    I made that change when working on the approach where "safe"
    conflicts (like a DELETE from within the BEFORE DELETE trigger) were
    quietly ignored.  Without that change, it didn't see the end of the
    ctid chain, and didn't behave correctly.  I've been wondering if it
    is still needed.  I had been planning to create a test case to try
    to show that it was.  Maybe an UPDATE with a join to force multiple
    row updates from the "primary" statement, followed by a triggered
    UPDATE to the row.  If that doesn't need the EvalPlanQualFetch
    change, I don't know what would.
     
    > And you did not touch the other two callers, which definitely
    > have got issues.  Here is an example
     
    > [example]
     
    > I'm not sure what to do about this.  If we throw an error there,
    > there will be no way that the trigger can override the error
    > because it will never get to run.  Possibly we could plow ahead
    > with the expectation of throwing an error later if the trigger
    > doesn't cancel the update/delete, but is it safe to do so if we
    > don't hold lock on the tuple?  In any case that idea doesn't help
    > with the remaining caller, ExecLockRows.
     
    It will take me some time to work though the example and review the
    relevant code.
     
    -Kevin
    
    

22. Re: Remembering bug #6123

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-13T19:13:21Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    > Tom Lane <tgl@sss.pgh.pa.us> wrote:
    >> 3. I modified heap_lock_tuple to also return a
    >> HeapUpdateFailureData, principally on the grounds that its API
    >> should be largely parallel to heap_update, but having done that I
    >> can't help wondering whether its callers are okay with skipping
    >> self-updated tuples.  I see that you changed EvalPlanQualFetch,
    >> but I'm not entirely sure that that is right; shouldn't it
    >> continue to ignore rows modified by the current command itself?
     
    > I made that change when working on the approach where "safe"
    > conflicts (like a DELETE from within the BEFORE DELETE trigger) were
    > quietly ignored.  Without that change, it didn't see the end of the
    > ctid chain, and didn't behave correctly.  I've been wondering if it
    > is still needed.  I had been planning to create a test case to try
    > to show that it was.  Maybe an UPDATE with a join to force multiple
    > row updates from the "primary" statement, followed by a triggered
    > UPDATE to the row.  If that doesn't need the EvalPlanQualFetch
    > change, I don't know what would.
    
    The EvalPlanQual code isn't really exercised by any existing regression
    tests, because it is only triggered when two sessions concurrently
    update the same row, something that we avoid for reproducibility's sake
    in the regression tests.  I think we could now test it using the
    isolation test scaffolding, and maybe it would be a good idea to add
    some tests there.  I did verify that whether that part of your patch
    is included or not makes no difference to any existing test.
    
    			regards, tom lane
    
    

23. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-13T21:15:11Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
     
    [rearranging so results directly follow statements]
     
    > select * from test;
    >  id | parent |     data     | nchildren 
    > ----+--------+--------------+-----------
    >   2 |      1 | root child A |         1
    >   4 |      2 | grandchild 1 |         0
    >   3 |      1 | root child B |         1
    >   5 |      3 | grandchild 2 |         0
    >   1 |        | root!        |         2
    > (5 rows)
     
    > delete from test;
    > DELETE 4
     
    > select * from test;
    >  id | parent | data  | nchildren 
    > ----+--------+-------+-----------
    >   1 |        | root! |         0
    > (1 row)
     
    And other minor updates to the data column can result in totally
    different sets of rows left over after a DELETE from the table with
    no WHERE clause.  It makes me pretty queasy whenever the semantics
    of a statement can depend on the order of tuples in the heap.  It's
    pretty hard to view this particular case as anything other than a
    bug.
     
    > the reason being that when the outer delete arrives at the last
    > (root!) row, GetTupleForTrigger fails because the tuple is already
    > self-updated, and we treat that as canceling the outer delete
    > action.
    > 
    > I'm not sure what to do about this.  If we throw an error there,
    > there will be no way that the trigger can override the error
    > because it will never get to run.  Possibly we could plow ahead
    > with the expectation of throwing an error later if the trigger
    > doesn't cancel the update/delete, but is it safe to do so if we
    > don't hold lock on the tuple?  In any case that idea doesn't help
    > with the remaining caller, ExecLockRows.
     
    I'm still trying to sort through what could be done at the source
    code level, but from a user level I would much rather see an error
    than such surprising and unpredictable behavior.
     
    -Kevin
    
    

24. Re: Remembering bug #6123

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-13T21:29:11Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    > Tom Lane <tgl@sss.pgh.pa.us> wrote:
    >> I'm not sure what to do about this.  If we throw an error there,
    >> there will be no way that the trigger can override the error
    >> because it will never get to run.  Possibly we could plow ahead
    >> with the expectation of throwing an error later if the trigger
    >> doesn't cancel the update/delete, but is it safe to do so if we
    >> don't hold lock on the tuple?  In any case that idea doesn't help
    >> with the remaining caller, ExecLockRows.
     
    > I'm still trying to sort through what could be done at the source
    > code level, but from a user level I would much rather see an error
    > than such surprising and unpredictable behavior.
    
    I don't object to throwing an error by default.  What I'm wondering is
    what would have to be done to make such updates work safely.  AFAICT,
    throwing an error in GetTupleForTrigger would preclude any chance of
    coding a trigger to make this work, which IMO greatly weakens the
    argument that this whole approach is acceptable.
    
    In this particular example, I think it would work just as well to do the
    reference-count updates in AFTER triggers, and maybe the short answer
    is to tell people they have to do it like that instead of in BEFORE
    triggers.  However, I wonder what use-case led you to file bug #6123 to
    begin with.
    
    			regards, tom lane
    
    

25. Re: Remembering bug #6123

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-13T21:41:37Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    > I'm also fine with generating an error for such dirty tricks, and I
    > agree that if that's indeed possible we should make the message
    > general enough to cover that case.  Nothing comes to mind at the
    > moment, but I'll think on it.
    
    What do you think of
    
    ERROR: tuple to be updated was already modified by an operation triggered by the UPDATE command
    HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.
    
    (s/update/delete/ for the DELETE case of course)
    
    The phrase "triggered by" seems slippery enough to cover cases such as a
    volatile function executed by the UPDATE.  The HINT doesn't cover that
    case of course, but we have a ground rule that HINTs can be wrong.
    
    			regards, tom lane
    
    

26. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-13T21:44:51Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
     
    > What do you think of
    > 
    > ERROR: tuple to be updated was already modified by an operation
    > triggered by the UPDATE command
    > HINT: Consider using an AFTER trigger instead of a BEFORE trigger
    > to propagate changes to other rows.
    > 
    > (s/update/delete/ for the DELETE case of course)
    > 
    > The phrase "triggered by" seems slippery enough to cover cases
    > such as a volatile function executed by the UPDATE.  The HINT
    > doesn't cover that case of course, but we have a ground rule that
    > HINTs can be wrong.
     
    Looks good to me.
     
    -Kevin
    
    

27. Re: Remembering bug #6123

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-01-13T23:18:50Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
     
    > In this particular example, I think it would work just as well to
    > do the reference-count updates in AFTER triggers, and maybe the
    > short answer is to tell people they have to do it like that
    > instead of in BEFORE triggers.
     
    I think that is quite often the right answer.
     
    > However, I wonder what use-case led you to file bug #6123 to begin
    > with.
     
    In our Circuit Court software, we have about 1600 trigger functions
    on about 400 tables, and this summer we converted them from our Java
    middle tier framework to native PostgreSQL triggers.  Since we had
    been writing them in our interpretation of ANSI SQL trigger code,
    parsing that, and using the parse tree to build a Java class for
    each trigger, we were able to generate the PostgreSQL trigger
    functions and CREATE TRIGGER statement mechanically (from the parse
    tree), with pretty good success.  In testing, though, our business
    analysts noticed a number of situations where an attempt to delete a
    row actually deleted related rows which should have gone away with
    the row they were directly trying to delete, but the target row was
    still there.  A few days of investigation, including stepping
    through query execution in gdb, turned up this issue.
     
    Having identified (at least one flavor of) the problem, we grepped
    the source code for the BEFORE triggers for UPDATE and DELETE
    statements, and were able to fix a number of them by moving code to
    AFTER triggers or setting values into NEW fields rather than running
    an UPDATE.  So far, so good.
     
    But there were a number of situations where the DELETE of a row
    needed to cause related rows in other tables to be deleted, and for
    one reason or another a foreign key with ON DELETE CASCADE was not
    an option.  At the same time, triggers on some of those related
    tables needed to update summary or redundant data in other tables
    for performance reasons.  Because a number of tables could be
    involved, and some of the triggers (at the "lower" levels) could be
    AFTER triggers and still contribute to the problem, (1) we had no
    reliable way to ensure we would find all of the cases of this on all
    code paths, and (2) due to referential integrity and other trigger-
    based validations, it would be hard to restructure such that the
    DELETE of the "child" rows was not done on the BEFORE DELETE trigger
    of the "parent".
     
    The patch we've been using in production throws errors if the row
    for a BEFORE UPDATE trigger is updated by another statement.  (Well,
    OK, you showed me that it really is throwing an error if the row is
    updated and there has been another statement executed, but as long
    as it is *more* strict than we actually need, we won't corrupt data
    -- and the current rule hasn't been hard for us to live with.)  It
    allows the DELETE to proceed if the tuple is updated from within the
    BEFORE DELETE trigger.  We would need to tweak some triggers to move
    to the approach embodied in the recent patch drafts, but the IF
    FOUND logic suggested by Florian looks like it will cover all of our
    use cases, and they should be fairly easy to find with grep.
     
    Hopefully this answers your question.  I went looking for details on
    particular failures here, but didn't have luck with so far.  I can
    try again if more detail like that would help.
     
    -Kevin
    
    

28. Re: Remembering bug #6123

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-14T00:17:43Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    > Tom Lane <tgl@sss.pgh.pa.us> wrote:
    >> However, I wonder what use-case led you to file bug #6123 to begin
    >> with.
    
    > [ details ]
     
    > Hopefully this answers your question.  I went looking for details on
    > particular failures here, but didn't have luck with so far.  I can
    > try again if more detail like that would help.
    
    Well, the bottom line that's concerning me here is whether throwing
    errors is going to push anyone's application into an unfixable corner.
    I'm somewhat encouraged that your Circuit Courts software can adapt
    to it, since that's certainly one of the larger and more complex
    applications out there.  Or at least I would be if you had actually
    verified that the CC code was okay with the recently-proposed patch
    versions.  Do you have any thorough tests you can run against whatever
    we end up with?
    
    			regards, tom lane
    
    

29. Re: Remembering bug #6123

    Simon Riggs <simon@2ndquadrant.com> — 2012-01-30T09:45:14Z

    On Thu, Jan 12, 2012 at 4:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    >> Tom Lane <tgl@sss.pgh.pa.us> wrote:
    >>> Also, what's the point of testing update_ctid?  I don't see that
    >>> it matters whether the outdate was a delete or an update.
    >
    >> The update_ctid code was a carry-over from my old, slightly
    >> different approach, which I failed to change as I should have.  I'll
    >> fix that along with the other.
    >
    > Actually, on reflection there might be a reason for checking
    > update_ctid, with a view to allowing "harmless" cases.  I see
    > these cases:
    >
    > * UPDATE finds a trigger already updated the row: must throw error
    > since we can't apply the update.
    >
    > * UPDATE finds a trigger already deleted the row: arguably, we could
    > let the deletion stand and ignore the update action.
    >
    > * DELETE finds a trigger already updated the row: must throw error
    > since we can't apply the delete.
    >
    > * DELETE finds a trigger already deleted the row: arguably, there's
    > no reason to complain.
    >
    > Don't know if that was your reasoning as well.  But if it is, then again
    > the comment needs to cover that.
    
    
    Just been reading this thread a little.
    
    ISTM that seeing a SelfUpdated row on a cursor when we didn't use
    WHERE CURRENT OF really ought to raise some kind of exception
    condition like a WARNING or a NOTICE. That situation seems most likely
    to be a bug or at least an unintended consequence.
    
    As Tom points out, the multiple flavours of weirdness that can result
    even if we do fix this are not things we should do silently. I think
    we should do the best job we can without throwing an error, but we
    must make some attempt to let the developer know we did that for them
    so they can investigate.
    
    -- 
     Simon Riggs                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services