Thread: update_pg_pwd trigger does not work very well
I was looking at the trigger function that's been added to try to
update pg_pwd automatically if pg_shadow is updated via standard
SQL commands. It's got some problems:
1. Since the trigger is executed as soon as a tuple is inserted/
updated/deleted, it will write pg_pwd before the transaction is
committed. If you then abort the transaction, pg_pwd contains wrong
data. Even if you don't abort, the postmaster may read and act on the
updated pg_pwd before you've committed, which could have bad
consequences (logging in a user who doesn't exist yet, for example).
2. The trigger tries to grab AccessExclusiveLock on pg_shadow.
Since this is being done in the middle of a transaction that has
previously grabbed some lower level of lock on pg_shadow, it's
very easy to create a deadlock situation. All you need is two
different transactions modifying pg_shadow concurrently, and
it'll fail.
3. CREATE USER and friends refuse to run inside a transaction block
in the vain hope of making life safe for the trigger. It's vain
since the above problems will occur anyway, if one simply alters
pg_shadow using ordinary SQL commands. (And if we're not going to
support that, why bother with the trigger?) I think this is a rather
unpleasant restriction, especially so when it isn't buying any
safety at all.
A possible solution for these problems is to have the trigger procedure
itself do nothing except set a flag variable. The flag is examined
somewhere in xact.c after successful completion of a transaction,
and if it's set then we run a new transaction cycle in which we
read pg_shadow and write pg_pwd. (A new transaction is needed so
that it's safe to demand AccessExclusiveLock on pg_shadow --- we
have to release all our old locks before we can do that.) Note that
*only* this second transaction would need AccessExclusiveLock; CREATE
USER and friends would not.
I am not quite certain that this is completely bulletproof when there
are multiple backends concurrently updating pg_shadow, but I have not
been able to think of a case where it'd fail. The worst possibility
is that a committed update in pg_shadow might not get propagated to
pg_pwd for a while because some other transaction is holding a lock on
pg_shadow. (But pg_pwd updates can be delayed for that reason now,
so it's certainly no worse than before.)
Comments?
regards, tom lane
> I am not quite certain that this is completely bulletproof when there > are multiple backends concurrently updating pg_shadow, but I have not > been able to think of a case where it'd fail. The worst possibility > is that a committed update in pg_shadow might not get propagated to > pg_pwd for a while because some other transaction is holding a lock on > pg_shadow. (But pg_pwd updates can be delayed for that reason now, > so it's certainly no worse than before.) > > Comments? I see your point. Guess we have to do it inside xact. -- Bruce Momjian | http://www.op.net/~candle pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Tom Lane writes:
> 1. Since the trigger is executed as soon as a tuple is inserted/
> updated/deleted, it will write pg_pwd before the transaction is
> committed. If you then abort the transaction, pg_pwd contains wrong
> data.
Wow, that implies that every trigger that contains non-database
side-effects is potentially bogus. That never occured to me. Perhaps (as a
future plan), it would be a good idea to have deferred triggers as well?
Now that I think of it, wasn't that the very reason Jan had to invent the
separate constraint triggers?
> 2. The trigger tries to grab AccessExclusiveLock on pg_shadow.
It doesn't actually need that exclusive lock, I think. A shared read lock
(i.e., none really) would suffice.
> A possible solution for these problems is to have the trigger procedure
> itself do nothing except set a flag variable. The flag is examined
> somewhere in xact.c after successful completion of a transaction,
> and if it's set then we run a new transaction cycle in which we
> read pg_shadow and write pg_pwd.
If you think that this is okay (and not just a hack), then go for it. If
the above mentioned deferred triggers are at all in the near future I
wouldn't mind scrapping that trigger altogether. There isn't a good reason
to muck with pg_shadow.{usename|password|validuntil} anyway. And it is in
general not safe to muck with system catalogs period. (Try to rename a
table by updating pg_class.relname. ;)
--
Peter Eisentraut Sernanders väg 10:115
peter_e@gmx.net 75262 Uppsala
http://yi.org/peter-e/ Sweden
[Charset ISO-8859-1 unsupported, filtering to ASCII...] > Tom Lane writes: > > > 1. Since the trigger is executed as soon as a tuple is inserted/ > > updated/deleted, it will write pg_pwd before the transaction is > > committed. If you then abort the transaction, pg_pwd contains wrong > > data. > > Wow, that implies that every trigger that contains non-database > side-effects is potentially bogus. That never occured to me. Perhaps (as a > future plan), it would be a good idea to have deferred triggers as well? > Now that I think of it, wasn't that the very reason Jan had to invent the > separate constraint triggers? Yes! I remember him talking about this. I bet you can just modify your trigger to be of that type. -- Bruce Momjian | http://www.op.net/~candle pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Peter Eisentraut wrote: > Tom Lane writes: > > > 1. Since the trigger is executed as soon as a tuple is inserted/ > > updated/deleted, it will write pg_pwd before the transaction is > > committed. If you then abort the transaction, pg_pwd contains wrong > > data. > > Wow, that implies that every trigger that contains non-database > side-effects is potentially bogus. That never occured to me. Perhaps (as a > future plan), it would be a good idea to have deferred triggers as well? > Now that I think of it, wasn't that the very reason Jan had to invent the > separate constraint triggers? The reason for the trigger queue was deferrability at all. And the way it's implemented doesn't help out. Imagine that the trigger not only affects external data, but does further checks and/or modifications to table data.If some statement affects multiple rows, a trigger ran from the queue can still abort the transaction, but earliertriggers have already done their external work. I think some "AT COMMIT EXECUTE function(args)" would be better. These functions go into a separate queue andshall not modify any database content any more. At least, it would restrict cases of external data inconsistencyto cases, where external modifications fail. > > itself do nothing except set a flag variable. The flag is examined > > somewhere in xact.c after successful completion of a transaction, > > and if it's set then we run a new transaction cycle in which we > > read pg_shadow and write pg_pwd. > > If you think that this is okay (and not just a hack), then go for it. I consider it a hack, since this particular trigger needs a global flag known explicitly by xact routines. I likegeneral solutions instead. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #========================================= wieck@debis.com (Jan Wieck) #
> [Charset ISO-8859-1 unsupported, filtering to ASCII...]
> > Tom Lane writes:
> >
> > > 1. Since the trigger is executed as soon as a tuple is inserted/
> > > updated/deleted, it will write pg_pwd before the transaction is
> > > committed. If you then abort the transaction, pg_pwd contains wrong
> > > data.
> >
> > Wow, that implies that every trigger that contains non-database
> > side-effects is potentially bogus. That never occured to me. Perhaps (as a
> > future plan), it would be a good idea to have deferred triggers as well?
> > Now that I think of it, wasn't that the very reason Jan had to invent the
> > separate constraint triggers?
>
> Yes! I remember him talking about this. I bet you can just modify your
> trigger to be of that type.
He could make the trigger look like a by default deferred RI trigger in pg_trigger, of course. Then it will go
onto the queue.
But as soon as someone does
SET CONSTRAINTS ALL IMMEDIATE;
it will be fired if queued or as soon as it appears. So it's not the final solution.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#========================================= wieck@debis.com (Jan Wieck) #
wieck@debis.com (Jan Wieck) writes:
>>>> itself do nothing except set a flag variable. The flag is examined
>>>> somewhere in xact.c after successful completion of a transaction,
>>>> and if it's set then we run a new transaction cycle in which we
>>>> read pg_shadow and write pg_pwd.
>>
>> If you think that this is okay (and not just a hack), then go for it.
> I consider it a hack, since this particular trigger needs a
> global flag known explicitly by xact routines. I like general
> solutions instead.
Well, really it's pg_pwd itself that is a hack --- we wouldn't need
to be worrying about all this if pg_pwd didn't exist outside the
database/transaction universe. But I don't think it'd be wise to
try to bring the postmaster into that universe, so we're stuck with
a hack for exporting user authorization info.
If we had examples of other problems that could be solved by such
a mechanism, then I'd agree with Jan that we ought to invent a general
after-commit-do mechanism. But I don't recall users clamoring for it,
so I question whether the extra effort is worthwhile.
regards, tom lane
Tom Lane wrote: > wieck@debis.com (Jan Wieck) writes: > > I consider it a hack, since this particular trigger needs a > > global flag known explicitly by xact routines. I like general > > solutions instead. > > Well, really it's pg_pwd itself that is a hack --- we wouldn't need > to be worrying about all this if pg_pwd didn't exist outside the > database/transaction universe. But I don't think it'd be wise to > try to bring the postmaster into that universe, so we're stuck with > a hack for exporting user authorization info. > > If we had examples of other problems that could be solved by such > a mechanism, then I'd agree with Jan that we ought to invent a general > after-commit-do mechanism. But I don't recall users clamoring for it, > so I question whether the extra effort is worthwhile. Exactly these days there was someone having trouble to dynamically load the tclLDAP package into PL/Tcl. He wanted to UPDATE his LDAP from inside a trigger. If he hasn't had this loading problem, I'd never known. So I assume there are already constructs like this out there. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #========================================= wieck@debis.com (Jan Wieck) #
At 08:34 AM 2/28/00 +0100, Jan Wieck wrote: > I consider it a hack, since this particular trigger needs a > global flag known explicitly by xact routines. I like general > solutions instead. It's very much a hack. Something like the "on commit execute()" solution would be much better. How crucial is a short-term fix for this particular problem? Could it wait until a more generalized facility is provided? If not I suppose it could be hacked in with copious notes that it should be redone later. - Don Baccus, Portland OR <dhogaza@pacifier.com> Nature photos, on-line guides, Pacific Northwest Rare Bird Alert Serviceand other goodies at http://donb.photo.net.