BUG #5358: Throwing unexpected ERROR

"Gurjeet Singh" <singh.gurjeet@gmail.com> writes:

select relname, pg_relation_size( 'public."' || relname || '"' )/1024
from (select distinct relname
from (select r.relname, c.attname, c.attnotnull, t.typname
from pg_namespace as s, pg_class as r, pg_attribute as c, pg_type as t
where s.oid = r.relnamespace
and r.oid = c.attrelid
and c.atttypid = t.oid
and s.nspname = 'public'
and t.typname in ('bytea', 'text') ) as s1
) as s2
where pg_relation_size( 'public."' || relname || '"' ) <> 0;

ERROR: relation "public.pg_type" does not exist

That approach to generating a textual name for a relation is really
pretty unworkable: it's on the hairy edge of being vulnerable to
SQL injection attacks, not to mention being inefficient and unwieldy.
Just pass r.oid to pg_relation_size, instead.

regards, tom lane

On Tue, Mar 2, 2010 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Gurjeet Singh" <singh.gurjeet@gmail.com> writes:

select relname, pg_relation_size( 'public."' || relname || '"' )/1024
from (select distinct relname
from (select r.relname, c.attname, c.attnotnull, t.typname
from pg_namespace as s, pg_class as r, pg_attribute as c,

pg_type as t

where s.oid = r.relnamespace
and r.oid = c.attrelid
and c.atttypid = t.oid
and s.nspname = 'public'
and t.typname in ('bytea', 'text') ) as s1
) as s2
where pg_relation_size( 'public."' || relname || '"' ) <> 0;

ERROR: relation "public.pg_type" does not exist

That approach to generating a textual name for a relation is really
pretty unworkable: it's on the hairy edge of being vulnerable to
SQL injection attacks, not to mention being inefficient and unwieldy.
Just pass r.oid to pg_relation_size, instead.

I have gotten on to that path already, thanks for the advice.

This query will never be used by an application, so no fear of SQL injection
there. I was in the middle of a migration effort when I brewed this query.
The main inner query is what I started with to migrate only specific tables,
and the started slapping on outer queries to monitor the amount of data
already transferred. So I was rather surprised to see this error at a stage
where I did not expect it to fail.

IMHO the outer-most WHERE clause is being pushed through the subqueries when
it should not be. I tried to stop the optimizer from doing that and it seems
putting a LIMIT clause on S1 subquery make Postgres happy.

select relname, pg_relation_size( 'public."' || relname || '"' )/1024
from (select distinct relname
from (select r.relname, c.attname, c.attnotnull, t.typname
from pg_namespace as s, pg_class as r, pg_attribute as c,
pg_type as t
where s.oid = r.relnamespace
and r.oid = c.attrelid
and c.atttypid = t.oid
and s.nspname = 'public'
and t.typname in ('bytea', 'text') ) as s1 limit 1000
) as s2
where pg_relation_size( 'public."' || relname || '"' ) <> 0 ;

From SQL perspective there should be no difference between this query and
the one in the first post since there's only one qualifying record.
Predicate push-down is definitely a good optimization, but it should not
affect the result-set. I have no idea how to tell optimizer to stop such
push-downs.

I am leaning towards marking this as a bug.

Best regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.enterprisedb.com

singh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

On Tue, Mar 2, 2010 at 10:24 PM, Gurjeet Singh <singh.gurjeet@gmail.com>wrote:

On Tue, Mar 2, 2010 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Gurjeet Singh" <singh.gurjeet@gmail.com> writes:

select relname, pg_relation_size( 'public."' || relname || '"' )/1024
from (select distinct relname
from (select r.relname, c.attname, c.attnotnull, t.typname
from pg_namespace as s, pg_class as r, pg_attribute as c,

pg_type as t

where s.oid = r.relnamespace
and r.oid = c.attrelid
and c.atttypid = t.oid
and s.nspname = 'public'
and t.typname in ('bytea', 'text') ) as s1
) as s2
where pg_relation_size( 'public."' || relname || '"' ) <> 0;

ERROR: relation "public.pg_type" does not exist

That approach to generating a textual name for a relation is really
pretty unworkable: it's on the hairy edge of being vulnerable to
SQL injection attacks, not to mention being inefficient and unwieldy.
Just pass r.oid to pg_relation_size, instead.

I have gotten on to that path already, thanks for the advice.

This query will never be used by an application, so no fear of SQL
injection there. I was in the middle of a migration effort when I brewed
this query. The main inner query is what I started with to migrate only
specific tables, and the started slapping on outer queries to monitor the
amount of data already transferred. So I was rather surprised to see this
error at a stage where I did not expect it to fail.

IMHO the outer-most WHERE clause is being pushed through the subqueries
when it should not be. I tried to stop the optimizer from doing that and it
seems putting a LIMIT clause on S1 subquery make Postgres happy.

select relname, pg_relation_size( 'public."' || relname || '"' )/1024
from (select distinct relname
from (select r.relname, c.attname, c.attnotnull, t.typname
from pg_namespace as s, pg_class as r, pg_attribute as c,
pg_type as t
where s.oid = r.relnamespace
and r.oid = c.attrelid
and c.atttypid = t.oid
and s.nspname = 'public'
and t.typname in ('bytea', 'text') ) as s1 limit 1000

) as s2
where pg_relation_size( 'public."' || relname || '"' ) <> 0 ;

