Re: Event Triggers: adding information

From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Event Triggers: adding information
Date: 2013-01-17 10:18:38
Message-ID: m2pq14m7sx.fsf@2ndQuadrant.fr
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> - adds ddl_command_trace and ddl_command_end events
> - causes those events to be called not only for actual SQL commands
> but also for things that happen, at present, to go through the same
> code path
> - adds additional magic variables to PL/pgsql to expose new
> information not previously exposed
> - adds CONTEXT as an additional event-trigger-filter variable, along with TAG
>
> In my mind, that's four or five separate patches. You don't have to
> agree, but that's what I think, and I'm not going to apologize for
> thinking it. Moreover, I think you will struggle to find examples of
> patches committed in the last three years that made as many different,
> only loosely-related changes as you're proposing for this one.

So, say I do that. Now we have 5 patches to review. They all are against
the same set of files, so there's a single possible ordering to apply
them, and so to review them. When we reach to an agreement out-of-order
(we will), it takes a sensible amount of time to rebuild the patch set
in a different order. I don't think we have the time to do that.

Now, if that's what it takes, I'll spend time on it. In which exact
order do you want to be reviewing and applying that series of patches?

> As for the patch not being well-thought-out, I'm sticking by that,
> too. Alvaro's comments about lock escalations should be enough to
> tickle your alarm bells, but there are plenty of other problems here,
> too.

Here's the comment in the code where the lock problems are discussed:

/*
* Fill in the EventTriggerTargetOid for a DROP command and a
* "ddl_command_start" Event Trigger: the lookup didn't happen yet and we
* still want to provide the user with that information when possible.
*
* We only do the lookup if the command contains a single element, which is
* often forced to be the case as per the grammar (see the drop_type:
* production for a list of cases where more than one object per command is
* allowed).
*
* We take no exclusive lock here, the main command will have to do the
* name lookup all over again and take appropriate locks down after the
* User Defined Code runs anyway, and the commands executed by the User
* Defined Code will take their own locks. We only lock the objects here so
* that they don't disapear under us in another concurrent transaction,
* hence using ShareUpdateExclusiveLock. XXX this seems prone to lock
* escalation troubles.
*/

My thinking is that the user code can do anything, even run a DROP
command against the object that we are preparing ourselves to be
dropping. I don't think we can bypass doing the lookup twice.

The way not to have to do any lookup in our code is not to report the
OID of the Object being dropped. Then, the event trigger code will do
that lookup as its first thing, certainly. The lookup will happen.

RAISE NOTICE 'drop table % (%)', tg_objectname::regclass,
tg_objectname::regclass::oid;

That is, at least, the angle I considered when crafting this patch. I'm
happy to change that angle if needed and adjust to things I missed and
you will tell me about.

>> I think you might want to review the use case behing ddl_command_trace,
>> that has been asked by who users wanted to see their use case supported
>> in some easier way than just what you're talking about here.
>
> […]
> Being able to do
> that in one command instead of two is not a sufficient reason to add
> another event type.

That feature was proposed in past october (with a patch) and received no
comment, so I went on with it. The performance costs of this patch is
another couple of cache lookups when doing a DDL command, which I saw as
acceptable.

http://www.postgresql.org/message-id/m28vbgcc30.fsf@2ndQuadrant.fr

I'm not so attached to it that I will put the rest of the patch in
danger with it, I'm fine about removing that part if we have a good
reason to. And yes, you (politely) saying "I don't like it" is a good
enough reason to.

Our process for a non-commiter proposal seems to me that you have to
send a patch showing what you're talking about for a thread to form and
to get some comments on the idea, either rejecting it or reaching to a
consensus about it. So the opening of that thread was in october and the
discussion happens now: it's very fine, and I could be happy and
thankfull about this fact with some subtle changes in the form of the
messages.

> I'm very much opposed to that proposal, as I am to your proposal to
> expose internal and generated events to users. Recursing into
> ProcessUtility is a nasty, messy hook that is responsible for subtle
> bugs and locking problems in our current DDL implementation. We

We are publishing 2 pieces of information tied to the implementation in
the current patch:

- the parse tree, only available to C code, and without any API
stability promises

- the fact that somme commands are running nested commands: serial does
a create sequence, create schema create table issues a create table.

My understanding of your proposal not to call them ddl_command_* events
is that you are exposing exactly the same set of implementation
dependent information to the users, just under another name.

