Re: mvcc catalo gsnapshots and TopTransactionContext

Noah Misch <noah(at)leadboat(dot)com> writes:
> The following test case reliably hits this new assertion:

> create table t (c int);
> begin;
> create index on t (c);
> savepoint q;
> insert into t values (1);
> select 1/0;

As of commit ac8bc3b6e4, this test case no longer triggers the assertion,
because it depends on the INSERT issuing a relcache invalidation request
against t's index. btree used to do that when changing the index
metapage, but no longer does. The underlying problem is certainly still
there, though, and can be triggered with this slightly more complex test:

create or replace function inverse(int) returns float8 as
$$
begin
analyze t1;
return 1::float8/$1;
exception
when division_by_zero then return 0;
end$$ language plpgsql volatile;

drop table if exists t1;

create table t1 (c float8 unique);
insert into t1 values (1);
insert into t1 values (inverse(0));

Here, the ANALYZE triggers a relcache inval within the subtransaction
established by the function's BEGIN/EXCEPTION block, and then we abort
that subtransaction with a zero-divide, so you end up at the same place
as with the older example.

Of note is that we still have to have an index on t1; remove that,
no assert. This is a bit surprising since the ANALYZE certainly causes
a relcache flush on the table not just the index. The reason turns out
to be that for a simple table like this, relcache entry rebuild does not
involve consulting any syscache: we load the pg_class row with a systable
scan on pg_class, and build the tuple descriptor using a systable scan on
pg_attribute, and we're done. IIRC this was done this way intentionally
to avoid duplicative caching of the pg_class and pg_attribute rows.
However, RelationReloadIndexInfo uses the syscaches with enthusiasm, so
you will hit the Assert in question if you're trying to rebuild an index;
and you possibly could hit it for a table if you have a more complicated
table definition, such as one with rules.

Of course, a direct system catalog scan is certainly no safer in an
aborted transaction than the one that catcache.c is refusing to do.
Therefore, in my opinion, relcache.c ought also to be doing an
Assert(IsTransactionState()), at least in ScanPgRelation and perhaps
everywhere that it does catalog scans.

I stuck such an Assert in ScanPgRelation, and verified that it doesn't
break any existing regression tests --- although of course the above
test case still fails, and now even without declaring an index.

Barring objections I'll go commit that. It's a bit pointless to be
Asserting that catcache.c does nothing unsafe when relcache.c does
the same things without any such test.

(Alternatively, maybe we should centralize the asserting in
systable_beginscan or some such place?)

regards, tom lane