Re: WIP patch: Improve relation size functions such as pg_relation_size() to avoid producing an error when called against a no longer visible relation

Attached is a patch that addresses the todo item "Improve relation
size functions such as pg_relation_size() to avoid producing an error
when called against a no longer visible relation."

http://archives.postgresql.org/pgsql-general/2010-10/msg00332.php

Instead of returning an error, they now return NULL if the OID is
found in pg_class when using SnapshotAny. I only applied it to 4
functions: select pg_relation_size, pg_total_relation_size,
pg_table_size and pg_indexes_size. These are the ones that were using
relation_open(). I changed them to using try_relation_open(). For
three of them I had to move the try_relation_open() call up one level
in the call stack and change the parameter types for some support
functions from Oid to Relation. They now also call a new function
relation_recently_dead() which is what checks for relation in
SnapshotAny. All regression tests passed.

Is SnapshotAny the snapshot I should be using? It seems to get the
correct results. I can drop a table and I get NULL. Then after a
vacuumdb it returns an error.

On Sat, Dec 17, 2011 at 3:52 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
> Attached is a patch that addresses the todo item "Improve relation
> size functions such as pg_relation_size() to avoid producing an error
> when called against a no longer visible relation."
>
> http://archives.postgresql.org/pgsql-general/2010-10/msg00332.php
>
> Instead of returning an error, they now return NULL if the OID is
> found in pg_class when using SnapshotAny. I only applied it to 4
> functions: select pg_relation_size, pg_total_relation_size,
> pg_table_size and pg_indexes_size. These are the ones that were using
> relation_open(). I changed them to using try_relation_open(). For
> three of them I had to move the try_relation_open() call up one level
> in the call stack and change the parameter types for some support
> functions from Oid to Relation. They now also call a new function
> relation_recently_dead() which is what checks for relation in
> SnapshotAny. All regression tests passed.
>
> Is SnapshotAny the snapshot I should be using? It seems to get the
> correct results. I can drop a table and I get NULL. Then after a
> vacuumdb it returns an error.

Something I'd like to point out myself is that I need to be using
ObjectIdAttributeNumber instead of Anum_pg_class_relfilenode. Probably
just luck that I got the intended results before. This will also
change the logic in a few other places.

Still not clear on the semantics of Snapshot{Any|Self|Now|Dirty}.

On Mon, Dec 19, 2011 at 1:27 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
> On Sat, Dec 17, 2011 at 3:52 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>> Attached is a patch that addresses the todo item "Improve relation
>> size functions such as pg_relation_size() to avoid producing an error
>> when called against a no longer visible relation."
>>
>> http://archives.postgresql.org/pgsql-general/2010-10/msg00332.php
>>
>> Instead of returning an error, they now return NULL if the OID is
>> found in pg_class when using SnapshotAny. I only applied it to 4
>> functions: select pg_relation_size, pg_total_relation_size,
>> pg_table_size and pg_indexes_size. These are the ones that were using
>> relation_open(). I changed them to using try_relation_open(). For
>> three of them I had to move the try_relation_open() call up one level
>> in the call stack and change the parameter types for some support
>> functions from Oid to Relation. They now also call a new function
>> relation_recently_dead() which is what checks for relation in
>> SnapshotAny. All regression tests passed.
>>
>> Is SnapshotAny the snapshot I should be using? It seems to get the
>> correct results. I can drop a table and I get NULL. Then after a
>> vacuumdb it returns an error.
>
> Something I'd like to point out myself is that I need to be using
> ObjectIdAttributeNumber instead of Anum_pg_class_relfilenode. Probably
> just luck that I got the intended results before. This will also
> change the logic in a few other places.
>
> Still not clear on the semantics of Snapshot{Any|Self|Now|Dirty}.

I've attached the updated version of my patch with the changes
mentioned in the previous email.

On Sat, Dec 17, 2011 at 3:52 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
> Is SnapshotAny the snapshot I should be using? It seems to get the
> correct results. I can drop a table and I get NULL. Then after a
> vacuumdb it returns an error.

The suggestion on the original thread was to use SnapshotDirty or the
caller's MVCC snapshot. SnapshotAny doesn't seem like a good idea.
We don't want to include rows from, say, transactions that have rolled
back. And SnapshotAny rows can stick around for a long time - weeks,
months. If a table is only occasionally updated, autovacuum may not
process it for a long time.

