On Tue, Mar 11, 2014 at 2:32 PM, Christian Kruse
<christian@2ndquadrant.com> wrote:
> On 11/03/14 13:23, Amit Kapila wrote:
>> Could you please once check (if you are comfortable doing so) wherever
>> this patch is passing tuple, whether it is okay to pass it based on visibility
>> rules, else I will again verify it once.
>
> I think I got all places, but it would be nice to have a confirmation.
How about in IndexBuildHeapScan()? This also uses SnapShotAny to
scan the heap data.
Normally in this function, passing tuple to lock routine should not be a
problem as it would have ensured to have exclusive lock on relation
before reaching this stage, but below comment in this function leads
me to think, that there can be problem during system catalog scan.
/*
* Since caller should hold ShareLock or better, normally
* the only way to see this is if it was inserted earlier
* in our own transaction. However, it can happen in
* system catalogs, since we tend to release write lock
* before commit there. Give a warning if neither case
* applies.
*/
I could not immediately think of testcase which can validate it, but this is
certainly a point to ponder.
Do you think it is safe to pass tuple to XactLockTableWaitWithInfo()
in this function?
I think now other things in your patch are good, just these tuple visibility
validations are tricky and it is taking time to validate the paths, because
these gets called in nested paths where in few cases even dirty or snapshot
any scans also seems to be safe w.r.t displaying tuple. Anyway, today I
have checked most paths, may be one more time I will give a look with fresh
mind and then pass on to Committer.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com