Fix for VACUUM in psql autocommit off

"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: