Обсуждение: Announcing Veil
This is to announce the first, Alpha, release of Veil, a database security add-on for Postgres. It allows databases to be created with efficient row-level access controls; different users have access to different subsets of data. Veil is hosted on pgfoundry at http://veil.projects.postgresql.org/ Documentation is here: http://veil.projects.postgresql.org/curdocs/index.html If there is sufficient interest, I would ultimately like Veil to be added to contrib - we will have to see how things pan out. I am announcing this to the postgres hackers list as I hope for review comments from a qualified community. All comments will be welcomed. If you would like somewhere controversial to start your review, take a look at veil_shmem.c which hooks into the postgres shared memory subsystem. Since I was unable to dynamically assign a LWLock using LWLockAssign (none available), I have fairly arbitrarily overloaded the use of existing LWLocks. When the flames die down perhaps we can discuss making a small number (one would be enough for me) of LWLocks available. Thank you. __ Marc Munro
Marc Munro <marc@bloodnok.com> writes:
> Since I was unable to dynamically assign a LWLock using
> LWLockAssign (none available), I have fairly arbitrarily overloaded the
> use of existing LWLocks. When the flames die down perhaps we can
> discuss making a small number (one would be enough for me) of LWLocks
> available.
Perhaps you missed the comment in NumLWLocks()?
regards, tom lane
Tom,
Thanks for your reponse. Unless I am missing your point, to add more
locks we require a minor code change to the postgres server. I am happy
to submit a patch but this will not help Veil work with existing
versions of Postgres. I am aiming for compatibility with 7.4 onward.
Your views on this would be appreciated.
Assuming that simply allocating a few extra LWLocks for user-defined
functions is acceptable, here are some patches:
--cut---------------
*** ./src/backend/storage/lmgr/lwlock.c Sat Aug 20 16:26:24 2005
--- lwlock.c Wed Oct 5 08:20:31 2005
***************
*** 120,126 **** */ numLocks += 2 * NUM_SLRU_BUFFERS;
! /* Perhaps create a few more for use by user-defined modules? */
return numLocks; }
--- 120,127 ---- */ numLocks += 2 * NUM_SLRU_BUFFERS;
! /* Create a few more for use by user-defined modules. */
! numLocks += NUM_USER_DEFINED_LWLOCKS;
return numLocks; }
--cut---------------
*** ./src/include/storage/lwlock.h Sat Aug 20 16:26:34 2005
--- lwlock.h Wed Oct 5 08:22:26 2005
***************
*** 53,58 ****
--- 53,62 ---- MaxDynamicLWLock = 1000000000 } LWLockId;
+ /*
+ * Allocate a few LWLocks for user-defined functions.
+ */
+ #define NUM_USER_DEFINED_LWLOCKS 4
typedef enum LWLockMode {
--cut---------------
__
Marc Munro
On Tue, 2005-10-04 at 22:51 -0400, Tom Lane wrote:
> Marc Munro <marc@bloodnok.com> writes:
> > Since I was unable to dynamically assign a LWLock using
> > LWLockAssign (none available), I have fairly arbitrarily overloaded the
> > use of existing LWLocks. When the flames die down perhaps we can
> > discuss making a small number (one would be enough for me) of LWLocks
> > available.
>
> Perhaps you missed the comment in NumLWLocks()?
>
> regards, tom lane
I don't see NUM_USER_DEFINED_LWLOCKS defined in 8.0 or 8.1, so what
system do you propose to allow you to set this value?
---------------------------------------------------------------------------
Marc Munro wrote:
-- Start of PGP signed section.
> Tom,
> Thanks for your reponse. Unless I am missing your point, to add more
> locks we require a minor code change to the postgres server. I am happy
> to submit a patch but this will not help Veil work with existing
> versions of Postgres. I am aiming for compatibility with 7.4 onward.
> Your views on this would be appreciated.
>
> Assuming that simply allocating a few extra LWLocks for user-defined
> functions is acceptable, here are some patches:
>
> --cut---------------
> *** ./src/backend/storage/lmgr/lwlock.c Sat Aug 20 16:26:24 2005
> --- lwlock.c Wed Oct 5 08:20:31 2005
> ***************
> *** 120,126 ****
> */
> numLocks += 2 * NUM_SLRU_BUFFERS;
>
> ! /* Perhaps create a few more for use by user-defined modules? */
>
> return numLocks;
> }
> --- 120,127 ----
> */
> numLocks += 2 * NUM_SLRU_BUFFERS;
>
> ! /* Create a few more for use by user-defined modules. */
> ! numLocks += NUM_USER_DEFINED_LWLOCKS;
>
> return numLocks;
> }
> --cut---------------
> *** ./src/include/storage/lwlock.h Sat Aug 20 16:26:34 2005
> --- lwlock.h Wed Oct 5 08:22:26 2005
> ***************
> *** 53,58 ****
> --- 53,62 ----
> MaxDynamicLWLock = 1000000000
> } LWLockId;
>
> + /*
> + * Allocate a few LWLocks for user-defined functions.
> + */
> + #define NUM_USER_DEFINED_LWLOCKS 4
>
> typedef enum LWLockMode
> {
> --cut---------------
>
>
> __
> Marc Munro
>
> On Tue, 2005-10-04 at 22:51 -0400, Tom Lane wrote:
> > Marc Munro <marc@bloodnok.com> writes:
> > > Since I was unable to dynamically assign a LWLock using
> > > LWLockAssign (none available), I have fairly arbitrarily overloaded the
> > > use of existing LWLocks. When the flames die down perhaps we can
> > > discuss making a small number (one would be enough for me) of LWLocks
> > > available.
> >
> > Perhaps you missed the comment in NumLWLocks()?
> >
> > regards, tom lane
-- End of PGP section, PGP failed!
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square,
Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I don't see NUM_USER_DEFINED_LWLOCKS defined in 8.0 or 8.1, so what
> system do you propose to allow you to set this value?
I'd be willing to add the proposed patch in 8.1 (style note:
NUM_USER_DEFINED_LWLOCKS should be set in pg_config_manual.h not
lwlock.h). However, it's certainly not going to magically appear in
existing releases, so I dunno if that'd make Marc happy or not.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I don't see NUM_USER_DEFINED_LWLOCKS defined in 8.0 or 8.1, so what > > system do you propose to allow you to set this value? > > I'd be willing to add the proposed patch in 8.1 (style note: > NUM_USER_DEFINED_LWLOCKS should be set in pg_config_manual.h not > lwlock.h). However, it's certainly not going to magically appear in > existing releases, so I dunno if that'd make Marc happy or not. Shouldn't it be something we can put in postgresql.conf? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> I'd be willing to add the proposed patch in 8.1 (style note:
>> NUM_USER_DEFINED_LWLOCKS should be set in pg_config_manual.h not
>> lwlock.h).
> Shouldn't it be something we can put in postgresql.conf?
No more than any of the other entries in pg_config_manual.h.
With only one known request for a user-allocated lock, it's hard to
justify the overhead of a GUC variable.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Tom Lane wrote: > >> I'd be willing to add the proposed patch in 8.1 (style note: > >> NUM_USER_DEFINED_LWLOCKS should be set in pg_config_manual.h not > >> lwlock.h). > > > Shouldn't it be something we can put in postgresql.conf? > > No more than any of the other entries in pg_config_manual.h. > With only one known request for a user-allocated lock, it's hard to > justify the overhead of a GUC variable. True, but are people going to recompile PostgreSQL to use this feature? Seems they would have to. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> With only one known request for a user-allocated lock, it's hard to
>> justify the overhead of a GUC variable.
> True, but are people going to recompile PostgreSQL to use this feature?
> Seems they would have to.
How you figure that? The proposed default value was 4, which seems
fine to me, given that the known worldwide demand amounts to 1.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Tom Lane wrote: > >> With only one known request for a user-allocated lock, it's hard to > >> justify the overhead of a GUC variable. > > > True, but are people going to recompile PostgreSQL to use this feature? > > Seems they would have to. > > How you figure that? The proposed default value was 4, which seems > fine to me, given that the known worldwide demand amounts to 1. Oh, so you are going to give him a few slots. I thought we were going to default to 0 and he was going to have to bump it up to use his software. That sounds fine to me. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Thu, 2005-06-10 at 23:56 -0400, Bruce Momjian wrote: > True, but are people going to recompile PostgreSQL to use this feature? > Seems they would have to. They would need to recompile PostgreSQL to use more than the default number of user-defined LWLocks, which seems perfectly reasonable to me. -Neil
In response to both Bruce and Tom, Thanks for this. I am very happy that this patch will be going in. Thanks also for pointing out the correct header to use. As Tom points out, this will do nothing for users of 7.4 and 8.0. For these versions I propose to continue to use MMCacheLock. As far as I can see, this is safe in 7.4, and otherwise unused in 8.0. On the use of LWLockAssign():can anyone tell me if I should protect the call using the ShmemLock spinlock? __ Marc Munro On Fri, 2005-10-07 at 00:10 -0400, Bruce Momjian wrote: > Tom Lane wrote: > > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > > Tom Lane wrote: > > >> With only one known request for a user-allocate d lock, it's hard to > > >> justify the overhead of a GUC variable. > > > > > True, but are people going to recompile PostgreSQL to use this feature? > > > Seems they would have to. > > > > How you figure that? The proposed default value was 4, which seems > > fine to me, given that the known worldwide demand amounts to 1. > > Oh, so you are going to give him a few slots. I thought we were going > to default to 0 and he was going to have to bump it up to use his > software. That sounds fine to me. >
Marc Munro <marc@bloodnok.com> writes:
> Thanks for this. I am very happy that this patch will be going in.
> Thanks also for pointing out the correct header to use.
Patch applied for 8.1.
> As Tom points out, this will do nothing for users of 7.4 and 8.0. For
> these versions I propose to continue to use MMCacheLock. As far as I
> can see, this is safe in 7.4, and otherwise unused in 8.0.
Yeah, MMCacheLock is for long-dead code, so you can probably get away
with using it in the back versions.
> On the use of LWLockAssign():can anyone tell me if I should protect the
> call using the ShmemLock spinlock?
Hmm ... the comment for LWLockAssign is not meant to imply that the
caller could do that; in the event of being out of LWLocks, the code
would elog(FATAL) without releasing the spinlock, which would lock up
the whole database. If we were to do it that way we'd need the spinlock
handling to be done inside LWLockAssign. This would not be that bad,
just a marginal slowdown during database startup, but given the low
demand for this feature I'm not very eager to do it.
The alternative though would seem to be to adopt some convention about
another LWLock to take while trying to assign a new LWLock post-startup.
None of the existing locks seem very appropriate for this, and putting
the responsibility on callers might be unwise anyway.
Thoughts?
regards, tom lane
For my part, I don't see any current need for extra locking here. Veil ensures that only one session ever calls LWLockAssign(), and as the Veil LWLock is allocated on the first piece of user-invoked SQL to call a Veil function, I see no scope for races between Veil and the rest of Postgres. Maybe the correct thing to do is only allow 1 user-defined LWLock for now, and place a comment with the definition of NUM_USER_DEFINED_LWLOCKS to warn that locking should be implemented if more than 1 is ever needed. __ Marc On Fri, 2005-10-07 at 16:21 -0400, Tom Lane wrote: > Marc Munro <marc@bloodnok.com> writes: > > On the use of LWLockAssign():can anyone tell me if I should protect the > > call using the ShmemLock spinlock? > > Hmm ... the comment for LWLockAssign is not meant to imply that the > caller could do that; in the event of being out of LWLocks, the code > would elog(FATAL) without releasing the spinlock, which would lock up > the whole database. If we were to do it that way we'd need the spinlock > handling to be done inside LWLockAssign. This would not be that bad, > just a marginal slowdown during database startup, but given the low > demand for this feature I'm not very eager to do it. > > The alternative though would seem to be to adopt some convention about > another LWLock to take while trying to assign a new LWLock post-startup. > None of the existing locks seem very appropriate for this, and putting > the responsibility on callers might be unwise anyway. > > Thoughts? > > regards, tom lane