RESET SESSION v3

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. +