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
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
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
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
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
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
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
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;
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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,
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
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
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
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
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:
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
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
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
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
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
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
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.
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
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:
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
39 matches
Mail list logo