Justin Pryzby <pry...@telsasoft.com> writes:
> You set the patch to "waiting on author", which indicates that there's
> no need for further input or review. But, I think that's precisely
> what's needed - without input from more people, what could I do to
> progress the patch ? I don't think it's reasonable to put 001 first and
> change thousands (actually, 1338) of regression results. If nobody
> wants to discuss 001, then this patch series won't progress.
Well ...
1. 0001 invents a new GUC but provides no documentation for it.
That certainly isn't committable, and it's discouraging the
discussion you seek because people have to read the whole patch
in detail to understand what is being proposed.
2. The same complaint for 0004, which labors under the additional
problem that MACHINE is one of the worst option names I've ever
seen proposed around here. It conveys *nothing* about what it does
or is good for. The fact that it's got no relationship to the GUC
name, and yet they're intimately connected, doesn't improve my
opinion of it.
3. I'm not really on board with the entire approach. Making
EXPLAIN work significantly differently for developers and test
critters than it does for everyone else seems likely to promote
confusion and hide bugs. I don't think getting rid of the need
for filter functions in test scripts is worth that.
4. I don't see how the current patches could pass under "make
installcheck" --- it looks to me like it only takes care of
applying the GUC change in installations built by pg_regress
or Cluster.pm. To make this actually work, you'd have to
add "explain_regress = on" to the ALTER DATABASE SET commands
issued for regression databases. That'd substantially increase
the problem of "it works differently than what users see", at
least for me --- I do a lot of stuff interactively in the
regression database.
5. The change in postgres_fdw.c in 0001 concerns me too.
Yeah, it will fix things if an updated postgres_fdw talks to
an updated server, but what about an older postgres_fdw?
Don't really want to see that case break; communication with
servers of different major versions is one of postgres_fdw's
main goals.
As a side note, 0001 also creates a hard break in postgres_fdw's
ability to work with pre-9.0 remote servers, which won't accept
"EXPLAIN (COSTS)". Maybe we don't care about that anymore, given
the policy we've adopted elsewhere that we won't test or support
interoperability with versions before 9.2. But we should either
make the command spelling conditional on remote version, or
officially move that goalpost.
regards, tom lane
