Thread: Isn't init_irels() dangerous ?

Hi all,

In InitPostgres()(postinit.c) I see the following code.
       RelationCacheInitialize();    /* pre-allocated reldescs created here
*/       InitializeTransactionSystem(); /* pg_log,etc init/crash recovery
here */

init_irels() is at the end of RelationCacheInitialize() and
accesses system tables to build some system index
relations. However InitializeTransactionSystem() isn't
called at this point and so TransactionIdDidCommit()
always returns true. Time qualification doesn't work
properly under such a situation.
It seems that init_irels() should be called after
InitializeTransactionSystem() was called.

Comments ?

Regards.
Hiroshi Inoue

"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> It seems that init_irels() should be called after
> InitializeTransactionSystem() was called.

Can we just swap the order of the RelationCacheInitialize() and
InitializeTransactionSystem() calls in InitPostgres?  If that
works, I'd have no objection.
        regards, tom lane

Tom Lane wrote:
> 
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> > It seems that init_irels() should be called after
> > InitializeTransactionSystem() was called.
> 
> Can we just swap the order of the RelationCacheInitialize() and
> InitializeTransactionSystem() calls in InitPostgres?  If that
> works, I'd have no objection.
>

It doesn't work. InitializeTransactionSystem() requires
pg_log/pg_variable relations which are already built in 
RelationCacheInitialize(). A few critical relations
including pg_log/pg_variable are built in RelationCache
Initialize() without touching database. It's OK but
init_irels() touches system tables to build a few
critical index relations. IMHO init_irels() should
be separated from RelationCacheInitialize().

In the meantime,I have another anxiety. init_irels()
(RelationCacheInitialize()) seems to be called while 
Locking is disabled. This seems to mean that init_irels()
could access to system tables even when they are in
vacuum. HeapTupleSatisfiesXXXX() doesn't seem to take
such cases into account except HeapTupleSatisfiesDirty().
HeapTupleSatisfiesXXXX() sets HEAP_XMIN_COMMITTED or
HEAP_XMIN_INVALID mask for HEAP_MOVED_IN(OFF) tuples.

Regards.
Hiroshi Inoue

Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Tom Lane wrote:
>> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>>>> It seems that init_irels() should be called after
>>>> InitializeTransactionSystem() was called.
>> 
>> Can we just swap the order of the RelationCacheInitialize() and
>> InitializeTransactionSystem() calls in InitPostgres?  If that
>> works, I'd have no objection.

> It doesn't work. InitializeTransactionSystem() requires
> pg_log/pg_variable relations which are already built in 
> RelationCacheInitialize().

OK.  Second proposal: do the init_irels() call in
RelationCacheInitializePhase2().  I've just looked through the
other stuff that's done in between, and I don't think any of it
needs valid relcache entries.

> In the meantime,I have another anxiety. init_irels()
> (RelationCacheInitialize()) seems to be called while 
> Locking is disabled.

This should fix that problem, too.
        regards, tom lane

Tom Lane wrote:
> 
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Tom Lane wrote:
> >> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> >>>> It seems that init_irels() should be called after
> >>>> InitializeTransactionSystem() was called.
> >>
> >> Can we just swap the order of the RelationCacheInitialize() and
> >> InitializeTransactionSystem() calls in InitPostgres?  If that
> >> works, I'd have no objection.
> 
> > It doesn't work. InitializeTransactionSystem() requires
> > pg_log/pg_variable relations which are already built in
> > RelationCacheInitialize().
> 
> OK.  Second proposal: do the init_irels() call in
> RelationCacheInitializePhase2().  I've just looked through the
> other stuff that's done in between, and I don't think any of it
> needs valid relcache entries.
> 

Oops, I neglected to reply "agreed", sorry.
It would be much safer for init_irels() to be called
in a proper transaction than the current implementation.

Regards.
Hiroshi Inoue

Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Tom Lane wrote:
>> OK.  Second proposal: do the init_irels() call in
>> RelationCacheInitializePhase2().  I've just looked through the
>> other stuff that's done in between, and I don't think any of it
>> needs valid relcache entries.

> Oops, I neglected to reply "agreed", sorry.
> It would be much safer for init_irels() to be called
> in a proper transaction than the current implementation.

Fine.  Were you going to do it, or do you want me to?
        regards, tom lane

Tom Lane wrote:
> 
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Tom Lane wrote:
> >> OK.  Second proposal: do the init_irels() call in
> >> RelationCacheInitializePhase2().  I've just looked through the
> >> other stuff that's done in between, and I don't think any of it
> >> needs valid relcache entries.
> 
> > Oops, I neglected to reply "agreed", sorry.
> > It would be much safer for init_irels() to be called
> > in a proper transaction than the current implementation.
> 
> Fine.  Were you going to do it, or do you want me to?
>

It would only need to change a few lines.
OK, I will do it. 

Regards.
Hiroshi Inoue