From SQL perspective there should be no difference between this query and
the one in the first post since there's only one qualifying record.
Predicate push-down is definitely a good optimization, but it should not
affect the result-set. I have no idea how to tell optimizer to stop such
push-downs.

I just realized that it is the subquery pull-up that is leading to this
problem, not predicate push-down. Sleeping over it does really help I guess
:)

So instead of the LIMIT 1000, OFFSET 0 clause is the right choice for
preventing subquery pull-up without affecting the results.

I don't think the optimizer has the push-down capabiity; I may be wrong.

I am leaning towards marking this as a bug.

Best regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.enterprisedb.com

singh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

On Wed, Mar 3, 2010 at 7:29 AM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:

I just realized that it is the subquery pull-up that is leading to this
problem, not predicate push-down. Sleeping over it does really help I guess
:)

So instead of the LIMIT 1000, OFFSET 0 clause is the right choice for
preventing subquery pull-up without affecting the results.

I don't think the optimizer has the push-down capabiity; I may be wrong.

Maybe I'm just dense, but I don't understand what you're complaining
about here. The SELECT DISTINCT already acts as an optimization
fence, so why would you need another one? And what problem would you
expect it to solve?

...Robert

On Wed, Mar 3, 2010 at 8:37 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Mar 3, 2010 at 7:29 AM, Gurjeet Singh <singh.gurjeet@gmail.com>
wrote:

I just realized that it is the subquery pull-up that is leading to this
problem, not predicate push-down. Sleeping over it does really help I

guess

:)

So instead of the LIMIT 1000, OFFSET 0 clause is the right choice for
preventing subquery pull-up without affecting the results.

I don't think the optimizer has the push-down capabiity; I may be wrong.

Maybe I'm just dense, but I don't understand what you're complaining
about here. The SELECT DISTINCT already acts as an optimization
fence, so why would you need another one? And what problem would you
expect it to solve?

I am complaining about the ERROR when I don't specify OFFSET or LIMIT.

The query isn't relevant. It is there just to illustrate the fact that two
supposedly equivalent forms of a query are not treated equivalent after all
by Postgres.

You don't put that OFFSET clause, you get an ERROR. You put in that OFFSET
clause and you get proper results.

I hope my complain is clearer now.

Best regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.enterprisedb.com

singh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

On Wed, Mar 3, 2010 at 8:48 AM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:

On Wed, Mar 3, 2010 at 8:37 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Mar 3, 2010 at 7:29 AM, Gurjeet Singh <singh.gurjeet@gmail.com>
wrote:

I just realized that it is the subquery pull-up that is leading to this
problem, not predicate push-down. Sleeping over it does really help I
guess
:)

So instead of the LIMIT 1000, OFFSET 0 clause is the right choice for
preventing subquery pull-up without affecting the results.

I don't think the optimizer has the push-down capabiity; I may be wrong.

Maybe I'm just dense, but I don't understand what you're complaining
about here.  The SELECT DISTINCT already acts as an optimization
fence, so why would you need another one?  And what problem would you
expect it to solve?

I am complaining about the ERROR when I don't specify OFFSET or LIMIT.

The query isn't relevant. It is there just to illustrate the fact that two
supposedly equivalent forms of a query are not treated equivalent after all
by Postgres.

You don't put that OFFSET clause, you get an ERROR. You put in that OFFSET
clause and you get proper results.

I hope my complain is clearer now.

It does seem a little weird, but I don't think we're likely to change
the behavior. The optimizer is allowed to reorder quals, and I don't
think we want to change that. Consider a very large table which has
an index on column b but not on column a, and the following query:

SELECT * FROM some_huge_table WHERE a = 1 AND b = 1

All other things being equal, we'll want to execute this query by
doing an index scan for rows with b = 1 and then checking whatever
comes back to see whether we also have a = 1. If we insisted that a =
1 had to be evaluated first, we'd have to scan the whole table.

Normally this kind of reordering doesn't actually affect the result of
the query because normally the quals that are being evaluated don't
have any side-effects, but in your query you've chosen something that
can throw an exception, so it's user-visible.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

Normally this kind of reordering doesn't actually affect the result of
the query because normally the quals that are being evaluated don't
have any side-effects, but in your query you've chosen something that
can throw an exception, so it's user-visible.

There's a pretty explicit disclaimer here:
http://www.postgresql.org/docs/8.4/static/sql-expressions.html#SYNTAX-EXPRESS-EVAL
that evaluation order is not guaranteed for WHERE clauses. While we
could try to tighten that up, I'll bet quite a lot that the howls of
pain from people whose queries stopped getting optimized would drown
out anyone who thought it was an improvement.

In this particular case I think you could make it better by putting the
unsafe function call in a subselect's output list, and placing the
restrictive condition in a WHERE clause above the subselect. However,
that's only safe because we have restrictions forbidding folding of
subselects whose output lists include volatile functions. The problem
here is *not* that pg_relation_size is volatile in the normal sense,
it's the possibility of throwing error on its input; so the fact that
the problem goes away seems a bit accidental. I don't know quite what
we might do about that. Marking functions as "could throw error"
doesn't seem especially helpful, because the set of functions not so
markable is durn close to empty.

regards, tom lane