On Sun, Jun 21, 2020 at 12:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Agreed as to the bug, but I think we ought to fix it by redefining
> > explain_get_index_name's API as "return the bare index name always",
> > and let the callers apply quoting.
>
> Concretely, as attached.
>
> I am unsure about back-patching this, but am leaning to doing so.
>
> (1) From the perspective of end users, this makes no difference for
> text output and fixes a pretty clear bug in non-text formats. If
> we don't fix it then users may be tempted to try to dequote in
> application code, which won't work very reliably and will do
> completely the wrong thing after the bug is fixed. You might argue
> that some apps might already be applying such dequoting, but
> I doubt it; wouldn't they have reported the bug?
>
> (2) From the perspective of extensions using explain_get_index_name_hook,
> this is a silent API change: before they were supposed to quote,
> now they are not. That's not great, but I don't think there is any
> fix that doesn't involve an API change for hook users.
>
> There's certainly a case to be made that it'd be better if the API
> change happened only in v13 and not in minor releases. But the
> consequences of not updating a hook-using extension immediately
> wouldn't be that severe --- non-text output is just as it was, and
> the double-double-quoting in text output would only be visible if the
> extension chooses quote-requiring names for hypothetical indexes.
> Even if it were visible it probably would just be ugly, and not
> fundamentally break anything.
>
> In any case, I think there are few enough people using index-advisor
> extensions that we should not let consideration (2) outweigh
> consideration (1).
It turns out that in hypopg I totally missed that
explain_get_index_name_hook should take care of quoting the indexname.
People only want to know whether the hypothetical index is used or
not, so the lack of correct quoting didn't prevent that (the
hypothetical index name is automatically generated and includes its
oid). The patch looks good to me, and +1 for backpatch!