I wrote:
> It wasn't marked Ready For Committer, so presumably the reviewer
> wasn't done with it. I know I hadn't looked at it at all, because
> I was waiting for the commitfest review process to finish.
... and now that I have, I find at least four highly questionable
things about it:
1. The placement of the hook. Why is it three lines down in
ProcessUtility? It's probably reasonable to have the Assert first,
but I don't see why the hook function should have the ability to
editorialize on the behavior of everything about ProcessUtility
*except* the read-only-xact check.
2. The naming and documentation of the added GUC setting for
pg_stat_statements. "track_ddl" seems pretty bizarre to me because
there are many utility statements that no one would call DDL. COPY,
for example, is certainly not DDL. Why not call it "track_utility"?
3. The enable-condition test in pgss_ProcessUtility. Is it really
appropriate to be gating this by isTopLevel? I should think that
the nested_level check in pgss_enabled would be sufficient and
more likely to do what's expected.
4. The special case for CopyStmt. That's just weird, and it adds
a maintenance requirement we don't need. I don't see a really good
argument why COPY (alone among utility statements) deserves to have
a rowcount tracked by pg_stat_statements, but even if you want that
it'd be better to rely on examining the completionTag after the fact.
The fact that the tag is "COPY nnnn" is part of the user-visible API
for COPY and won't change lightly. The division of labor between
ProcessUtility and copy.c is far more volatile, but this patch has
injected itself into that.
regards, tom lane
