Re: Comments on Custom RMGRs

On Fri, Mar 29, 2024 at 1:09 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> I am fine with this.
>
> You've moved the discussion forward in two ways:
>
> 1. Changes to pg_stat_statements to actually use the API; and
> 2. The hook is called at multiple points.
>
> Those at least partially address the concerns raised by Andres and
> Robert. But given that there was pushback from multiple people on the
> feature, I'd like to hear from at least one of them. It's very late in
> the cycle so I'm not sure we'll get more feedback in time, though.

In my seemingly-neverending pass through the July CommitFest, I
reached this patch. My comment is: it's possible that
rmgr_003.v3.patch is enough to be useful, but does anyone in the world
think they know that for a fact?

I mean, pgss_001.v1.patch purports to demonstrate that it can be used,
but that's based on rmgr_003.v2.patch, not the v3 patch, and the
emails seem to indicate that it may not actually work. I also think,
looking at it, that it looks much more like a POC than something we'd
consider ready for commit. It also seems very unclear that we'd want
pg_stat_statements to behave this way, and indeed "this way" isn't
really spelled out anywhere.

I think it would be nice if we had an example that uses the proposed
hook that we could actually commit. Maybe that's asking too much,
though. I think the minimum thing we need is a compelling rationale
for why this particular hook design is going to be good enough. That
could be demonstrated by means of (1) a well-commented example that
accomplishes some understandable goal and/or (2) a detailed
description of how a non-core table AM or index AM is expected to be
able to make use of this. Bonus points if the person providing that
rationale can say credibly that they've actually implemented this and
it works great with 100TB of production data.

The problem here is not only that we don't want to commit a hook that
does nothing useful. We also don't want to commit a hook that works
wonderfully for someone but we have no idea why. If we do that, then
we don't know whether it's OK to modify the hook in the future as the
code evolves, or more to the point, which kinds of modifications will
be acceptable. And also, the next person who wants to use it is likely
to have to figure out all on their own how to do so, instead of being
able to refer to comments or documentation or the commit message or at
least a mailing list post.

My basic position is not that this patch is a bad idea, but that it
isn't really finished. The idea is probably a pretty good one, but
whether this is a reasonable implementation of the idea doesn't seem
clear, at least not to me.

--
Robert Haas
EDB: http://www.enterprisedb.com