Thread: WIP patch: Improve relation size functions such as pg_relation_size() to avoid producing an error when called against a no longer visible relation
WIP patch: Improve relation size functions such as pg_relation_size() to avoid producing an error when called against a no longer visible relation
From
Phil Sorber
Date:
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.
Attachment
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
From
Phil Sorber
Date:
On Sat, Dec 17, 2011 at 3:52 PM, Phil Sorber <phil@omniti.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}.
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
From
Phil Sorber
Date:
On Mon, Dec 19, 2011 at 1:27 PM, Phil Sorber <phil@omniti.com> wrote: > On Sat, Dec 17, 2011 at 3:52 PM, Phil Sorber <phil@omniti.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.
Attachment
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
From
Robert Haas
Date:
On Sat, Dec 17, 2011 at 3:52 PM, Phil Sorber <phil@omniti.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
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
From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.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
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
From
Phil Sorber
Date:
On Thu, Dec 22, 2011 at 1:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.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.
Attachment
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
From
Robert Haas
Date:
On Thu, Dec 22, 2011 at 2:02 PM, Phil Sorber <phil@omniti.com> wrote: > On Thu, Dec 22, 2011 at 1:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Robert Haas <robertmhaas@gmail.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
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
From
Phil Sorber
Date:
On Thu, Dec 22, 2011 at 3:19 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Dec 22, 2011 at 2:02 PM, Phil Sorber <phil@omniti.com> wrote: >> On Thu, Dec 22, 2011 at 1:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Robert Haas <robertmhaas@gmail.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.
Attachment
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
From
Heikki Linnakangas
Date:
On 23.12.2011 02:01, Phil Sorber wrote: > On Thu, Dec 22, 2011 at 3:19 PM, Robert Haas<robertmhaas@gmail.com> wrote: >> On Thu, Dec 22, 2011 at 2:02 PM, Phil Sorber<phil@omniti.com> wrote: >>> On Thu, Dec 22, 2011 at 1:33 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >>>> Robert Haas<robertmhaas@gmail.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