Re: autocommit vs TRUNCATE et al

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> There are a number of statements, such as TRUNCATE TABLE, that refuse to
>> run in a transaction block because they perform actions that can't be
>> rolled back later.
>>
>> These statements currently do not look at autocommit, which means that
>> if autocommit is off, their tests will succeed ... but then a
>> transaction block is started anyway, defeating the point of the test.

> ... I think we
> should just do an automatic COMMIT if it is the first statement of a
> transaction, and if not, throw the same error we used to throw. We are
> performing autocommit for SET at the start of a transaction now anyway,
> so it isn't totally strange to do it for TRUNCATE, etc. too.

There is another aspect of this, which is: what if one of these
statements is called by a user-defined function, say a plpgsql function
that does a TRUNCATE and then other stuff? If the function is called
by a SELECT that's not inside a transaction block, then its
IsTransactionBlock() test will succeed --- but the possibility remains
that the later actions of the function could cause an elog and
transaction rollback. Which is what we wanted to prevent.

We can go with the auto-COMMIT idea for statements that are invoked at
the outer interactive level, but that doesn't work for stuff invoked
inside a function. I think we need to forbid these statements inside
functions, too. We already have that for VACUUM, because of its
internal commits causing problems for functions, but we'll need to
extend it to all of them.

Just FYI, the statements involved are
VACUUM
TRUNCATE TABLE
CREATE/DROP DATABASE
REINDEX (all forms)
ALTER USER changing password
DROP USER

ALTER and DROP USER just issue NOTICEs rather than failing, which seems
pretty bogus in itself. The reason they are worried is that there's
no mechanism for rolling back updates of the flat password file.
I think we could fix that by arranging not to write the flat password
file at all until we are ready to commit the current transaction;
will take a look at it.

REINDEX perhaps could be treated as transaction-safe in the forms that
build a new index file rather than truncating. Will look at that, too.

Another place that is calling IsTransactionBlock is parser/analyze.c
while processing DECLARE CURSOR. I think this is pretty bogus for
several reasons:
1. There's no good reason to make DECLARE CURSOR outside a transaction
block be an ERROR; at most it should be a NOTICE.
2. Parse analysis is the wrong place anyway; it should be tested
at execution time, methinks.
3. If the cursor is being declared and used inside a function, then
it could be used successfully without being inside a transaction
block at all.

Point 3 makes me think we should just get rid of the test entirely.
Comments?

regards, tom lane