Re: BUG #16502: EXPLAIN JSON format adds extra quotes around index names

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).

            regards, tom lane

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 67bdcb2b27..a131d15ac0 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1456,7 +1456,8 @@ ExplainNode(PlanState *planstate, List *ancestors,
                 explain_get_index_name(bitmapindexscan->indexid);

                 if (es->format == EXPLAIN_FORMAT_TEXT)
-                    appendStringInfo(es->str, " on %s", indexname);
+                    appendStringInfo(es->str, " on %s",
+                                     quote_identifier(indexname));
                 else
                     ExplainPropertyText("Index Name", indexname, es);
             }
@@ -3267,6 +3268,10 @@ show_eval_params(Bitmapset *bms_params, ExplainState *es)
  *
  * We allow plugins to get control here so that plans involving hypothetical
  * indexes can be explained.
+ *
+ * Note: names returned by this function should be "raw"; the caller will
+ * apply quoting if needed.  Formerly the convention was to do quoting here,
+ * but we don't want that in non-text output formats.
  */
 static const char *
 explain_get_index_name(Oid indexId)
@@ -3279,11 +3284,10 @@ explain_get_index_name(Oid indexId)
         result = NULL;
     if (result == NULL)
     {
-        /* default behavior: look in the catalogs and quote it */
+        /* default behavior: look it up in the catalogs */
         result = get_rel_name(indexId);
         if (result == NULL)
             elog(ERROR, "cache lookup failed for index %u", indexId);
-        result = quote_identifier(result);
     }
     return result;
 }
@@ -3463,7 +3467,7 @@ ExplainIndexScanDetails(Oid indexid, ScanDirection indexorderdir,
     {
         if (ScanDirectionIsBackward(indexorderdir))
             appendStringInfoString(es->str, " Backward");
-        appendStringInfo(es->str, " using %s", indexname);
+        appendStringInfo(es->str, " using %s", quote_identifier(indexname));
     }
     else
     {