Thread: RESET SESSION v3
Changes in v3: * DEALLOCATE PREPARE ALL * RESET PLANS wont check for ACL_CREATE_TEMP anymore. * Add prepare.h and portal.h to guc.c. On 4/7/07, Neil Conway <neilc@samurai.com> wrote: > > * RESET SESSION does not ABORT anymore, instead fails if in transaction. > I think it's quite bizarre to have RESET SESSION fail if used in a > transaction, but to allow an equivalent sequence of commands to be > executed by hand inside a transaction. I think implicit ABORT would annoy various tools that partially parse user sql and expect to know what transaction state currently is. For them a new tranaction control statement would be nuisance. > guc.c is missing some #includes. For some reason gcc4.0 did not show any warnings by default. > > * DEALLOCATE PREPARE ALL gives bison conflicts. Is that even needed? > > Seems best to have it, for the sake of consistency. The shift/reduce > conflict is easy to workaround, provided you're content to duplicate the > body of the DEALLOCATE ALL rule -- e.g. see the attached incremental > diff. Thanks, included. > > * Are the CommandComplete changes needed? > > Seems warranted to me. BTW, why is CLOSE's command tag "CLOSE CURSOR", > not "CLOSE"? That seems needlessly verbose, and inconsistent with other > commands (e.g. DEALLOCATE). Because the regular tag is "CLOSE CURSOR". I did not want to break any expectations. But yes, the inconsistency is weird. > > * ResetPlanCache() is implemented as PlanCacheCallback((Datum)0, > > InvalidOid); That seems to leave plans for utility commands untouched. > > Is it problem? > > Yes, I'd think you'd also want to cleanup plans for utility commands. Tom thought otherwise, so I kept the old way. -- marko
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Marko Kreen wrote:
> Changes in v3:
>
> * DEALLOCATE PREPARE ALL
> * RESET PLANS wont check for ACL_CREATE_TEMP anymore.
> * Add prepare.h and portal.h to guc.c.
>
>
> On 4/7/07, Neil Conway <neilc@samurai.com> wrote:
> > > * RESET SESSION does not ABORT anymore, instead fails if in transaction.
>
> > I think it's quite bizarre to have RESET SESSION fail if used in a
> > transaction, but to allow an equivalent sequence of commands to be
> > executed by hand inside a transaction.
>
> I think implicit ABORT would annoy various tools that
> partially parse user sql and expect to know what transaction
> state currently is. For them a new tranaction control statement
> would be nuisance.
>
> > guc.c is missing some #includes.
>
> For some reason gcc4.0 did not show any warnings by default.
>
> > > * DEALLOCATE PREPARE ALL gives bison conflicts. Is that even needed?
> >
> > Seems best to have it, for the sake of consistency. The shift/reduce
> > conflict is easy to workaround, provided you're content to duplicate the
> > body of the DEALLOCATE ALL rule -- e.g. see the attached incremental
> > diff.
>
> Thanks, included.
>
> > > * Are the CommandComplete changes needed?
> >
> > Seems warranted to me. BTW, why is CLOSE's command tag "CLOSE CURSOR",
> > not "CLOSE"? That seems needlessly verbose, and inconsistent with other
> > commands (e.g. DEALLOCATE).
>
> Because the regular tag is "CLOSE CURSOR". I did not
> want to break any expectations. But yes, the inconsistency
> is weird.
>
> > > * ResetPlanCache() is implemented as PlanCacheCallback((Datum)0,
> > > InvalidOid); That seems to leave plans for utility commands untouched.
> > > Is it problem?
> >
> > Yes, I'd think you'd also want to cleanup plans for utility commands.
>
> Tom thought otherwise, so I kept the old way.
>
>
> --
> marko
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
On Sun, 2007-04-08 at 11:08 +0300, Marko Kreen wrote: > I think implicit ABORT would annoy various tools that > partially parse user sql and expect to know what transaction > state currently is. For them a new tranaction control statement > would be nuisance. That's not the only alternative: we could also either disallow all of the "ALL" variants in a transaction block, or allow RESET SESSION inside a transaction block. I've committed the patch basically as-is: thanks for the patch. I don't feel strongly about the above, but if there's a consensus, we can change the behavior later. -Neil
On 4/12/07, Neil Conway <neilc@samurai.com> wrote: > On Sun, 2007-04-08 at 11:08 +0300, Marko Kreen wrote: > > I think implicit ABORT would annoy various tools that > > partially parse user sql and expect to know what transaction > > state currently is. For them a new tranaction control statement > > would be nuisance. > > That's not the only alternative: we could also either disallow all of > the "ALL" variants in a transaction block, or allow RESET SESSION inside > a transaction block. > > I've committed the patch basically as-is: thanks for the patch. I don't > feel strongly about the above, but if there's a consensus, we can change > the behavior later. Thanks for reviewing it. One argument for top-level ALL commands is also that poolers and other tools in the middle of connection can track them. But it could also argued that they should have similar rules than ordinary CLOSE/DEALLOCATE statements. Also it seems that disallowing them inside functions for no good reason is couterproductive. But I also dont feel strongly either way. -- marko
Marko Kreen wrote: > On 4/12/07, Neil Conway <neilc@samurai.com> wrote: > > On Sun, 2007-04-08 at 11:08 +0300, Marko Kreen wrote: > > > I think implicit ABORT would annoy various tools that > > > partially parse user sql and expect to know what transaction > > > state currently is. For them a new tranaction control statement > > > would be nuisance. > > > > That's not the only alternative: we could also either disallow all of > > the "ALL" variants in a transaction block, or allow RESET SESSION inside > > a transaction block. > > > > I've committed the patch basically as-is: thanks for the patch. I don't > > feel strongly about the above, but if there's a consensus, we can change > > the behavior later. > > Thanks for reviewing it. > > One argument for top-level ALL commands is also that > poolers and other tools in the middle of connection can track > them. But it could also argued that they should have similar > rules than ordinary CLOSE/DEALLOCATE statements. Also it seems > that disallowing them inside functions for no good reason is > couterproductive. > > But I also dont feel strongly either way. I was thinking we would throw a WARNING rather than an error for RESET SESSION inside a transaction. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +