Re: SET WITHOUT OIDS and VACUUM badness?

This is what we did:

0. BEGIN;

1. ALTER TABLE ... SET WITHOUT OIDS

2. A bunch of things are selected out of this table and inserted into
another (using INSERT ... SELECT)

3. An index is created on a timestamp field on this table

4. Then there's an update on a related table, that selects stuff from
this table.

5. Renames a column

6. Drops a constraint

7. Adds a foreign key

8. Drops 8 columns

9. Drops 2 indexes

10. Drops 3 triggers

11. Then a tsearch 'txtidx' field is updated, and then cancelled halfway
through

12. ROLLBACK;

13. VACUUM FULL forums_posts;

Then we get thousands of these:

WARNING: relation "forums_posts" TID 22763/10: OID is invalid
WARNING: relation "forums_posts" TID 22763/11: OID is invalid
WARNING: relation "forums_posts" TID 22763/12: OID is invalid
WARNING: relation "forums_posts" TID 22763/13: OID is invalid
WARNING: relation "forums_posts" TID 22763/14: OID is invalid
WARNING: relation "forums_posts" TID 22763/15: OID is invalid
WARNING: relation "forums_posts" TID 22763/16: OID is invalid
WARNING: relation "forums_posts" TID 22763/17: OID is invalid
WARNING: relation "forums_posts" TID 22764/1: OID is invalid
WARNING: relation "forums_posts" TID 22764/2: OID is invalid
WARNING: relation "forums_posts" TID 22764/3: OID is invalid
WARNING: relation "forums_posts" TID 22764/4: OID is invalid

This seems to be reproducible...

Chris

On Wed, 21 Jan 2004, Christopher Kings-Lynne wrote:

> This seems to be reproducible...

Here is a smaller example that show the problem:

CREATE TABLE foo (a INT);

BEGIN;
ALTER TABLE foo SET WITHOUT OIDS;
INSERT INTO foo values (5);
ROLLBACK;

VACUUM FULL foo;

It's easy to guess what is causing this, but I'll leave that to the person
that wants to fix it.

--
/Dennis Björklund

On Wed, 21 Jan 2004, Christopher Kings-Lynne wrote:

> This is what we did:
>
> 0. BEGIN;
>
> 1. ALTER TABLE ... SET WITHOUT OIDS

> 12. ROLLBACK;
>
> 13. VACUUM FULL forums_posts;

The problem here is that this conditional doesn't take into account the
change in state which the above transaction causes:

if (onerel->rd_rel->relhasoids &&
!OidIsValid(HeapTupleGetOid(&tuple)))

Tuples inserted after step one have no (valid) OID. However, since we
rollback, the change to pg_class.relhasoids => 'f' is rolled back. The
only solution I can think of is removing the test or storing relhasoids as
a per tuple flag (argh).

Gavin

On Wed, 21 Jan 2004, Gavin Sherry wrote:

> On Wed, 21 Jan 2004, Christopher Kings-Lynne wrote:
>
> > This is what we did:
> >
> > 0. BEGIN;
> >
> > 1. ALTER TABLE ... SET WITHOUT OIDS
>
> > 12. ROLLBACK;
> >
> > 13. VACUUM FULL forums_posts;
>
> The problem here is that this conditional doesn't take into account the
> change in state which the above transaction causes:
>
> if (onerel->rd_rel->relhasoids &&
> !OidIsValid(HeapTupleGetOid(&tuple)))
>
> Tuples inserted after step one have no (valid) OID. However, since we
> rollback, the change to pg_class.relhasoids => 'f' is rolled back. The
> only solution I can think of is removing the test or storing relhasoids as
> a per tuple flag (argh).

What am I talking about. Can't we test for:

(&tuple)->t_infomask & HEAP_HASOID

Instead of:

onerel->rd_rel->relhasoids

Gavin

Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> What am I talking about. Can't we test for:
> (&tuple)->t_infomask & HEAP_HASOID
> Instead of:
> onerel->rd_rel->relhasoids

ISTM the point of the check is to detect rows that are out of sync with
the relation's relhasoids flag, so we might as well just get rid of the
check entirely as do that.

I'm not averse to dropping the check, but if we want to keep it, I'd be
inclined to restrict it to live tuples.

regards, tom lane

Gavin Sherry wrote:
> On Wed, 21 Jan 2004, Gavin Sherry wrote:
>
> > On Wed, 21 Jan 2004, Christopher Kings-Lynne wrote:
> >
> > > This is what we did:
> > >
> > > 0. BEGIN;
> > >
> > > 1. ALTER TABLE ... SET WITHOUT OIDS
> >
> > > 12. ROLLBACK;
> > >
> > > 13. VACUUM FULL forums_posts;
> >
> > The problem here is that this conditional doesn't take into account the
> > change in state which the above transaction causes:
> >
> > if (onerel->rd_rel->relhasoids &&
> > !OidIsValid(HeapTupleGetOid(&tuple)))
> >
> > Tuples inserted after step one have no (valid) OID. However, since we
> > rollback, the change to pg_class.relhasoids => 'f' is rolled back. The
> > only solution I can think of is removing the test or storing relhasoids as
> > a per tuple flag (argh).
>
> What am I talking about. Can't we test for:
>
> (&tuple)->t_infomask & HEAP_HASOID
>
> Instead of:
>
> onerel->rd_rel->relhasoids

I can confirm we still have this bug:

test=> CREATE TABLE foo (a INT);
CREATE TABLE
test=> BEGIN;
BEGIN
test=> ALTER TABLE foo SET WITHOUT OIDS;
INSERT INTO foo values (5);
ROLLBACK;

VACUUM FULL foo;
ALTER TABLE
test=> INSERT INTO foo values (5);
INSERT 0 1
test=> ROLLBACK;
ROLLBACK
test=>
test=> VACUUM FULL foo;
WARNING: relation "foo" TID 0/1: OID is invalid
VACUUM

Anyone want to fix it?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Wed, 11 Feb 2004, Bruce Momjian wrote:

> Gavin Sherry wrote:
> > On Wed, 21 Jan 2004, Gavin Sherry wrote:
> >
> > > On Wed, 21 Jan 2004, Christopher Kings-Lynne wrote:
> > >
> > > > This is what we did:
> > > >
> > > > 0. BEGIN;
> > > >
> > > > 1. ALTER TABLE ... SET WITHOUT OIDS
> > >
> > > > 12. ROLLBACK;
> > > >
> > > > 13. VACUUM FULL forums_posts;
> > >
> > > The problem here is that this conditional doesn't take into account the
> > > change in state which the above transaction causes:
> > >
> > > if (onerel->rd_rel->relhasoids &&
> > > !OidIsValid(HeapTupleGetOid(&tuple)))
> > >
> > > Tuples inserted after step one have no (valid) OID. However, since we
> > > rollback, the change to pg_class.relhasoids => 'f' is rolled back. The
> > > only solution I can think of is removing the test or storing relhasoids as
> > > a per tuple flag (argh).
> >
> > What am I talking about. Can't we test for:
> >
> > (&tuple)->t_infomask & HEAP_HASOID
> >
> > Instead of:
> >
> > onerel->rd_rel->relhasoids
>
> I can confirm we still have this bug:
>

[sample]

Tom had two suggestions later in the thread:

http://archives.postgresql.org/pgsql-hackers/2004-01/msg00467.php

Gavin

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Anyone want to fix it?

Done.

regards, tom lane