On the other hand, I think using SnapshotDirty is going to work out so
well: isn't there a race condition? The caller's MVCC snapshot seems
better, but I still see pitfalls. Who is to say that the immediate
caller has the same snapshot as the scan? I'm thinking of cases
involving things like CTEs and nested function calls.

I'm wondering if we oughta just return NULL and be done with it. If
SELECT select 1241241241::regclass doesn't choke, I'm not sure select
pg_relation_size(1241241241) ought to either. It's not like the user
is going to see NULL and have no clue that the relation wasn't found.
At the very worst they might think it's empty.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I'm wondering if we oughta just return NULL and be done with it.

+1. There are multiple precedents for that sort of response, which we
introduced exactly so that "SELECT some_function(oid) FROM some_catalog"
wouldn't fail just because one of the rows had gotten deleted by the
time the scan got to it. I don't think it's necessary for the
relation-size functions to be any smarter. Indeed, I'd assumed that's
all that Phil's patch did, since I'd not looked closer till just now.

regards, tom lane

On Thu, Dec 22, 2011 at 1:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I'm wondering if we oughta just return NULL and be done with it.
>
> +1.  There are multiple precedents for that sort of response, which we
> introduced exactly so that "SELECT some_function(oid) FROM some_catalog"
> wouldn't fail just because one of the rows had gotten deleted by the
> time the scan got to it.  I don't think it's necessary for the
> relation-size functions to be any smarter.  Indeed, I'd assumed that's
> all that Phil's patch did, since I'd not looked closer till just now.
>
>                        regards, tom lane

Here it is without the checking for recently dead. If it can't open
the relation it simply returns NULL.

On Thu, Dec 22, 2011 at 2:02 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
> On Thu, Dec 22, 2011 at 1:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> I'm wondering if we oughta just return NULL and be done with it.
>>
>> +1.  There are multiple precedents for that sort of response, which we
>> introduced exactly so that "SELECT some_function(oid) FROM some_catalog"
>> wouldn't fail just because one of the rows had gotten deleted by the
>> time the scan got to it.  I don't think it's necessary for the
>> relation-size functions to be any smarter.  Indeed, I'd assumed that's
>> all that Phil's patch did, since I'd not looked closer till just now.
>
> Here it is without the checking for recently dead. If it can't open
> the relation it simply returns NULL.

I think we probably ought to make pg_database_size() and
pg_tablespace_size() behave similarly.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Dec 22, 2011 at 3:19 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Dec 22, 2011 at 2:02 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>> On Thu, Dec 22, 2011 at 1:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> I'm wondering if we oughta just return NULL and be done with it.
>>>
>>> +1.  There are multiple precedents for that sort of response, which we
>>> introduced exactly so that "SELECT some_function(oid) FROM some_catalog"
>>> wouldn't fail just because one of the rows had gotten deleted by the
>>> time the scan got to it.  I don't think it's necessary for the
>>> relation-size functions to be any smarter.  Indeed, I'd assumed that's
>>> all that Phil's patch did, since I'd not looked closer till just now.
>>
>> Here it is without the checking for recently dead. If it can't open
>> the relation it simply returns NULL.
>
> I think we probably ought to make pg_database_size() and
> pg_tablespace_size() behave similarly.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

Changes added.

On 23.12.2011 02:01, Phil Sorber wrote:
> On Thu, Dec 22, 2011 at 3:19 PM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Dec 22, 2011 at 2:02 PM, Phil Sorber<phil(at)omniti(dot)com> wrote:
>>> On Thu, Dec 22, 2011 at 1:33 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>>>>> I'm wondering if we oughta just return NULL and be done with it.
>>>>
>>>> +1. There are multiple precedents for that sort of response, which we
>>>> introduced exactly so that "SELECT some_function(oid) FROM some_catalog"
>>>> wouldn't fail just because one of the rows had gotten deleted by the
>>>> time the scan got to it. I don't think it's necessary for the
>>>> relation-size functions to be any smarter. Indeed, I'd assumed that's
>>>> all that Phil's patch did, since I'd not looked closer till just now.
>>>
>>> Here it is without the checking for recently dead. If it can't open
>>> the relation it simply returns NULL.
>>
>> I think we probably ought to make pg_database_size() and
>> pg_tablespace_size() behave similarly.
>
> Changes added.

Looks good to me, committed. I added a sentence to the docs mentioning
the new behavior, and also a code comment to explain why returning NULL
is better than throwing an error.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com