Обсуждение: 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

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

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

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

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
https://www.postgresql.org/message-id/flat/CAHnozTjkOP8o0MqNZtuc5HgPD1tLRmTQvZAKY%2BRNNvOkmMbK0A%40mail.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

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:

https://www.postgresql.org/message-id/CAB7nPqTxF5dtxjEzB7xkJvOWxX8D_2atxmTu3PSnkhcWT_JY5A@mail.gmail.com

regards,
andreas

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:
> https://www.postgresql.org/message-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

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:
>> https://www.postgresql.org/message-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