Thread: Can avoid list_copy in recomputeNamespacePath() conditionally?

Can avoid list_copy in recomputeNamespacePath() conditionally?

From

amul sul

Date:

31 October 2019, 11:41:03

Hi all,

I wondered can we have a shortcut somewhat similar to following POC
in recomputeNamespacePath () when the recomputed path is the same as the
previous baseSearchPath/activeSearchPath :

== POC patch ==
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index e251f5a9fdc..b25ef489e47 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -3813,6 +3813,9 @@ recomputeNamespacePath(void)
!list_member_oid(oidlist, myTempNamespace))
oidlist = lcons_oid(myTempNamespace, oidlist);

+ /* TODO: POC */
+ if (equal(oidlist, baseSearchPath))
+ return;
/*
* Now that we've successfully built the new list of namespace OIDs, save
* it in permanent storage.
== POC patch end ==

It can have two advantages as:

1. Avoid unnecessary list_copy() in TopMemoryContext context &
2. Global pointers like activeSearchPath/baseSearchPath will not change if some
implementation end up with cascaded call to recomputeNamespacePath().

Thoughts/Comments?

Regards,

Amul

Re: Can avoid list_copy in recomputeNamespacePath() conditionally?

From

Tom Lane

Date:

02 November 2019, 17:31:14

amul sul <sulamul@gmail.com> writes:
> I wondered can we have a shortcut somewhat similar to following POC
> in recomputeNamespacePath () when the recomputed path is the same as the
> previous baseSearchPath/activeSearchPath :
> +   /* TODO: POC */
> +   if (equal(oidlist, baseSearchPath))
> +       return;

There's an awful lot missing from that sketch; all of the remaining
steps still need to be done:

    baseCreationNamespace = firstNS;
    baseTempCreationPending = temp_missing;

    /* Mark the path valid. */
    baseSearchPathValid = true;
    namespaceUser = roleid;

    /* And make it active. */
    activeSearchPath = baseSearchPath;
    activeCreationNamespace = baseCreationNamespace;
    activeTempCreationPending = baseTempCreationPending;

    /* Clean up. */
    pfree(rawname);
    list_free(namelist);
    list_free(oidlist);

More to the point, I think the onus would be on the patch submitter
to prove that the extra complexity had some measurable benefit.
I really doubt that it would, since the list_copy is surely trivial
compared to the catalog lookup work we had to do to compute the OID
list above here.

It'd likely be more useful to see if you could reduce the number of
places where we have to invalidate the path in the first place.

            regards, tom lane

Re: Can avoid list_copy in recomputeNamespacePath() conditionally?

From

amul sul

Date:

04 November 2019, 08:03:25

On Sat, Nov 2, 2019 at 8:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

amul sul <sulamul@gmail.com> writes:
> I wondered can we have a shortcut somewhat similar to following POC
> in recomputeNamespacePath () when the recomputed path is the same as the
> previous baseSearchPath/activeSearchPath :
> + /* TODO: POC */
> + if (equal(oidlist, baseSearchPath))
> + return;

There's an awful lot missing from that sketch; all of the remaining
steps still need to be done:

You are correct, but that was intentionally skipped to avoid longer post
descriptions for the initial discussion. Sorry for being little lazy.

baseCreationNamespace = firstNS;
baseTempCreationPending = temp_missing;

/* Mark the path valid. */
baseSearchPathValid = true;
namespaceUser = roleid;

/* And make it active. */
activeSearchPath = baseSearchPath;
activeCreationNamespace = baseCreationNamespace;
activeTempCreationPending = baseTempCreationPending;

/* Clean up. */
pfree(rawname);
list_free(namelist);
list_free(oidlist);

More to the point, I think the onus would be on the patch submitter
to prove that the extra complexity had some measurable benefit.
I really doubt that it would, since the list_copy is surely trivial
compared to the catalog lookup work we had to do to compute the OID
list above here.

Agree.

It'd likely be more useful to see if you could reduce the number of
places where we have to invalidate the path in the first place.

Understood, let me check.

Regards,

Amul