Thread: Where to call SetQuerySnapshot

I noticed that the new EXECUTE statement does not call SetQuerySnapshot,
which seems like a bad thing.  The omission is masked by the defensive
code in CopyQuerySnaphot, which will automatically do SetQuerySnapshot
if it hasn't been done yet in the current transaction.  However, this
doesn't provide the right behavior in read-committed mode: if we are
inside a transaction and not the first query, I'd think EXECUTE should
take a new snapshot; but it won't.

Comparable code can be found in COPY OUT, for which tcopy/utility.c
does SetQuerySnapshot() before calling the command-specific routine.

Questions for the list:

1. Where is the cleanest place to call SetQuerySnapshot() for utility
statements that need it?  Should we follow the lead of the existing
COPY code, and add the call to the ExecuteStmt case in utility.c?
Or should we move the calls into the command-specific routines (DoCopy
and ExecuteQuery)?  Or perhaps it should be done in postgres.c, which
has this responsibility for non-utility statements?

2. Would it be a good idea to change CopyQuerySnapshot to elog(ERROR)
instead of silently creating a snapshot when none has been made?
I think I was the one who put in its auto-create-snapshot behavior,
but I'm now feeling like that was a mistake.  It hides omissions that
we should want to find.
        regards, tom lane

Tom Lane wrote:
> 1. Where is the cleanest place to call SetQuerySnapshot() for utility
> statements that need it?  Should we follow the lead of the existing
> COPY code, and add the call to the ExecuteStmt case in utility.c?
> Or should we move the calls into the command-specific routines (DoCopy
> and ExecuteQuery)?  Or perhaps it should be done in postgres.c, which
> has this responsibility for non-utility statements?

Without looking at it too closely, I would think postgres.c would be best, 
unless there is a legit reason for a utility statement to *not* want 
SetQuerySnapshot called. If that's the case, then utility.c seems to make sense.

> 2. Would it be a good idea to change CopyQuerySnapshot to elog(ERROR)
> instead of silently creating a snapshot when none has been made?
> I think I was the one who put in its auto-create-snapshot behavior,
> but I'm now feeling like that was a mistake.  It hides omissions that
> we should want to find.

Is an assert appropriate?

Joe

Joe Conway <mail@joeconway.com> writes:
> Tom Lane wrote:
>> 1. Where is the cleanest place to call SetQuerySnapshot() for utility
>> statements that need it?

> Without looking at it too closely, I would think postgres.c would be best, 
> unless there is a legit reason for a utility statement to *not* want 
> SetQuerySnapshot called.

Actually, there are a number of past threads concerned with whether we
are doing SetQuerySnapshot in the right places --- eg, should it occur
between statements of a plplgsql function?  Right now it doesn't, but
maybe it should.  In any case I have a note that doing SetQuerySnapshot
for COPY OUT in utility.c is a bad idea, because it makes COPY OUT act
differently from any other statement, when used inside a function: it
*will* change the query snapshot, where nothing else does.  So I had
been thinking of pulling it out to postgres.c anyway.  I will do that.

>> 2. Would it be a good idea to change CopyQuerySnapshot to elog(ERROR)
>> instead of silently creating a snapshot when none has been made?

> Is an assert appropriate?

Works for me.
        regards, tom lane

I said:
> ...  So I had
> been thinking of pulling it out to postgres.c anyway.  I will do that.

I did this and ended up with a rather long list of statement types that
might need a snapshot:
               elog(DEBUG2, "ProcessUtility");
               /* set snapshot if utility stmt needs one */               /* XXX maybe cleaner to list those that
shouldn'tset one? */               if (IsA(utilityStmt, AlterTableStmt) ||                   IsA(utilityStmt,
ClusterStmt)||                   IsA(utilityStmt, CopyStmt) ||                   IsA(utilityStmt, ExecuteStmt) ||
           IsA(utilityStmt, ExplainStmt) ||                   IsA(utilityStmt, IndexStmt) ||
IsA(utilityStmt,PrepareStmt) ||                   IsA(utilityStmt, ReindexStmt))                   SetQuerySnapshot();
 

(Anything that can call the planner or might create entries in
functional indexes had better set a snapshot, thus stuff like
ReindexStmt has the issue.)

I wonder if we should turn this around, and set a snapshot for all
utility statements that can't show cause why they don't need one.
Offhand, TransactionStmt, FetchStmt, and VariableSet/Show/Reset
might be the only ones that need be excluded.  Comments?
        regards, tom lane

Tom Lane wrote:
> I did this and ended up with a rather long list of statement types that
> might need a snapshot:
> 
>                 elog(DEBUG2, "ProcessUtility");
> 
>                 /* set snapshot if utility stmt needs one */
>                 /* XXX maybe cleaner to list those that shouldn't set one? */
>                 if (IsA(utilityStmt, AlterTableStmt) ||
>                     IsA(utilityStmt, ClusterStmt) ||
>                     IsA(utilityStmt, CopyStmt) ||
>                     IsA(utilityStmt, ExecuteStmt) ||
>                     IsA(utilityStmt, ExplainStmt) ||
>                     IsA(utilityStmt, IndexStmt) ||
>                     IsA(utilityStmt, PrepareStmt) ||
>                     IsA(utilityStmt, ReindexStmt))
>                     SetQuerySnapshot();
> 
> (Anything that can call the planner or might create entries in
> functional indexes had better set a snapshot, thus stuff like
> ReindexStmt has the issue.)
> 
> I wonder if we should turn this around, and set a snapshot for all
> utility statements that can't show cause why they don't need one.
> Offhand, TransactionStmt, FetchStmt, and VariableSet/Show/Reset
> might be the only ones that need be excluded.  Comments?

It looks like an exclusion list would be easier to read and maintain.

Joe