Re: API change advice: Passing plan invalidation info from the rewriter into the planner? - Mailing list pgsql-hackers

Craig, Tom, all,

I've been through the RLS code over the past couple of days which I
pulled from Craig's repo and have a bunch of minor updates.  In general,
the patch seems pretty reasonable- except for the issues discussed
below.  Quite a bit of this patch is tied up in plan invalidation and
tracking if the security quals depend on the current user, all of which
seems pretty grotty and the wrong way around to me.

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Craig Ringer <craig@2ndquadrant.com> writes:
> > It's only that the plan depends on the user ID. There's no point keeping
> > the plan around if the user no longer exists.
>
> [ shrug... ]  Leaving such a plan cached would be harmless, though.

Agreed.

> Furthermore, the user ID we'd be talking about is either the owner
> of the current session, or the owner of some view or security-definer
> function that the plan is already dependent on, so it's fairly hard
> to credit that the plan would survive long enough for the issue to
> arise.

I don't entirely follow which 'issue' is being referred to here, but we
need to consider that 'set role' changes should also cause a new plan.

> Even if there is a scenario where invalidating by user ID is actually
> useful, I think adding infrastructure to cause invalidation in such a case
> is optimizing for the wrong thing.  You're adding cycles to every query to
> benefit a case that is going to be quite infrequent in practice.

Yeah, I have a hard time seeing that there's an issue w/ keeping the
cached plans around even if the session never goes back to being under
the user ID for which those older plans were built.  Also, wouldn't a
'RESET ALL' clear any of them anyway?

> > Yes, that would be good, but is IMO more of a separate optimization. I'm
> > currently using KaiGai's code to invalidate and re-plan when a user ID
> > change is detected.
>
> I'm unlikely to accept a patch that does that; wouldn't it be catastrophic
> for performance in the presence of security-definer functions?  You can't
> just trash the whole plan cache when a user ID switch occurs.

Yeah, this doesn't seem like the right approach.  Adding the user ID to
the cache key definitely strikes me as the right way to fix this.

I've uploaded the latest patch, rebased against master, with my changes
to here: http://snowman.net/~sfrost/rls_ringerc_sf.patch.gz as I don't
believe it'd clear the mailing list (it's 29k).

I'll take a look at changing the cache key to include user ID and
ripping out the plan invalidation logic from the current patch tomorrow
but I seriously doubt I'll be able to get all of that done in the next
day or two.  If anyone else is able to help out, it'd certainly be
appreciated; I really think that's the main hurdle to address at this
point with this patch- without the plan invalidation complexity, the
the patch is really just building out the catalog, the SQL-level
statements for managing it, and the bit of code required to add the
conditional to statements involving RLS-enabled tables.
Thanks,
    Stephen