On Sun, Jul 18, 2010 at 1:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On Sunday 18 July 2010 18:02:26 Simon Riggs wrote:
>>> Then I think the fix is to look at the xmin values on all of the tables
>>> used during planning and ensure that we only use constraint-based
>>> optimisations in a serializable transaction when our top xmin is later
>>> than the last DDL change (via its xmin).
>
>> Why not just use a the normal snapshot at that point?
>
> There isn't a "normal snapshot" that the planner should be relying on.
> It doesn't know what snap the resulting plan will be used with.
>
> I'm unconvinced that this is a problem worth worrying about, but if it
> is then Simon's probably got the right idea: check the xmin of a
> pg_constraint row before depending on it for planning. Compare the
> handling of indexes made with CREATE INDEX CONCURRENTLY.
It generally seems like a Bad Thing to use one snapshot for planning
and another snapshot for execution. For example, if one transaction
(ostensibly serializable) runs a query twice in a row and in the mean
time some other transaction redefines a function used by that query,
the two runs will return different results, which is inconsistent with
any serial order of execution of those transactions. But it seems
that it's far from clear what to do about it, and it's not the job of
this patch to fix it anyway.
Regarding the actual patch, it looks mostly good. Questions:
1. Why in rewriteSupport.c are we adding a call to
heap_inplace_update() in some situations? Doesn't seem like this is
something we should need or want to be monkeying with.
2. Instead of AlterTableGreatestLockLevel(), how about
AlterTableGetLockLevel()? Yeah, it's going to be the highest lock
level required by any subcommand, but it seems mildly overspecified.
I don't feel strongly about this one, though, if someone has a strong
contrary opinion...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company