Thread: Fix for VACUUM in psql autocommit off

In current cvs (as in version 7.4.5), VACUUM does not work at all in
autocommit=off mode. That is, because psql does not know that VACUUM cannot
be performed inside an transaction.

Even if you do
ROLLBACK; VACUUM;
it will internally issue a ROLLBACK; BEGIN; VACUUM;

I consider this a bug and suggest the attached fix. is_transact_command() in
src/bin/psql/common.c is used to determine if a command is a transaction
modifying command. The diff just adds "vacuum" to those commands, so that
psql will not issue a BEGIN before a VACUUM.

Best Regards,
Michael Paesold

"Michael Paesold" <mpaesold@gmx.at> writes:
> In current cvs (as in version 7.4.5), VACUUM does not work at all in
> autocommit=off mode. That is, because psql does not know that VACUUM cannot
> be performed inside an transaction.
> I consider this a bug and suggest the attached fix.

If we're going to do that, we should also include the other statements
that disallow execution in a transaction, and we should rename
is_transact_command to something more appropriate (not to mention fix
its comments).  A quick grep shows

/home/postgres/pgsql/src/backend/commands/dbcommands.c: 95:     PreventTransactionChain((void *) stmt, "CREATE
DATABASE");
/home/postgres/pgsql/src/backend/commands/dbcommands.c: 497:     PreventTransactionChain((void *) dbname, "DROP
DATABASE");
/home/postgres/pgsql/src/backend/commands/cluster.c: 175:         PreventTransactionChain((void *) stmt, "CLUSTER");
/home/postgres/pgsql/src/backend/commands/indexcmds.c: 995:     PreventTransactionChain((void *) dbname, "REINDEX
DATABASE");
/home/postgres/pgsql/src/backend/commands/vacuum.c: 262:         PreventTransactionChain((void *) vacstmt, stmttype);
/home/postgres/pgsql/src/backend/commands/tablespace.c: 221:     PreventTransactionChain((void *) stmt, "CREATE
TABLESPACE");
/home/postgres/pgsql/src/backend/commands/tablespace.c: 407:     PreventTransactionChain((void *) stmt, "DROP
TABLESPACE");

Handling the multi-keyword cases is going to take a nontrivial increment
of functionality.  Perhaps while we're at it, we could teach this code
about nested /* comments ...

            regards, tom lane

Tom Lane wrote:


> If we're going to do that, we should also include the other statements
> that disallow execution in a transaction, and we should rename
> is_transact_command to something more appropriate (not to mention fix
> its comments).  A quick grep shows
>
> PreventTransactionChain((void *) stmt, "CREATE DATABASE");
> PreventTransactionChain((void *) dbname, "DROP DATABASE");
> PreventTransactionChain((void *) stmt, "CLUSTER");
> PreventTransactionChain((void *) dbname, "REINDEX DATABASE");
> PreventTransactionChain((void *) vacstmt, stmttype);
> PreventTransactionChain((void *) stmt, "CREATE TABLESPACE");
> PreventTransactionChain((void *) stmt, "DROP TABLESPACE");

> Handling the multi-keyword cases is going to take a nontrivial increment
> of functionality.  Perhaps while we're at it, we could teach this code
> about nested /* comments ...

Currently there is no need for nested comments, because those are only
single word queries. Or do I not understand what you mean by nested
comments? (There is code for ignore /* .. */ before the first keyword.)

Any suggestion how to that? I can think of a way myself, but it may not be
the best, as I don't consider C my natural language. I can try, or does
anyone else feel inclined to fix this?

Best Regards,
Michael Paesold

"Michael Paesold" <mpaesold@gmx.at> writes:
>  Or do I not understand what you mean by nested
> comments? (There is code for ignore /* .. */ before the first keyword.)

Per SQL spec, the backend thinks that /* .. */ nests:

regression=# /* some /* comment */ comment */ select 1;
 ?column?
----------
        1
(1 row)

As it stands, is_transact_command will get confused by this.

> Any suggestion how to that? I can think of a way myself, but it may not be
> the best, as I don't consider C my natural language. I can try, or does
> anyone else feel inclined to fix this?

I'd split out the code that locates "the next keyword" into a separate
function that could be called twice.

            regards, tom lane

Tom Lane wrote:

> "Michael Paesold" <mpaesold@gmx.at> writes:
> >  Or do I not understand what you mean by nested
> > comments? (There is code for ignore /* .. */ before the first keyword.)
>
> Per SQL spec, the backend thinks that /* .. */ nests:
>
> regression=# /* some /* comment */ comment */ select 1;
>  ?column?
> ----------
>         1
> (1 row)
>
> As it stands, is_transact_command will get confused by this.

Ok, my patch does not solve that, but I will have a look at it. Tomorrow it
will be then, since it's already late here in Europe.

> > Any suggestion how to that? I can think of a way myself, but it may not
be
> > the best, as I don't consider C my natural language. I can try, or does
> > anyone else feel inclined to fix this?
>
> I'd split out the code that locates "the next keyword" into a separate
> function that could be called twice.

I have read this mail after making the attached patch, so I have used a
little different approach. I have moved the code to skip over whitespace and
comments into it's own function.

The code is not perfect, but it works. By not perfect I mean, it also
returns true, if you say "REINDEX TABLESPACE", but this does not exist
anyways.

Please have a look.

Regards,
Michael Paesold

Michael Paesold wrote
> Tom Lane wrote:
> > Per SQL spec, the backend thinks that /* .. */ nests:
> >
> > regression=# /* some /* comment */ comment */ select 1;
> >  ?column?
> > ----------
> >         1
> > (1 row)
> >
> > As it stands, is_transact_command will get confused by this.

It is now fixed in the attached patch.

> > I'd split out the code that locates "the next keyword" into a separate
> > function that could be called twice.
>
> I have read this mail after making the attached patch, so I have used a
> little different approach. I have moved the code to skip over whitespace
and
> comments into it's own function.

Well, reading this again after some sleep, we probably meant the same.

Concerning my other patch that also touches psql/common.c (Rollback of only
last statement), have you looked into it?

Best Regards,
Michael Paesold

"Michael Paesold" <mpaesold@gmx.at> writes:
>> Per SQL spec, the backend thinks that /* .. */ nests:

> It is now fixed in the attached patch.

Applied with some additional cleanup (the code wasn't multibyte-aware,
and so could get fooled in some Far Eastern encodings).

> Concerning my other patch that also touches psql/common.c (Rollback of only
> last statement), have you looked into it?

I have not.

            regards, tom lane

Tom Lane wrote:

> > It is now fixed in the attached patch.
>
> Applied with some additional cleanup (the code wasn't multibyte-aware,
> and so could get fooled in some Far Eastern encodings).

I am very pleased to hear. This was my first patch submitted. :-)

Best Regards,
Michael Paesold

> Tom Lane wrote:
>
> > > It is now fixed in the attached patch.
> >
> > Applied with some additional cleanup (the code wasn't multibyte-aware,
> > and so could get fooled in some Far Eastern encodings).

Looking at your cleanup is a good for learning more about C. :-)

But I have one another question, you wrote: