Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

On Sun, Sep 29, 2013 at 11:40:51AM +0530, Amit Kapila wrote:
> >> Shouldn't we do it for Set Constraints as well?
> >
> > Oh, very good point.  I missed that one.  Updated patch attached.

I am glad you are seeing things I am not.  :-)

> 1. The function set_config also needs similar functionality, else
> there will be inconsistency, the SQL statement will give error but
> equivalent function set_config() will succeed.
>
> SQL Command
> postgres=# set local search_path='public';
> ERROR:  SET LOCAL can only be used in transaction blocks
>
> Function
> postgres=# select set_config('search_path', 'public', true);
>  set_config
> ------------
>  public
> (1 row)

I looked at this but could not see how to easily pass the value of
'isTopLevel' down to the SELECT.  All the other checks have isTopLevel
passed down from the utility case statement.

> 2. I think we should update the documentation as well.
>
> For example:
> The documentation of LOCK TABLE
> (http://www.postgresql.org/docs/devel/static/sql-lock.html) clearly
> indicates that it will give error if used outside transaction block.
>
> "LOCK TABLE is useless outside a transaction block: the lock would
> remain held only to the completion of the statement. Therefore
> PostgreSQL reports an error if LOCK is used outside a transaction
> block. Use BEGINand COMMIT (or ROLLBACK) to define a transaction
> block."
>
>
> The documentation of SET TRANSACTION
> (http://www.postgresql.org/docs/devel/static/sql-set-transaction.html)
> has below line which doesn't indicate that it will give error if used
> outside transaction block.
>
> "If SET TRANSACTION is executed without a prior START TRANSACTION or
> BEGIN, it will appear to have no effect, since the transaction will
> immediately end."

Yes, you are right. All cases I changed had mentions of the command
having no effect;  I have updated it to mention an error is generated.

> 3.
>
> void
> ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
> {
> ..
> ..
> else if (strcmp(stmt->name, "TRANSACTION SNAPSHOT") == 0)
> {
> A_Const    *con = (A_Const *) linitial(stmt->args);
>
> RequireTransactionChain(isTopLevel, "SET TRANSACTION");
>
> if (stmt->is_local)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("SET LOCAL TRANSACTION SNAPSHOT is not implemented")));
> ..
> }
> ..
> ..
> }
>
> Wouldn't it be better if call to RequireTransactionChain() is done
> after if (stmt->is_local)?

Yes, good point.  Done.

Thanks much for your review.  Updated patch attached.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +