Re: Temporary tables prevent autovacuum, leading to XID wraparound

Hello.

At Wed, 18 Jul 2018 07:34:10 +0000, "Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> wrote in <0A3221C70F24FB45833433255569204D1FA538FD@G01JPEXMBYT05>
> From: Michael Paquier [mailto:michael@paquier.xyz]
> > +   /* Does the backend own the temp schema? */
> > +   if (proc->tempNamespaceId != namespaceID)
> > +       return false;
> > I have a very hard time believing that this is safe lock-less, and a spin
> > lock would be enough it seems.
> 
> The lwlock in BackendIdGetProc() flushes the CPU cache so that proc->tempNamespaceId reflects the latest value.  Or, do you mean another spinlock elsewhere?

It seems to be allowed that the series of checks on *proc results
in false-positive, which is the safer side for the usage, even it
is not atomically updated. Actually ->databaseId is written
without taking a lock.

> > +   /* Is the backend connected to this database? */
> > +   if (proc->databaseId != MyDatabaseId)
> > +       return false;
> > Wouldn't it be more interesting to do this cleanup as well if the backend
> > is *not* connected to the database autovacuum'ed?  This would make the
> > cleanup more aggresive, which is better.
> 
> IIUC, the above code does what you suggest.  proc->databaseId is the database the client is connected to, and MyDatabaseId is the database being autovacuumed (by this autovacuum worker.)

It seems that you're right. But maybe the comments in
backned_uses_temp_namespace should be written invsered. And the
comment on the last condition needs to be more detailed. For
example:

|  /* Not used if the backend is not on connection */
|  if (proc == NULL)
|  ..
|  /* Not used if the backend is on another database */
|  if (proc->databaseId !=  MyDatabaseID)
|  ..
|  /* 
|   * Not used if this backend has no temp namespace or one with
|   * different OID.  However we could reuse the namespce OID,
|   * tables in the old teblespace have been cleaned up at the
|   * creation time.  See InitTempTableNamespace.
|   */
| if (proc->tempNamespaceId != namespaceId)
...(is it correct?)

backend_uses_temp_namespace is taking two parameters but the
first one is always derived from the second. backendID doesn't
seem to be needed outside so just one parameter namespaceID is
needed.

There may be a case where classForm->relnamespace is not found in
catalog or nspname is corrupt, but the namespace is identified as
inactive and orphaned tables are cleaned up in the case so we
don't need to inform that to users..


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center