Re: Insufficient locking for ALTER DEFAULT PRIVILEGES

On Sat, Jun 20, 2015 at 10:57 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:
>
> Vik Fearing wrote:
>
> > Session 1:
> > begin;
> > alter default privileges in schema bug grant all on tables to postgres;
> >
> > Session 2:
> > alter default privileges in schema bug grant all on tables to postgres;
> > <hangs>
> >
> > Session 1:
> > commit;
> >
> > Session 2:
> > ERROR: tuple concurrently updated
>
> So it turns out we don't have any locking here at all. I don't believe
> we have it for all object types, but in most cases it's not as obnoxious
> as this one. But at least for relations we have some nice coding in
> RangeVarGetRelidExtended and RangeVarGetAndCheckCreationNamespace that
> protect things.
>
> I was thinking of adding some similar locking-and-looping logic in
> StoreDefaultACL: grab the tuple from catalogs, LockDatabaseObject()
> using the OID of the tuple so obtained; check the sinval counter like
> RangeVarGetRelidExtended, if no change we're okay; if it changed, go
> grab the OID once again, and if it changed, restart from the top; if
> the OID did not change, then we're done.
>
> This sounds complicated, but it's actually reasonably straightforward
> and contained within a single routine.
>
> But then, it doesn't handle the case that two transactions try to start
> a row for the same combination at the same time. One of them is going
> to get the heap_insert() call to succeed and the other one is going to
> get an ugly error message.
>
> I wonder if I'm over-thinking this. Other thoughts?
>

This problem won't occur for dml statements (2 sessions trying to update
the tuple in same way as described in above example) and the reason is
that ExecUpdate() has EvalPlanQual() mechanism to avoid this, I am
talking about below code:

ExecUpdate()
{
..
heap_update();

case HeapTupleUpdated:
..
if (!ItemPointerEquals(tupleid, &hufd.ctid))
..
epqslot = EvalPlanQual(estate,
..
}

Now for the similar case in simple_heap_update(), we throw error, can't
we use similar coding pattern (as in ExecUpdate) in simple_heap_update()?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com