Re: Non-transactional pg_class, try 2

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> VACUUM FULL also refuses to operate on these tables, and ANALYZE
> silently skips them. Only plain VACUUM cleans them.

I wonder whether VACUUM FULL applied to an NT table shouldn't just act
like plain VACUUM instead. Probably not very important though.

> Note that you can DELETE from pg_ntclass. Not sure if we should
> disallow it somehow, because it's not easy to get out from that if you
> do.

No worse than DELETE FROM pg_class ;-). Please verify that the
aclcheck prohibitions on changing system catalogs are properly applied
to these catalogs too.

> There is one caveat that I'm worried about. I had to add a "typedef" to
> pg_class.h to put ItemPointerData in FormData_pg_class, because the C
> struct doesn't recognize the "tid" type, but the bootstrap type system
> does not recognize ItemPointerData as a valid type. I find this mighty
> ugly because it will have side effects whenever we #include pg_class.h
> (which is virtually anywhere, since that header is #included in htup.h
> which in turn is included almost everywhere). Suggestions welcome.
> Maybe this is not a problem.

Would it work to do

#define tid ItemPointerData
...
tid relntrans;
...
#undef tid
?

I'm not sure whether this will cause the right things to appear in the
.bki file, but if it does then the #undef would limit the scope of the
"tid" name.

In any case, the only thing uglier than a hack is an uncommented hack
;-) ... the typedef or macro needs to have a comments explaining what
and why.

The *real* problem with what you've done is that pg_class.h now depends
on having ItemPointerData defined before it's included, which creates an
inclusion ordering dependency that should not exist. If you stick with
either of these approaches then pg_class.h needs to #include whatever
defines ItemPointerData.

I notice that postgres.h defines a typedef for aclitem to work around a
similar issue. Is it reasonable to move ItemPointerData into postgres.h
so we could put the tid typedef beside the aclitem one?

> Other two caveats are:
> 1. During bootstrap, RelationBuildLocalRelation creates nailed relations
> with hardcoded TID=(0,1). This is because we don't have access to
> pg_class yet, so we can't find the real pointer; and furthermore, we are
> going to fix the entries later in the bootstrapping process.

This seems dangerous; can't you set it to InvalidItemPointer instead?
If it's not used before fixed, this doesn't matter, and if someone
*does* try to use it, that will catch the problem.

> 2. The whole VACUUM/VACUUM FULL/ANALYZE relation list stuff is pretty
> ugly as well; and autovacuum is skipping pg_ntclass (really all
> non-transactional catalogs) altogether. We could improve the situation
> by introducing some sort of struct like {relid, relkind}, so that
> vacuum_rel could know what relkind to expect, and it could skip
> non-transactional catalogs cleanly in vacuum full and analyze.

Need to do something about this. pg_ntclass will contain XIDs (of
inserting/deleting transactions) so it MUST get vacuumed to be sure
we don't expose ourselves to XID wrap problems.

regards, tom lane