Re: [HACKERS] Remembering bug #6123

2012-06-05 Thread Kevin Grittner
Tom Lane t...@sss.pgh.pa.us wrote: http://archives.postgresql.org/pgsql-hackers/2012-01/msg01241.php OK, here's an updated version of the patch. I was on vacation after PGCon and just got back to the office yesterday, so I have just now been able to check on the status of our testing of this

Re: [HACKERS] Remembering bug #6123

2012-01-30 Thread Simon Riggs
On Thu, Jan 12, 2012 at 4:30 PM, Tom Lane t...@sss.pgh.pa.us wrote: Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-22 Thread Tom Lane
OK, here's an updated version of the patch. I changed the error message texts as per discussion (except I decided to use one message string for all the cases rather than saddle translators with several variants). Also, I put in an error in GetTupleForTrigger, which fixes the self-reference case I

Re: [HACKERS] Remembering bug #6123

2012-01-20 Thread Kevin Grittner
Kevin Grittner kevin.gritt...@wicourts.gov wrote: Tom Lane wrote: 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

Re: [HACKERS] Remembering bug #6123

2012-01-20 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes: After a couple meetings, I have approval to get this into an application release currently in development. Assuming that your patch from the 13th is good for doing the testing, I think I can post test results in about three weeks. I'll also

Re: [HACKERS] Remembering bug #6123

2012-01-20 Thread Kevin Grittner
Tom Lane t...@sss.pgh.pa.us wrote: I'm up to my elbows in planner guts at the moment, but will try to fix up the patch this weekend if you want. They have scheduled testers to check on this issue next week, so it would be great to get as close as we can on the stuff that matters. -Kevin

Re: [HACKERS] Remembering bug #6123

2012-01-14 Thread Kevin Grittner
Tom Lane wrote: 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

Re: [HACKERS] Remembering bug #6123

2012-01-13 Thread Tom Lane
Kevin Grittner kevin.gritt...@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;

Re: [HACKERS] Remembering bug #6123

2012-01-13 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-13 Thread Kevin Grittner
Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-13 Thread Kevin Grittner
Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-13 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-13 Thread Tom Lane
Kevin Grittner kevin.gritt...@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

Re: [HACKERS] Remembering bug #6123

2012-01-13 Thread Kevin Grittner
Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-13 Thread Kevin Grittner
Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-13 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Tom Lane
Kevin Grittner kevin.gritt...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Florian Pflug
On Jan12, 2012, at 00:32 , Kevin Grittner wrote: 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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Tom Lane
Florian Pflug f...@phlo.org writes: So, I guess my question is, if we add safeguards against these sorts of bugs for triggers, should we also add them to FOR UPDATE? Historically, we seem to have taken the stand that modifications of self-updated tuples should be ignored. If we're going to

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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,

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Tom Lane
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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Florian Pflug
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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Tom Lane
Florian Pflug f...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Tom Lane t...@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:

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Kevin Grittner kevin.gritt...@wicourts.gov wrote: Attached is a patch based on these thoughts. OK, really attached this time. -Kevin *** a/src/backend/executor/execMain.c --- b/src/backend/executor/execMain.c *** *** 1921,1928 EvalPlanQualFetch(EState *estate, Relation

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Tom Lane t...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Tom Lane
Kevin Grittner kevin.gritt...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Tom Lane t...@sss.pgh.pa.us wrote: Kevin Grittner kevin.gritt...@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

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Kevin Grittner kevin.gritt...@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.

Re: [HACKERS] Remembering bug #6123

2012-01-12 Thread Kevin Grittner
Tom Lane t...@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

[HACKERS] Remembering bug #6123

2012-01-11 Thread Kevin Grittner
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:

Re: [HACKERS] Remembering bug #6123

2012-01-11 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov 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