Re: Command Triggers

Robert Haas <robertmhaas(at)gmail(dot)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