Tom Lane 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 after being asked
On Thu, Jan 12, 2012 at 4:30 PM, Tom Lane wrote:
> "Kevin Grittner" writes:
>> Tom Lane 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
>> di
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
Tom Lane 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
--
Sent via pgsql
"Kevin Grittner" 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 work on a
> follow-on
"Kevin Grittner" 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 that's certainly one
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
> compl
"Kevin Grittner" writes:
> Tom Lane 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 mor
Tom Lane 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.
>
Tom Lane 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 co
"Kevin Grittner" 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
"Kevin Grittner" writes:
> Tom Lane 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 t
Tom Lane 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 |
Tom Lane 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
"Kevin Grittner" writes:
> Tom Lane 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
>>
"Kevin Grittner" 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. Ho
Tom Lane 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
"Kevin Grittner" 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 an
Tom Lane wrote:
> "Kevin Grittner" 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 i
"Kevin Grittner" 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
Tom Lane 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
Tom Lane 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 t
"Kevin Grittner" writes:
> Tom Lane 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:
> GetCur
"Kevin Grittner" 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 relation, int
lockmode,
Tom Lane 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->e
Tom Lane wrote:
> Florian Pflug 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.
>
>
Florian Pflug 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 i
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.
>
> *
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 infor
"Kevin Grittner" writes:
> Tom Lane 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 ha
Florian Pflug 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 reverse that
Tom Lane 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 i
"Kevin Grittner" writes:
> Tom Lane 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,
>> b
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 propo
Tom Lane 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
"Kevin Grittner" 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 ac
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 prop
"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:
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/pgs
39 matches
Mail list logo
