Re: problems with table corruption continued

> This is trying to get rid of the original copy of a tuple that's been
> moved to another page. The problem is that your index
> function causes a
> table scan, which means that by the time control gets here,
> someone else
> has looked at this tuple and marked it good --- so the initial test of
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> HEAP_XMIN_COMMITTED fails, and the tuple is never removed!
>
> I would say that it's incorrect for vacuum.c to assume that
> HEAP_XMIN_COMMITTED can't become set on HEAP_MOVED_OFF/HEAP_MOVED_IN
> tuples during the course of vacuum's processing; after all, the xmin
> definitely does refer to a committed xact, and we can't realistically
> assume that we know what processing will be induced by user-defined
> index functions. Vadim, what do you think? How should we fix this?

But it's incorrect for table scan to mark tuple as good neither.
Looks like we have to add checks for the case
TransactionIdIsCurrentTransactionId(tuple->t_cmin) when
there is HEAP_MOVED_OFF or HEAP_MOVED_IN in t_infomask to
all HeapTupleSatisfies* in tqual.c as we do in
HeapTupleSatisfiesDirty - note comments about uniq btree-s there.

Vadim

"Mikheev, Vadim" <vmikheev(at)SECTORBASE(dot)COM> writes:
>> I would say that it's incorrect for vacuum.c to assume that
>> HEAP_XMIN_COMMITTED can't become set on HEAP_MOVED_OFF/HEAP_MOVED_IN
>> tuples during the course of vacuum's processing; after all, the xmin
>> definitely does refer to a committed xact, and we can't realistically
>> assume that we know what processing will be induced by user-defined
>> index functions. Vadim, what do you think? How should we fix this?

> But it's incorrect for table scan to mark tuple as good neither.

Oh, that makes sense.

> Looks like we have to add checks for the case
> TransactionIdIsCurrentTransactionId(tuple->t_cmin) when
> there is HEAP_MOVED_OFF or HEAP_MOVED_IN in t_infomask to
> all HeapTupleSatisfies* in tqual.c as we do in
> HeapTupleSatisfiesDirty - note comments about uniq btree-s there.

Sounds like a plan. Do you want to work on this, or shall I?

regards, tom lane

> -----Original Message-----
> From: Tom Lane
>
> "Mikheev, Vadim" <vmikheev(at)SECTORBASE(dot)COM> writes:
> >> I would say that it's incorrect for vacuum.c to assume that
> >> HEAP_XMIN_COMMITTED can't become set on HEAP_MOVED_OFF/HEAP_MOVED_IN
> >> tuples during the course of vacuum's processing; after all, the xmin
> >> definitely does refer to a committed xact, and we can't realistically
> >> assume that we know what processing will be induced by user-defined
> >> index functions. Vadim, what do you think? How should we fix this?
>
> > But it's incorrect for table scan to mark tuple as good neither.
>
> Oh, that makes sense.
>
> > Looks like we have to add checks for the case
> > TransactionIdIsCurrentTransactionId(tuple->t_cmin) when
> > there is HEAP_MOVED_OFF or HEAP_MOVED_IN in t_infomask to
> > all HeapTupleSatisfies* in tqual.c as we do in
> > HeapTupleSatisfiesDirty - note comments about uniq btree-s there.

Should the change be TransactionIdIsInProgress(tuple->t_cmin) ?

The cause of Brian's issue was exactly what I was afraid of.
VACUUM is guarded by AccessExclusive lock but IMHO we
shouldn't rely on it too heavily.

regards,
Hiroshi Inoue

"Hiroshi Inoue" <Inoue(at)tpf(dot)co(dot)jp> writes:
> Should the change be TransactionIdIsInProgress(tuple->t_cmin) ?

I'd be willing to do
if (tuple->t_cmin is my XID)
do something;
Assert(!TransactionIdIsInProgress(tuple->t_cmin));
if that makes you feel better. But anything that's scanning
a table exclusive-locked by another transaction is broken.
(BTW: contrib/pgstattuple is thus broken. Will fix.)

regards, tom lane

