> On Jul 14, 2020, at 4:12 PM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
>
> This patch is way too large. Probably in part explained by the fact
> that you seem to have run pgindent and absorbed a lot of unrelated
> changes. This makes the patch essentially unreviewable.
I did not run pgindent, but when changing
if (relkind == RELKIND_INDEX)
{
/* foo */
}
to
switch (relkind)
{
case RELKIND_INDEX:
/* foo */
}
the indentation of /* foo */ changes. For large foo, that results in a lot of
lines. There are also cases in the code where comparisons of multiple
variables are mixed together. To split those out into switch/case statements I
had to rearrange some of the code blocks.
> I think you should define a RelationGetRelkind() static function that
> returns the appropriate datatype without requiring a cast and assert in
> every single place that processes a relation's relkind. Similarly
> you've chosen to leave get_rel_relkind untouched, but that seems unwise.
I was well aware of how large the patch had gotten, and didn't want to add
more....
> I think the chr_ macros are pointless.
Look more closely at the #define RelKindAsString(x) CppAsString2(chr_##x)
> Reading back the thread, it seems that the whole point of your patch was
> to change the tests that currently use 'if' tests to switch blocks. I
> cannot understand what's the motivation for that,
There might not be sufficient motivation to make the patch worth doing. The
motivation was to leverage the project's recent addition of -Wswitch to make it
easier to know which code needs updating when you add a new relkind. That
doesn't happen very often, but I was working towards that kind of thing, and
thought this might be a good prerequisite patch for that work. Stylistically,
I also prefer
+ switch ((RelKind) rel->rd_rel->relkind)
+ {
+ case RELKIND_RELATION:
+ case RELKIND_MATVIEW:
+ case RELKIND_TOASTVALUE:
over
- if (rel->rd_rel->relkind == RELKIND_RELATION ||
- rel->rd_rel->relkind == RELKIND_MATVIEW ||
- rel->rd_rel->relkind == RELKIND_TOASTVALUE)
which is a somewhat common pattern in the code. It takes less mental effort to
see that only one variable is being compared against those three enum values.
In some cases, though not necessarily this exact example, it also *might* save
duplicated work computing the variable, depending on the situation and what the
compiler can optimize away.
> but it appears to me
> that the approach is backwards: I'd counsel to *first* change the APIs
> (get_rel_relkind and defining an enum, plus adding RelationGetRelkind)
> so that everything is more sensible and safe, including appropriate
> answers for the places where an "invalid" relkind is returned;
Ok.
> and once
> that's in place, replace if tests with switch blocks where it makes
> sense to do so.
Ok.
>
> Also, I suggest that this thread is not a good one for this patch.
> Subject is entirely not appropriate. Open a new thread perhaps?
I've changed the subject line.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
