Michael Paquier <mich...@paquier.xyz> writes:
> On Wed, Aug 07, 2019 at 10:17:25AM -0400, Tom Lane wrote:
>> Not objecting to the patch, exactly, just feeling like there's
>> more here than meets the eye. Not quite sure if it's worth
>> investigating closer, or what we'd even need to do to do so.
> Yes, something's weird here. I'd think that the index only scan
> ensures a proper ordering in this case, so it could be possible that a
> different plan got selected here? That would mean that the plan
> selected would not be an index-only scan or an index scan. So perhaps
> that was a bitmap scan?
I hacked temp.sql to print a couple different plans (doing it that way,
rather than manually, just to ensure that I was getting plans matching
what would actually happen right there). And what I see, as attached,
is that IOS and plain index and bitmap scans all have pretty much the
same total cost. The planner then ought to prefer IOS or plain on the
secondary grounds of cheaper startup cost. However, it's not so hard
to believe that it might switch to bitmap if something caused the cost
estimates to change by a few percent. So probably we should write this
off as "something affected the plan choice" and just add the ORDER BY
as you suggest.
>> BTW, I realize from looking at the plan that LIKE is interpreting the
>> underscores as wildcards. Maybe it's worth s/_/\_/ while you're
> Right. Looking around there are much more tests which have the same
> problem. This could become a problem if other tests running in
> parallel use relation names with the same pattern, which is not a
> issue as of HEAD, so I'd rather just back-patch the ORDER BY part of
> it (temp.sql is the only test missing that). What do you think about
> the attached?
Hmm, I wasn't thinking of changing anything more than this one query.
I'm not sure that a wide-ranging patch is going to be worth the
potential back-patching land mines it'd introduce. However, if you
want to do it anyway, please at least patch v12 as well --- that
should still be a pretty painless back-patch, even if it's not so
easy to go further.
BTW, most of the problem here seems to be that the SQL committee
made an infelicitous choice of wildcard characters for LIKE.
I wonder if it'd be saner to fix this by switching to regexes?
regression=# explain select relname from pg_class where relname like
'temp_parted_oncommit_test%';
QUERY PLAN
-------------------------------------------------------------------------------------------------
Index Only Scan using pg_class_relname_nsp_index on pg_class (cost=0.28..4.30
rows=1 width=64)
Index Cond: ((relname >= 'temp'::text) AND (relname < 'temq'::text))
Filter: (relname ~~ 'temp_parted_oncommit_test%'::text)
(3 rows)
regression=# explain select relname from pg_class where relname ~
'^temp_parted_oncommit_test';
QUERY PLAN
------------------------------------------------------------------------------------------------------------------
Index Only Scan using pg_class_relname_nsp_index on pg_class (cost=0.28..4.30
rows=1 width=64)
Index Cond: ((relname >= 'temp_parted_oncommit_test'::text) AND (relname <
'temp_parted_oncommit_tesu'::text))
Filter: (relname ~ '^temp_parted_oncommit_test'::text)
(3 rows)
regards, tom lane
diff -U3 /home/postgres/pgsql/src/test/regress/expected/temp.out /home/postgres/pgsql/src/test/regress/results/temp.out
--- /home/postgres/pgsql/src/test/regress/expected/temp.out 2019-08-05 11:20:31.612729499 -0400
+++ /home/postgres/pgsql/src/test/regress/results/temp.out 2019-08-11 15:45:24.471393832 -0400
@@ -273,6 +273,36 @@
(1 row)
-- two relations remain in this case.
+explain
+select relname from pg_class where relname like 'temp_parted_oncommit_test%';
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------
+ Index Only Scan using pg_class_relname_nsp_index on pg_class (cost=0.28..8.30 rows=1 width=64)
+ Index Cond: ((relname >= 'temp'::text) AND (relname < 'temq'::text))
+ Filter: (relname ~~ 'temp_parted_oncommit_test%'::text)
+(3 rows)
+
+set enable_indexonlyscan TO 0;
+explain
+select relname from pg_class where relname like 'temp_parted_oncommit_test%';
+ QUERY PLAN
+--------------------------------------------------------------------------------------------
+ Index Scan using pg_class_relname_nsp_index on pg_class (cost=0.28..8.30 rows=1 width=64)
+ Index Cond: ((relname >= 'temp'::text) AND (relname < 'temq'::text))
+ Filter: (relname ~~ 'temp_parted_oncommit_test%'::text)
+(3 rows)
+
+set enable_indexscan TO 0;
+explain
+select relname from pg_class where relname like 'temp_parted_oncommit_test%';
+ QUERY PLAN
+-----------------------------------------------------------------------------------------
+ Bitmap Heap Scan on pg_class (cost=4.29..8.30 rows=1 width=64)
+ Filter: (relname ~~ 'temp_parted_oncommit_test%'::text)
+ -> Bitmap Index Scan on pg_class_relname_nsp_index (cost=0.00..4.29 rows=1 width=0)
+ Index Cond: ((relname >= 'temp'::text) AND (relname < 'temq'::text))
+(4 rows)
+
select relname from pg_class where relname like 'temp_parted_oncommit_test%';
relname
----------------------------
@@ -280,6 +310,8 @@
temp_parted_oncommit_test1
(2 rows)
+reset enable_indexscan;
+reset enable_indexonlyscan;
drop table temp_parted_oncommit_test;
-- Check dependencies between ON COMMIT actions with inheritance trees.
-- Using ON COMMIT DROP on a parent removes the whole set.