Tom Lane wrote:
>
> "Hiroshi Inoue" <Inoue(at)tpf(dot)co(dot)jp> writes:
> > Should the change be TransactionIdIsInProgress(tuple->t_cmin) ?
>
> I'd be willing to do
> if (tuple->t_cmin is my XID)
> do something;
> Assert(!TransactionIdIsInProgress(tuple->t_cmin));
> if that makes you feel better.

It's useless in hard to reproduce cases.
I've thought but given up many times to propose this
change and my decision seems to have been right because
I see only *Assert* even after this issue.

> But anything that's scanning
> a table exclusive-locked by another transaction is broken.
> (BTW: contrib/pgstattuple is thus broken. Will fix.)

It seems dangerous to me to rely only on developers'
carefulness. There are some places where relations
are opened with NoLock. We had better change them.
I once examined them but AFAIR there are few cases
when they are really opened with NoLock. In most
cases they are already opened previously with other
lock modes. I don't remember well if there was a
really dangerous(scan an relation opened with NoLock)
place.

regards,
Hiroshi Inoue

Hiroshi Inoue <Inoue(at)tpf(dot)co(dot)jp> writes:
> I don't remember well if there was a really dangerous(scan an relation
> opened with NoLock) place.

I have just looked for that, and the only such place is in the new
contrib module pgstattuple. This is clearly broken, since there is
nothing stopping someone from (eg) dropping the relation while
pgsstattuple is scanning it. I think it should get AccessShareLock
on the relation.

The ri_triggers code has a lot of places that open things NoLock,
but it only looks into the relcache entry and doesn't try to scan
the relation. Nonetheless that code bothers me; we could be using
an obsolete relcache entry if someone has just committed an ALTER
TABLE on the relation. Some of the cases may be safe because a lock
is held higher up (eg, on the table from which the trigger was fired)
but I doubt they all are.

regards, tom lane

On Tue, 18 Dec 2001, Tom Lane wrote:

> The ri_triggers code has a lot of places that open things NoLock,
> but it only looks into the relcache entry and doesn't try to scan
> the relation. Nonetheless that code bothers me; we could be using
> an obsolete relcache entry if someone has just committed an ALTER
> TABLE on the relation. Some of the cases may be safe because a lock
> is held higher up (eg, on the table from which the trigger was fired)
> but I doubt they all are.

Probably not, since it looks like that's being done for the other table of
the constraint (not the one on which the trigger was fired).

Stephan Szabo wrote:
>
> On Tue, 18 Dec 2001, Tom Lane wrote:
>
> > The ri_triggers code has a lot of places that open things NoLock,
> > but it only looks into the relcache entry and doesn't try to scan
> > the relation. Nonetheless that code bothers me; we could be using
> > an obsolete relcache entry if someone has just committed an ALTER
> > TABLE on the relation. Some of the cases may be safe because a lock
> > is held higher up (eg, on the table from which the trigger was fired)
> > but I doubt they all are.
>
> Probably not, since it looks like that's being done for the other table of
> the constraint (not the one on which the trigger was fired).

If a lock is held already, acquiring an AccessShareLock
would cause no addtional conflict. I don't see any reason
to walk a tightrope with NoLock intentionally.

regards,
Hiroshi Inoue

On Wed, 19 Dec 2001, Hiroshi Inoue wrote:

> Stephan Szabo wrote:
> >
> > On Tue, 18 Dec 2001, Tom Lane wrote:
> >
> > > The ri_triggers code has a lot of places that open things NoLock,
> > > but it only looks into the relcache entry and doesn't try to scan
> > > the relation. Nonetheless that code bothers me; we could be using
> > > an obsolete relcache entry if someone has just committed an ALTER
> > > TABLE on the relation. Some of the cases may be safe because a lock
> > > is held higher up (eg, on the table from which the trigger was fired)
> > > but I doubt they all are.
> >
> > Probably not, since it looks like that's being done for the other table of
> > the constraint (not the one on which the trigger was fired).
>
> If a lock is held already, acquiring an AccessShareLock
> would cause no addtional conflict. I don't see any reason
> to walk a tightrope with NoLock intentionally.

I don't know why NoLock was used there, I was just pointing out that the
odds of a lock being held higher up is probably low.