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

On Sun, Jun 21, 2020 at 4:29 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > 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!
>
> Ah, I'd wondered whether there might be hook users that "should" be
> doing quoting and were not. So the impact on hypopg would be:
>
> 1. Non-text EXPLAIN formats are already properly quoted, and will
> remain so.

Correct. Simple test using hypothetical index on a "T1" (id integer) table:

=# select * from hypopg_create_index('create index on "T1"(id)');
indexrelid | indexname
------------+---------------------
576402 | <576402>btree_T1_id
(1 row)

Before:

=# explain (format yaml) select * from "T1" where id = 1;
QUERY PLAN
---------------------------------------
- Plan: +
Node Type: "Index Only Scan" +
Parallel Aware: false +
Scan Direction: "Forward" +
Index Name: "<576402>btree_T1_id"+
Relation Name: "T1" +
Alias: "T1" +
Startup Cost: 0.04 +
Total Cost: 8.06 +
Plan Rows: 1 +
Plan Width: 4 +
Index Cond: "(id = 1)"
(1 row)

After:

=# explain (format yaml)select * from "T1" where id = 1;
QUERY PLAN
--------------------------------------
- Plan: +
Node Type: "Index Only Scan" +
Parallel Aware: false +
Scan Direction: "Forward" +
Index Name: "<16442>btree_T1_id"+
Relation Name: "T1" +
Alias: "T1" +
Startup Cost: 0.04 +
Total Cost: 4.06 +
Plan Rows: 1 +
Plan Width: 4 +
Index Cond: "(id = 1)"
(1 row)

> 2. Text-format EXPLAIN output will grow double-quotes around the
> hypothetical index names, which are not there at present (I believe,
> but didn't test it). This shouldn't bother human users particularly,
> but conceivably it might break hypopg's regression tests?

Yes, the patch is working as expected. Before:

=# explain select * from "T1" where id = 1;
QUERY PLAN
-------------------------------------------------------------------------------------
Index Only Scan using <576402>btree_T1_id on "T1" (cost=0.04..8.06
rows=1 width=4)
Index Cond: (id = 1)
(2 rows)

Patched:

=# explain select * from "T1" where id = 1;
QUERY PLAN
--------------------------------------------------------------------------------------
Index Only Scan using "<16442>btree_T1_id" on "T1" (cost=0.04..4.06
rows=1 width=4)
Index Cond: (id = 1)
(2 rows)

All of the regression tests except the one I added when the fix for
brin hypothetical indexes was applied are still working, as they're
looking for patterns like "Index.*<\d+>${amname}_${relname}" in the
textual explain output. There are a few fixes not released yet, so
I'll update the broken brin regression test and publish a release soon
to make sure that the packages don't get broken when the new minor
versions are released.