Manu <[email protected]> writes:
> Thanks, that reads better. Done in v3, attached. The length cap goes
> too: next_suffix is an unsigned, and strtoul saturates instead of
> wrapping, so a longer suffix never comes out below it.
I still don't like this patch too much: it is complicated and not
clearly correct. For example, consider the case where we've
generated "expr_1" through "expr_3", and then we encounter a
user-written name "expr_5". It looks to me like we will consider
that *not* to match the SubplanNameSuffix for "expr", so we will
enter "expr_5" as a simple base name in the other list. I think
that the right things happen afterwards because subplan_name_in_use
always searches both lists, but this situation is hard to reason
about and utterly undocumented in the comments. The business about
strtoul overflow somehow didn't make it into the comments either.
I wonder if it'd be better to fold the two lists into one,
with a list of structs like
typedef struct SubplanNameData
{
const char *base; /* the base name */
bool taken; /* if true, the base name itself is taken */
unsigned next_suffix; /* if > 0, every smaller suffix is taken */
} SubplanNameData;
That doesn't directly fix the complication I mention above,
but maybe it'd provide a basis for documenting it. Or maybe
it's not helpful, not sure.
I still wonder if we couldn't simplify this a great deal by
abandoning the constraint that the generated names should match
what the existing code does, along the lines Heikki mentioned.
I wouldn't want to change that behavior post-19.0, but it's not
quite too late to consider it now.
regards, tom lane
