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

Sat, 26 Feb 2000 23:48:49 -0500
Tom Lane <tgl@sss.pgh.pa.us>
update_pg_pwd trigger does not work very well
> 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, Pennsylvania 19026

Sun, 27 Feb 2000 00:35:42 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] update_pg_pwd trigger does not work very well
help

Sat, 26 Feb 2000 21:52:14 -0800
"Ray Messier" <messier@nichewareinc.com>
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



Mon, 28 Feb 2000 00:54:45 +0100 (CET)
Peter Eisentraut <peter_e@gmx.net>
Re: [HACKERS] update_pg_pwd trigger does not work very well
[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, Pennsylvania 19026

Sun, 27 Feb 2000 19:18:22 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] update_pg_pwd trigger does not work very well
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 earlier triggers
    have already done their external work.

    I think some "AT  COMMIT  EXECUTE  function(args)"  would  be
    better.   These  functions go into a separate queue and shall
    not modify any database content any more. At least, it  would
    restrict cases of external data inconsistency to 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 like general
    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) #



Mon, 28 Feb 2000 08:34:43 +0100 (CET)
wieck@debis.com (Jan Wieck)
Re: [HACKERS] update_pg_pwd trigger does not work very well
> [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) #



Mon, 28 Feb 2000 08:47:21 +0100 (CET)
wieck@debis.com (Jan Wieck)
Re: [HACKERS] update_pg_pwd trigger does not work very well
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

Mon, 28 Feb 2000 03:02:22 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] update_pg_pwd trigger does not work very well 
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) #



Mon, 28 Feb 2000 09:13:38 +0100 (CET)
wieck@debis.com (Jan Wieck)
Re: [HACKERS] update_pg_pwd trigger does not work very well
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 Service and other goodies at
  http://donb.photo.net.

Mon, 28 Feb 2000 06:16:20 -0800
Don Baccus <dhogaza@pacifier.com>
Re: [HACKERS] update_pg_pwd trigger does not work very well