What I mean is that if a user wants to log any sequence creation, he
will have to either know that some of them happen within the CONTEXT
named 'GENERATED', or to know that some of them happen as ddl_command_*
events and some of them happen as sequence_for_serial_* events, say.

In other words, while I can follow your general approach to things, I
don't see any practical impact that makes me think it's a much cleaner
proposal in terms of not publishing the implementation details, because
I don't think that it is possible to both give the users the information
they need and hide those implementation details.

> should be trying to refactor that to clean it up, not exposing it as a
> user-visible detail. I do NOT want a future refactoring effort to
> clean up race conditions and duplicate name lookups in the DDL code to
> be blocked because someone is relying on the details of the existing
> implementation in their event trigger implementation.

That problem is going to happen in some way or another. It's not about
avoiding the problem completely, but choosing a set of compromises. I'm
yet to understand what your set of compromises is in detailed enough
practical implementation terms (not just "it's another set of events")
and how it's a better compromise over all (I trust your judgement on
that, I still want to understand).

After all, the whole point of the Event Trigger patch is to expose some
level of implementation details.

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> What was discussed at the last dev meeting was assigning a committer to
> each large patch to start with, which would reduce the risk of the
> goalposts moving that way. It seems to me that Robert's at least
> unofficially taken that role for event triggers. You should be happy,
> because if I were reviewing it I'd likely bounce the whole thing.

As Robert, I don't think the problem is about moving goal posts here,
it's more about how we want to spend our time. I don't want to be
refactoring 5 branches each time we reach on a new consensus only to
realise that of course the inter-dependent set of patches just need to
be redone from scratch because we will be applying another one first.

As for the nested commands design, see above for why I'm not already
buying into his counter proposal. Hint: I don't think it solves the
problem Robert is complaining about. Details: I certainly don't
understand what problem he is complaining about really, and I know I've
been trying to get that information before. It must be something
obvious.

> I'm not convinced this will *ever* be a stable feature that doesn't
> create more problems than it fixes.

I'm still open to hearing about another way to implement

- ddl support for logical replication

- extensions to ddl behaviour

I want to be able to install an extension that will change the way some
DDL are run, and I know that what I want to do here will never get
accepted for the core version of PostgreSQL. That's a very fine position
that I understand well enough to work on a patch series where the only
goal is that I can do my hacking outside of PostgreSQL core.

> I've seen no such proposal, and it seems like a nonstarter just from the
> idea. dependency.c doesn't have a syntax tree to describe each object
> that it finds to drop; creating one, and then doing a lookup to re-find
> the object, is just going to make drops hugely slower and buggier. Not

We wouldn't want to be doing it that way, of course. One idea was to
create a new parse node (CascadeDropStmt, say) that gets the OID of the
object to drop, and only does another schema/name lookup when there's an
event trigger to fire.

So the only obvious part of that proposal is that the problem is complex
enough and the solution not ready yet. That's also why that problem is
not addressed in any patch I sent to this day. The status of that part
of the problem is still to find together a good design to implement.

That part of the discussion really should live in a separate thread.

> to mention the large amount of code that would have to be added and
> maintained. Not to mention that the objects dependency.c works with
> aren't necessarily all that interesting from the user's level --- for
> instance, do you really want to see each column default expression
> dropped individually? Not to mention that the permissions
> considerations are different from a standalone DROP.

No, we don't want to see all the details. Yes, we will need to come up
with a list of the ones we want to see. As I'm currently working on the
DDL part of the Event Triggers, my current thinking is to only include
objects that we could have been DROPing with a TOPLEVEL command.

> The bigger picture there is that it's taken us years, and multiple major
> iterations, to get cascaded drops to work properly and reliably. I'm
> disinclined to rip that code open and rewrite it completely; let alone
> add hooks that might inject random operations at any point in the
> process.

Fair enough. If that's the consensus, there's nothing to change in my
current patch, or maybe a paragraph or two in the docs to set users
expectations.

Maybe we should ask the logical replication guys their opinion about how
to solve DROP CASCADE support for them, so that we all agree that not
doing anything is a good enough answer.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Magnus Hagander 2013-01-17 10:28:15 Re: Multiple --table options for other commands
Previous Message Magnus Hagander 2013-01-17 10:16:16 Re: review: pgbench - aggregation of info written into log