Обсуждение: updateable cursors
Attached is a patch implementing updatable cursors in HEAD. Regression test and documentation are included. Updateable cursors are used as follows: begin; declare foo cursor for select * from bar for update; fetch foo; update bar set abc='def' where current of foo; fetch foo; delete from bar where current of foo; commit; Two points: i) The patch doesn't implement updateable cursors for cursors marked WITH HOLD. I have working code for this and will send it in soon. ii) I've implemented a new snapshot type since, AFAICT, updateable cursors have a type of tuple visibility. Namely, if the base table of a cursor is updated based on that cursor, the new and old tuples are removed from the cursor's set of visible tuples. Like wise, deleted tuples are also removed. Thanks, Gavin
Вложения
Can I get some comments on this? I know the work was completed pre-feature freeze, but submitted only recently. --------------------------------------------------------------------------- Gavin Sherry wrote: > Attached is a patch implementing updatable cursors in HEAD. Regression > test and documentation are included. > > Updateable cursors are used as follows: > > begin; > declare foo cursor for select * from bar for update; > fetch foo; > update bar set abc='def' where current of foo; > fetch foo; > delete from bar where current of foo; > commit; > > > Two points: > > i) The patch doesn't implement updateable cursors for cursors marked WITH > HOLD. I have working code for this and will send it in soon. > > ii) I've implemented a new snapshot type since, AFAICT, updateable cursors > have a type of tuple visibility. Namely, if the base table of a cursor is > updated based on that cursor, the new and old tuples are removed from the > cursor's set of visible tuples. Like wise, deleted tuples are also > removed. > > Thanks, > > Gavin Content-Description: [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 8: explain analyze is your friend -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Can I get some comments on this?
It's far from ready for prime time :-(
The main problem with it is it's doing the wrong things in the wrong
places. The key part of the patch is a hack in parse_expr.c to
transform "WHERE CURRENT OF cursor" into "WHERE ctid = <tid constant>",
where the TID constant has been obtained by reaching into the cursor and
grabbing the current position. Parse analysis is entirely the wrong
time to be doing that --- in any context where the statement will not be
executed immediately, there is no good reason to expect that the cursor
even exists, let alone is positioned at the row it will be positioned at
when you finally execute the query. This approach cannot support WHERE
CURRENT OF in plpgsql functions, rules, or prepared statements.
A lesser but still fatal objection is that the rule reverse-lister would
show the query as "WHERE ctid = <tid constant>" rather than "WHERE
CURRENT OF".
AFAICS, WHERE CURRENT OF has to be a construct that propagates in just
that form all the way into the executor (with a hack in the planner to
ensure that a TidScan plan is chosen) and then the reaching into the
cursor has to happen at plan startup in nodeTidscan.c.
The business with a substitute snapshot seems pretty dubious too. It
looks like a cursor that started with a normal snapshot --- some
transactions visible, some not --- will suddenly flip into SnapshotNow
except for the specific row(s) updated by the current transaction.
That's not gonna do. In the first place, only rows updated *through
that cursor* ought to change visibility, according to my reading of the
spec. In the second place, SnapshotNow will allow visibility of
external transactions that should not have been visible according to the
original snapshot.
I don't understand what the diffs in postgres.c are supposed to
accomplish, but I can just about promise that they are misplaced too,
since they won't affect queries executed via functions or SPI.
regards, tom lane
This is exactly what I worried about, and having it arrive so long after feature freeze, there is no time to recover. I will hold it for 7.5. Thanks for the analysis. Sorry, Gavin. Saved to 7.5 open items queue. --------------------------------------------------------------------------- Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Can I get some comments on this? > > It's far from ready for prime time :-( > > The main problem with it is it's doing the wrong things in the wrong > places. The key part of the patch is a hack in parse_expr.c to > transform "WHERE CURRENT OF cursor" into "WHERE ctid = <tid constant>", > where the TID constant has been obtained by reaching into the cursor and > grabbing the current position. Parse analysis is entirely the wrong > time to be doing that --- in any context where the statement will not be > executed immediately, there is no good reason to expect that the cursor > even exists, let alone is positioned at the row it will be positioned at > when you finally execute the query. This approach cannot support WHERE > CURRENT OF in plpgsql functions, rules, or prepared statements. > > A lesser but still fatal objection is that the rule reverse-lister would > show the query as "WHERE ctid = <tid constant>" rather than "WHERE > CURRENT OF". > > AFAICS, WHERE CURRENT OF has to be a construct that propagates in just > that form all the way into the executor (with a hack in the planner to > ensure that a TidScan plan is chosen) and then the reaching into the > cursor has to happen at plan startup in nodeTidscan.c. > > The business with a substitute snapshot seems pretty dubious too. It > looks like a cursor that started with a normal snapshot --- some > transactions visible, some not --- will suddenly flip into SnapshotNow > except for the specific row(s) updated by the current transaction. > That's not gonna do. In the first place, only rows updated *through > that cursor* ought to change visibility, according to my reading of the > spec. In the second place, SnapshotNow will allow visibility of > external transactions that should not have been visible according to the > original snapshot. > > I don't understand what the diffs in postgres.c are supposed to > accomplish, but I can just about promise that they are misplaced too, > since they won't affect queries executed via functions or SPI. > > regards, tom lane > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
This has been saved for the 7.5 release:
http:/momjian.postgresql.org/cgi-bin/pgpatches2
---------------------------------------------------------------------------
Gavin Sherry wrote:
> Attached is a patch implementing updatable cursors in HEAD. Regression
> test and documentation are included.
>
> Updateable cursors are used as follows:
>
> begin;
> declare foo cursor for select * from bar for update;
> fetch foo;
> update bar set abc='def' where current of foo;
> fetch foo;
> delete from bar where current of foo;
> commit;
>
>
> Two points:
>
> i) The patch doesn't implement updateable cursors for cursors marked WITH
> HOLD. I have working code for this and will send it in soon.
>
> ii) I've implemented a new snapshot type since, AFAICT, updateable cursors
> have a type of tuple visibility. Namely, if the base table of a cursor is
> updated based on that cursor, the new and old tuples are removed from the
> cursor's set of visible tuples. Like wise, deleted tuples are also
> removed.
>
> Thanks,
>
> Gavin
Content-Description:
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > This has been saved for the 7.5 release: > > http:/momjian.postgresql.org/cgi-bin/pgpatches2 > > "Saved for"? It's not gonna be acceptable in its current form for 7.5, > either. I saved the patch plus your comments about it, so we can start on it again for 7.5. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> This has been saved for the 7.5 release:
> http:/momjian.postgresql.org/cgi-bin/pgpatches2
"Saved for"? It's not gonna be acceptable in its current form for 7.5,
either.
regards, tom lane