Issue in pg_catalog.pg_indexes view definition

Started by Dilip Kumarover 9 years ago12 messages

dilipbalaut@gmail.com

over 9 years ago

While running sqlsmith tool, I saw some cache lookup failure issues
reported,

While investigating those issues, I found one strange reason, and I feel
It's a bug in pg code.

Query:
postgres=# select * from pg_catalog.pg_indexes where indexdef is not null;
ERROR: cache lookup failed for index 2619

If we see the plan for the same:
-------------------------------------------
Nested Loop Left Join
Join Filter: (t.oid = i.reltablespace)
-> Hash Left Join
Hash Cond: (c.relnamespace = n.oid)
-> Hash Join
Hash Cond: (i.oid = x.indexrelid)
* -> Seq Scan on pg_class i*
* Filter: ((pg_get_indexdef(oid) IS NOT NULL) AND
(relkind = 'i'::"char"))*
.......

Problem Analysis:
-------------------------
pg_get_indexdef(oid) clause is pushed down to pg_class, Which is logically
correct,
but pg_class will have other oids also (which are not index) and will get
cache lookup
failure error.

I think problem is in definition of pg_indexes view,
(projectio"*pg_get_indexdef(i.oid) AS
indexdef*").

Basically we are using some function which can only be called on index oid
otherwise we will get an error. So logically both view and push down in
above query
is fine, but we are using restricted function (*pg_get_indexdef(i.oid)*)
which should not
be push down. Or should be pushed down to pg_index.

View definition:
SELECT n.nspname AS schemaname,
c.relname AS tablename,
i.relname AS indexname,
t.spcname AS tablespace,
*pg_get_indexdef(i.oid) *AS indexdef
FROM pg_index x
JOIN pg_class c ON c.oid = x.indrelid
JOIN pg_class i ON i.oid = x.indexrelid
LEFT JOIN pg_namespace n ON n.oid = c.relnamespace
LEFT JOIN pg_tablespace t ON t.oid = i.reltablespace
WHERE (c.relkind = ANY (ARRAY['r'::"char", 'm'::"char"])) AND i.relkind =
'i'::"char";

I am not sure what should be the correct fix for this problem.

I think even if we try to call this function on index oid *pg_get_indexdef(*
x.indexrelid*) *AS indexdef, problem will not be solved, because both will
fall in same equivalence class hence clause can be distributed to pg_class
also.

Is this a bug ?
If yes, what should be the right fix ?

Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Dilip Kumar

dilipbalaut@gmail.com

over 9 years ago

In reply to: Dilip Kumar (#1)

1 attachment(s)

Re: Issue in pg_catalog.pg_indexes view definition

On Thu, Jul 14, 2016 at 12:38 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:

I am not sure what should be the correct fix for this problem.

I think even if we try to call this function on index oid
*pg_get_indexdef(*x.indexrelid*) *AS indexdef, problem will not be
solved, because both will fall in same equivalence class hence clause can
be distributed to pg_class also.

I was wrong, Actually If we change the view and call function on
x.indexrelid, It will fix the issue, because *pg_get_indexdef(*x.indexrelid*)
*is non equal clause and of course will not fall in same equivalence class.

Patch is attached for the same..

*Plan after patch: *(Now it pushed to pg_index table, which is perfectly
fine)

Nested Loop Left Join (cost=22.36..39.30 rows=9 width=288)
Join Filter: (t.oid = i.reltablespace)
-> Hash Left Join (cost=22.36..37.98 rows=9 width=200)
Hash Cond: (c.relnamespace = n.oid)
-> Hash Join (cost=21.23..36.72 rows=9 width=140)
Hash Cond: (i.oid = x.indexrelid)
-> Seq Scan on pg_class i (cost=0.00..14.95 rows=121
width=72)
*Filter: (relkind = 'i'::"char")*
-> Hash (cost=20.93..20.93 rows=24 width=72)
-> Hash Join (cost=15.72..20.93 rows=24 width=72)
Hash Cond: (x.indrelid = c.oid)
-> Seq Scan on pg_index x (cost=0.00..4.51
rows=120 width=8)
* Filter: (pg_get_indexdef(indexrelid) IS
NOT NULL)*

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Attachments:

pg_indexes_fix.patchapplication/octet-stream; name=pg_indexes_fix.patchDownload

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 4fc5d5a..28707ee 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -151,7 +151,7 @@ CREATE VIEW pg_indexes AS
         C.relname AS tablename,
         I.relname AS indexname,
         T.spcname AS tablespace,
-        pg_get_indexdef(I.oid) AS indexdef
+        pg_get_indexdef(X.indexrelid) AS indexdef
     FROM pg_index X JOIN pg_class C ON (C.oid = X.indrelid)
          JOIN pg_class I ON (I.oid = X.indexrelid)
          LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 9 years ago

In reply to: Dilip Kumar (#1)

Re: Issue in pg_catalog.pg_indexes view definition

On 2016/07/14 16:08, Dilip Kumar wrote:

Query:
postgres=# select * from pg_catalog.pg_indexes where indexdef is not null;
ERROR: cache lookup failed for index 2619

If we see the plan for the same:
-------------------------------------------
Nested Loop Left Join
Join Filter: (t.oid = i.reltablespace)
-> Hash Left Join
Hash Cond: (c.relnamespace = n.oid)
-> Hash Join
Hash Cond: (i.oid = x.indexrelid)
* -> Seq Scan on pg_class i*
* Filter: ((pg_get_indexdef(oid) IS NOT NULL) AND
(relkind = 'i'::"char"))*
.......

...

I am not sure what should be the correct fix for this problem.

I think even if we try to call this function on index oid *pg_get_indexdef(*
x.indexrelid*) *AS indexdef, problem will not be solved, because both will
fall in same equivalence class hence clause can be distributed to pg_class
also.

Is this a bug ?
If yes, what should be the right fix ?

Can we say that pg_get_indexdef() has "side-effects" because it can error
like this? Shouldn't such a function be marked *volatile*? Because if I
do so by updating pg_proc, the plan changes (perhaps) to a safe one in
this context:

explain (costs off) select * from pg_catalog.pg_indexes where indexdef is
not null;
QUERY PLAN

-----------------------------------------------------------------------------------------
Subquery Scan on pg_indexes
Filter: (pg_indexes.indexdef IS NOT NULL)
-> Nested Loop Left Join
Join Filter: (t.oid = i.reltablespace)
-> Hash Left Join
Hash Cond: (c.relnamespace = n.oid)
-> Hash Join
Hash Cond: (i.oid = x.indexrelid)
-> Seq Scan on pg_class i
Filter: (relkind = 'i'::"char")
-> Hash
-> Hash Join
Hash Cond: (x.indrelid = c.oid)
-> Seq Scan on pg_index x
-> Hash
-> Seq Scan on pg_class c
Filter: (relkind = ANY
('{r,m}'::"char"[]))
-> Hash
-> Seq Scan on pg_namespace n
-> Materialize
-> Seq Scan on pg_tablespace t
(21 rows)

Thanks,
Amit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

over 9 years ago

In reply to: Dilip Kumar (#2)

Re: Issue in pg_catalog.pg_indexes view definition

On Thu, Jul 14, 2016 at 4:59 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:

I was wrong, Actually If we change the view and call function on
x.indexrelid, It will fix the issue, because pg_get_indexdef(x.indexrelid)
is non equal clause and of course will not fall in same equivalence class.

-        pg_get_indexdef(I.oid) AS indexdef
+        pg_get_indexdef(X.indexrelid) AS indexdef
Fixing it this way looks like a good idea to me to bypass those cache
lookup errors caused by non-index relations. Now I think that you need
as well to update the regression test output.
-- 
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 9 years ago

In reply to: Amit Langote (#3)

Re: Issue in pg_catalog.pg_indexes view definition

On 2016/07/14 17:07, Amit Langote wrote:

Can we say that pg_get_indexdef() has "side-effects" because it can error
like this? Shouldn't such a function be marked *volatile*? Because if I
do so by updating pg_proc, the plan changes (perhaps) to a safe one in
this context:

Didn't mean to write: "Because if I do so..."

Instead: "If I do so..."

Thanks,
Amit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Dilip Kumar

dilipbalaut@gmail.com

over 9 years ago

In reply to: Amit Langote (#3)

Re: Issue in pg_catalog.pg_indexes view definition

On Thu, Jul 14, 2016 at 1:37 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp

wrote:

Can we say that pg_get_indexdef() has "side-effects" because it can error
like this? Shouldn't such a function be marked *volatile*? Because if I
do so by updating pg_proc, the plan changes (perhaps) to a safe one in
this context:

That is another option, but by nature this function is not actually
volatile, because if clause is on *pg_index* indexrelid then it can be
pushed down.

So I think changing the view definition and calling this function on
indexrelid will remove the error. So I think
correct fix is to change view definition, as I proposed in above patch.

Any other opinion on this ?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Dilip Kumar

dilipbalaut@gmail.com

over 9 years ago

In reply to: Michael Paquier (#4)

1 attachment(s)

Re: Issue in pg_catalog.pg_indexes view definition

On Thu, Jul 14, 2016 at 1:40 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

-        pg_get_indexdef(I.oid) AS indexdef
+        pg_get_indexdef(X.indexrelid) AS indexdef
Fixing it this way looks like a good idea to me to bypass those cache
lookup errors caused by non-index relations. Now I think that you need
as well to update the regression test output.

I observed one regression failure caused by this fix. So fixed the same in
new patch.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Attachments:

pg_indexes_fix_v1.patchapplication/octet-stream; name=pg_indexes_fix_v1.patchDownload

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 4fc5d5a..28707ee 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -151,7 +151,7 @@ CREATE VIEW pg_indexes AS
         C.relname AS tablename,
         I.relname AS indexname,
         T.spcname AS tablespace,
-        pg_get_indexdef(I.oid) AS indexdef
+        pg_get_indexdef(X.indexrelid) AS indexdef
     FROM pg_index X JOIN pg_class C ON (C.oid = X.indrelid)
          JOIN pg_class I ON (I.oid = X.indexrelid)
          LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index ad44ae2..9285d4f 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1342,7 +1342,7 @@ pg_indexes| SELECT n.nspname AS schemaname,
     c.relname AS tablename,
     i.relname AS indexname,
     t.spcname AS tablespace,
-    pg_get_indexdef(i.oid) AS indexdef
+    pg_get_indexdef(x.indexrelid) AS indexdef
    FROM ((((pg_index x
      JOIN pg_class c ON ((c.oid = x.indrelid)))
      JOIN pg_class i ON ((i.oid = x.indexrelid)))

Michael Paquier

michael.paquier@gmail.com

over 9 years ago

In reply to: Dilip Kumar (#7)

Re: Issue in pg_catalog.pg_indexes view definition

On Thu, Jul 14, 2016 at 5:29 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Jul 14, 2016 at 1:40 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
-        pg_get_indexdef(I.oid) AS indexdef
+        pg_get_indexdef(X.indexrelid) AS indexdef
Fixing it this way looks like a good idea to me to bypass those cache
lookup errors caused by non-index relations. Now I think that you need
as well to update the regression test output.
I observed one regression failure caused by this fix. So fixed the same in
new patch.

Yes that was rules.out that needed a refresh. Thanks!
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

over 9 years ago

In reply to: Dilip Kumar (#6)

Re: Issue in pg_catalog.pg_indexes view definition

Dilip Kumar <dilipbalaut@gmail.com> writes:

On Thu, Jul 14, 2016 at 1:37 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp

wrote:
Can we say that pg_get_indexdef() has "side-effects" because it can error
like this? Shouldn't such a function be marked *volatile*? Because if I
do so by updating pg_proc, the plan changes (perhaps) to a safe one in
this context:

That is another option, but by nature this function is not actually
volatile, because if clause is on *pg_index* indexrelid then it can be
pushed down.

So I think changing the view definition and calling this function on
indexrelid will remove the error. So I think
correct fix is to change view definition, as I proposed in above patch.

I'm unimpressed with that solution. It cannot be back-patched, because
it forces an initdb; and while it might avoid the issue for this specific
use of this specific view, there are plenty of other scenarios in which
someone could apply pg_get_indexdef() and get a similar error. For
example, it's not at all uncommon to see reports of failures like this
for a just-dropped index (when the query's snapshot can still see the
index's catalog entries, but internally the backend realizes it's gone).
One recent example is
/messages/by-id/CAHnozTjkOP8o0MqNZtuc5HgPD1tLRmTQvZAKY+RNNvOkmMbK0A@mail.gmail.com

We've dealt with similar issues in places like pg_relation_size() by
making the functions return NULL instead of throwing an error for an
unmatched argument OID. It's pretty tempting to make the ruleutils.c
functions behave similarly. We'd have to look at the usages in pg_dump
and psql to see if any of them need adjustment; I imagine pg_dump at least
would need to be taught to fail if it gets back a NULL for the index
definition.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10

Andreas Seltenreich

seltenreich@gmx.de

over 9 years ago

In reply to: Tom Lane (#9)

Re: Issue in pg_catalog.pg_indexes view definition

Tom Lane writes:

Dilip Kumar <dilipbalaut@gmail.com> writes:

So I think changing the view definition and calling this function on
indexrelid will remove the error. So I think
correct fix is to change view definition, as I proposed in above patch.

[...]

We've dealt with similar issues in places like pg_relation_size() by
making the functions return NULL instead of throwing an error for an
unmatched argument OID.

Note that Michael Paquier sent a patch implementing this in another
thread:

/messages/by-id/CAB7nPqTxF5dtxjEzB7xkJvOWxX8D_2atxmTu3PSnkhcWT_JY5A@mail.gmail.com

regards,
andreas

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11

Tom Lane

tgl@sss.pgh.pa.us

over 9 years ago

In reply to: Andreas Seltenreich (#10)

Re: Issue in pg_catalog.pg_indexes view definition

Andreas Seltenreich <seltenreich@gmx.de> writes:

Tom Lane writes:

We've dealt with similar issues in places like pg_relation_size() by
making the functions return NULL instead of throwing an error for an
unmatched argument OID.

Note that Michael Paquier sent a patch implementing this in another
thread:
/messages/by-id/CAB7nPqTxF5dtxjEzB7xkJvOWxX8D_2atxmTu3PSnkhcWT_JY5A@mail.gmail.com

Ah, I had a feeling that this had come up recently, but failed to remember
that there was already a patch. Maybe we should just bump up the priority
of reviewing that patch.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12

Michael Paquier

michael.paquier@gmail.com

over 9 years ago

In reply to: Tom Lane (#11)

Re: Issue in pg_catalog.pg_indexes view definition

On Fri, Jul 15, 2016 at 3:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andreas Seltenreich <seltenreich@gmx.de> writes:

Tom Lane writes:

We've dealt with similar issues in places like pg_relation_size() by
making the functions return NULL instead of throwing an error for an
unmatched argument OID.

Note that Michael Paquier sent a patch implementing this in another
thread:
/messages/by-id/CAB7nPqTxF5dtxjEzB7xkJvOWxX8D_2atxmTu3PSnkhcWT_JY5A@mail.gmail.com

Ah, I had a feeling that this had come up recently, but failed to remember
that there was already a patch. Maybe we should just bump up the priority
of reviewing that patch.

Indeed, I didn't immediately recall that by looking at this thread.
That's registered in the next CF, so it is not lost.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers