Thread: tick buildfarm failure

All,
 I've been keeping an eye on tick as it failed a day-or-so ago and it looks to be related to RLS.  Using a local
CLFAGS="-DCLOBBER_CACHE_ALWAYS-DRANDOMIZE_ALLOCATED_MEMORY" build, I was able to see the regression tests failing in
check_role_for_policy()due to a pretty clear reset of the memory used for the policies. 
 Looking through relcache.c, which I have to admit to not being as familiar with as some, the issue becomes rather
apparent(I believe)- RelationClearRelation() hasn't been set up to handle the RLS policies specially, as it does for
rulesand tupledesc.  Fair enough, it's reasonably straight-forward to add an equalPolicies() and handle the policies
thesame way- but I'm left wondering if that's actually *safe* to do? 
 To be a bit more clear- why is it safe to change the contents if the equal() function returns 'false'?  I'm guessing
theanswer is "locking"- whereby such a change would imply a lock was acquired on the relation by another backend, which
shouldn'tbe possible if we're currently working with it, but wanted to double-check that.  I also wonder if we might be
betteroff with a way to identify that nothing has actually been changed due to the locks we hold and avoid rebuilding
therelation cache entry entirely.. 
     Thanks!
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
>   To be a bit more clear- why is it safe to change the contents if the
>   equal() function returns 'false'?  I'm guessing the answer is
>   "locking"- whereby such a change would imply a lock was acquired on
>   the relation by another backend, which shouldn't be possible if we're
>   currently working with it, but wanted to double-check that.

Right.  If that were to fail, it would be the fault of something else
not having gotten sufficient lock for whatever it had been doing: either
lack of exclusive lock for an RLS update, or lack of an access lock to
examine the cached data.  The reason we want to make the equal() check
rather than just summarily replace the data is that code that *does*
hold AccessShare might reasonably expect the data to not move while it's
looking at it. 
>   I also wonder if we might be better off with a way to identify that
>   nothing has actually been changed due to the locks we hold and avoid
>   rebuilding the relation cache entry entirely..

I am entirely disinclined to reinvent relcache as part of the RLS patch.
You've got enough problems.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> >   To be a bit more clear- why is it safe to change the contents if the
> >   equal() function returns 'false'?  I'm guessing the answer is
> >   "locking"- whereby such a change would imply a lock was acquired on
> >   the relation by another backend, which shouldn't be possible if we're
> >   currently working with it, but wanted to double-check that.
>
> Right.  If that were to fail, it would be the fault of something else
> not having gotten sufficient lock for whatever it had been doing: either
> lack of exclusive lock for an RLS update, or lack of an access lock to
> examine the cached data.  The reason we want to make the equal() check
> rather than just summarily replace the data is that code that *does*
> hold AccessShare might reasonably expect the data to not move while it's
> looking at it.

Ok, great, thanks for the confirmation.  Yes- the RLS code wasn't
anticipating a change happening underneath it such as this (as other
places didn't appear worried about same, I didn't expect to see it be an
issue).  No problem, I'll definitely address it.

> >   I also wonder if we might be better off with a way to identify that
> >   nothing has actually been changed due to the locks we hold and avoid
> >   rebuilding the relation cache entry entirely..
>
> I am entirely disinclined to reinvent relcache as part of the RLS patch.

Apologies- I hadn't intend to even suggest that but rather to speculate
about this possibility for future improvements.
Thanks again!
    Stephen