Thread: isTempNamespaceInUse() is incorrect with its handling of MyBackendId

Hi all,

While reviewing some code in namespace.c, I have bumped into the
following issue introduced by 246a6c8:
diff --git a/src/backend/catalog/namespace.c
b/src/backend/catalog/namespace.c
index c82f9fc4b5..e70243a008 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -3235,8 +3235,8 @@ isTempNamespaceInUse(Oid namespaceId)

    backendId = GetTempNamespaceBackendId(namespaceId);

-   if (backendId == InvalidBackendId ||
-       backendId == MyBackendId)
+   /* No such temporary namespace? */
+   if (backendId == InvalidBackendId)
        return false;

The current logic of isTempNamespaceInUse() would cause a session
calling the routine to return always false if trying to check if its
own temporary session is in use, but that's incorrect.  It is actually
safe to remove the check on MyBackendId as the code would fall back on
a check equivalent to MyProc->tempNamespaceId a bit down as per the
attached, so let's fix it.

Thoughts?
--
Michael

On Mon, Jan 13, 2020 at 06:37:03PM +0900, Michael Paquier wrote:
> Hi all,
>
> While reviewing some code in namespace.c, I have bumped into the
> following issue introduced by 246a6c8:
> diff --git a/src/backend/catalog/namespace.c
> b/src/backend/catalog/namespace.c
> index c82f9fc4b5..e70243a008 100644
> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c
> @@ -3235,8 +3235,8 @@ isTempNamespaceInUse(Oid namespaceId)
>
>     backendId = GetTempNamespaceBackendId(namespaceId);
>
> -   if (backendId == InvalidBackendId ||
> -       backendId == MyBackendId)
> +   /* No such temporary namespace? */
> +   if (backendId == InvalidBackendId)
>         return false;
>
> The current logic of isTempNamespaceInUse() would cause a session
> calling the routine to return always false if trying to check if its
> own temporary session is in use, but that's incorrect.

Indeed.

> It is actually
> safe to remove the check on MyBackendId as the code would fall back on
> a check equivalent to MyProc->tempNamespaceId a bit down as per the
> attached, so let's fix it.
>
> Thoughts?

But that means an extraneous call to BackendIdGetProc() in that case, it seems
better to avoid it if we already have the information.

On Mon, Jan 13, 2020 at 01:09:01PM +0100, Julien Rouhaud wrote:
> But that means an extraneous call to BackendIdGetProc() in that
> case, it seems better to avoid it if we already have the information.

Note that you cannot make a direct comparison of the result from
GetTempNamespaceBackendId() with MyBackendId, because it is critical
to be able to handle the case of a session which has not created yet
an object on its own temp namespace (this way any temp objects from
previous connections which used the same backend slot can be marked as
orphaned and discarded by autovacuum, the whole point of 246a6c8).  So
in order to get a fast-exit path we could do the following:
- Add a routine GetTempNamespace which returns myTempNamespace (the
result can be InvalidOid).
- Add an assertion at the beginning of isTempNamespaceInUse() to make
sure that it never gets called with InvalidOid as input argument.
- Return true as a first step of GetTempNamespaceBackendId() if
namespaceId matches GetTempNamespace().

What do you think?
--
Michael

On Mon, Jan 13, 2020 at 10:14:52PM +0900, Michael Paquier wrote:
> On Mon, Jan 13, 2020 at 01:09:01PM +0100, Julien Rouhaud wrote:
> > But that means an extraneous call to BackendIdGetProc() in that
> > case, it seems better to avoid it if we already have the information.
>
> Note that you cannot make a direct comparison of the result from
> GetTempNamespaceBackendId() with MyBackendId, because it is critical
> to be able to handle the case of a session which has not created yet
> an object on its own temp namespace (this way any temp objects from
> previous connections which used the same backend slot can be marked as
> orphaned and discarded by autovacuum, the whole point of 246a6c8).

Oh right.

> So in order to get a fast-exit path we could do the following:
> - Add a routine GetTempNamespace which returns myTempNamespace (the
> result can be InvalidOid).
> - Add an assertion at the beginning of isTempNamespaceInUse() to make
> sure that it never gets called with InvalidOid as input argument.
> - Return true as a first step of GetTempNamespaceBackendId() if
> namespaceId matches GetTempNamespace().
>
> What do you think?

Well, since isTempNamespaceInUse is for now only called by autovacuum, and
shouldn't change soon, this really feels premature optimzation, so your
original approach looks better.

On Mon, Jan 13, 2020 at 02:56:13PM +0100, Julien Rouhaud wrote:
> Well, since isTempNamespaceInUse is for now only called by autovacuum, and
> shouldn't change soon, this really feels premature optimzation, so your
> original approach looks better.

Yes, I'd rather keep this routine in its simplest shape for now.  If
the optimization makes sense, though in most cases it won't because it
just helps sessions to detect faster their own temp schema, then let's
do it.  I'll let this patch aside for a couple of days to let others
comment on it, and if there are no objections, I'll commit the fix.
Thanks for the lookup!
--
Michael

On Tue, Jan 14, 2020 at 07:23:19AM +0900, Michael Paquier wrote:
> Yes, I'd rather keep this routine in its simplest shape for now.  If
> the optimization makes sense, though in most cases it won't because it
> just helps sessions to detect faster their own temp schema, then let's
> do it.  I'll let this patch aside for a couple of days to let others
> comment on it, and if there are no objections, I'll commit the fix.
> Thanks for the lookup!

For the archive's sake: this has been fixed with ac5bdf6, down to 11.
--
Michael