Re: Command Triggers

Robert Haas <robertmhaas@gmail.com> writes:
> I'm just saying that nobody's realistically going to be able to verify
> a patch of this size.  It's either going to get committed warts and
> all, or it's going to not get committed.  Decomposing it into a series
> of patches would make it possible to actually verify the logic.  I
> guess on reflection I don't really care whether you decompose it at
> this point; the parts are pretty independent and it's easy enough to
> revert pieces of it.  But if I commit any of this it's certainly not
> going to be the whole thing in one go.

Ok, I can perfectly understand that.  The principled implementation is
not saving us here, we still need to review each call site.  The way I
read your comment, I continue working on my big patch and we'll see what
pieces get in, right?

> I think that there is no problem with cancelling a command via RAISE
> EXCEPTION.  It's an established precedent that errors can be thrown
> anywhere, and any code that doesn't deal with that is flat broken.

Sure.

> But I think letting either a BEFORE or INSTEAD trigger cancel the
> command is going to break things, and shouldn't be allowed without a
> lot of careful study.  So -1 from me on supporting INSTEAD triggers in
> the first version of this.

I'm sad about that, but I hear you.  Removing.

> Yowza.  A catalog scan is WAY more expensive than a syscache lookup.
> I definitely don't think you can afford to have every command result
> in an extra index probe into pg_cmdtrigger.  You definitely need some
> kind of caching there.
>
> Or at least, I think you do.  You could try pgbench -f foo.sql, where
> foo.sql repeatedly creates and drops a function.  See if there's a
> significant slowdown with your patch vs. HEAD.  If there is, you need
> some caching.  You might actually need some whole new type of sinval
> message to make this work efficiently.

Ok, I will test that down the road (before the end of this week).

>> I'm confused here, because all error messages that needs to contain the
>> namespace are doing exactly the same thing as I'm doing in my patch.
>
> Hmm.  I wonder what happens if those errors fire after the schema has
> been dropped?  I suppose the real answer here is probably to add
> enough locking that that can't happen in the first place... so maybe
> this isn't an issue for your patch to worry about.

I get it that I continue doing things this way, get_namespace_name() and
friends are trustworthy as far as I'm concerned.

>> You mean tablespace here, I guess, what else? I don't think I've added
>> other shared objects in there yet. I share your analyze btw, will
>> remove support.
>
> And databases.

Right, on their way.

>> something else, e.g. \dct would be your choice here?
>
> Yeah, probably.

Next version will sports that.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support