Improve the performance of CitusHasBeenLoaded function for a database that does not do CREATE EXTENSION citus but load citus.so. #7123

emelsimsek · 2023-08-16T10:29:33Z

For a database that does not create the citus extension by running

CREATE EXTENSION citus;

CitusHasBeenLoaded function ends up querying the pg_extension table every time it is invoked. This is not an ideal situation for a such a database.

The idea in this PR is as follows:

A new field in MetadataCache.

Add a new variable extensionCreatedState of the following type:

typedef enum ExtensionCreatedState
{
        UNKNOWN = 0,
        CREATED = 1,
        NOTCREATED = 2,
} ExtensionCreatedState;

When the MetadataCache is invalidated, ExtensionCreatedState will be set to UNKNOWN.

Invalidate MetadataCache when CREATE/DROP/ALTER EXTENSION citus commands are run.

Register a callback function, named InvalidateDistRelationCacheCallback, for relcache invalidation during the shared library initialization for citus.so. This callback function is invoked in all the backends whenever the relcache is invalidated in one of the backends. (This could be caused many DDLs operations).
In the cache invalidation callback, InvalidateDistRelationCacheCallback, invalidate MetadataCache zeroing it out.
In CitusHasBeenLoaded, perform the costly citus is loaded check only if the MetadataCache is not valid.

Downsides

Any relcache invalidation (caused by various DDL operations) will case Citus MetadataCache to get invalidated. Most of the time it will be unnecessary. But we rely on that DDL operations on relations will not be too frequent.

JelteF · 2023-08-17T09:08:55Z

src/backend/distributed/metadata/metadata_cache.c

+	 * MetadataCache.extensionLoaded and MetadataCache.extensionNotLoaded
+	 * cannot be true at the same time.
+	 */
+	Assert(!(MetadataCache.extensionLoaded && MetadataCache.extensionNotLoaded));


nit: I think most of these checks become easier to understand with a 3 element enum instead of 2 booleans. Something like this: EXTENSION_LOADED_UNKNOWN EXTENSION_LOADED EXTENSION_NOT_LOADED (feel free to choose better names)

JelteF

Overall really nice and simple solution. If we can have a test for cross backend invalidation that would be great. You would probably need either isolation tests or the python testing framework for that.

JelteF · 2023-08-17T09:30:45Z

src/backend/distributed/metadata/metadata_cache.c

@@ -2185,7 +2186,19 @@ HasOverlappingShardInterval(ShardInterval **shardIntervalArray,
 bool
 CitusHasBeenLoaded(void)
 {
-	if (!MetadataCache.extensionLoaded || creating_extension)


We should probably call AcceptInvalidationMessages() here. To make sure we've processed any outstanding messages.

I have reorganized this part of the code such that when creating_extension == true for citus we do not change the cached value of extensionLoadedState but simply return false.

This change is to make the following scneario work:

BEGIN;

SET client_min_messages TO ERROR;
SET search_path TO public;
CREATE EXTENSION citus;
create table l1 (a int unique);
SELECT create_reference_table('l1');

When creating_extension=true, If we change the extensionCreatedState to false during creating_extension, it stayed false after this line causing the subsequent create_reference_table to fail.

This might be due to that we invalidate cache during preprocessing phase of the citus utility hook when
CREATE EXTENSION citus;
runs. And if we recreated the cache during creating_extension (we set extensionCreatedState to false), we do not get another cache invalidation message that would invalidate the cache.

AcceptInvalidationMessages();

did not eliminate the issue.

I opted for not changing the cache during creating_extension phase.

This change should not have any impact on performance since

if (creating_extension)

used to run previously every time even when the cache is valid.

JelteF · 2023-08-17T09:48:06Z

src/backend/distributed/metadata/metadata_cache.c

+		{
+			return;
+		}
+
 		CitusTableCacheEntrySlot *cacheSlot =
 			hash_search(DistTableCacheHash, hashKey, HASH_FIND, &foundInCache);
 		if (foundInCache)


I think we can now remove the InvalidateMetadataSystemCache call below here.

Removing InvalidateMetadataSystemCache call here causes check-vanilla test suit to fail. Specifically check_index is failing consistently in the CI if I remove this code. Let me investigate why, a bit further.

It turns out that

REINDEX SCHEMA CONCURRENTLY pg_catalog;

command causes the relcache invalidation of pg_dist tables which are under pg_catalog. If we do not invalidate Metadatacache, it will end up with stale oids causing the subsequent commands to fail.

For instance, the following command
\d

will run a SELECT command that invokes citus hooks and will cause failed cached lookups of pg_dist* tables.

So we need to invalidate the cache when one of the pg_dist* tables got invalidated.

I have restored the previous code not to regress check-vanilla.
We will track the issue of pg_dist_ tables getting invalidated in a seperate PR.

Note that, even if I restore the original code there are likely two issues with the original code

When a pg_dist table other than pg_dist_partition gets invalidated for example using
REINDEX TABLE pg_dist_background_task;
we currently do not refresh its oid in the Metadata cache.

When pg_dist_partition gets invalidated we unnecessarily nuke the entire MetadataCache.

codecov · 2023-08-18T08:16:52Z

Codecov Report

Merging #7123 (d3c3fb8) into main (1d540b6) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7123      +/-   ##
==========================================
- Coverage   93.20%   93.19%   -0.02%     
==========================================
  Files         274      274              
  Lines       59232    59240       +8     
==========================================
- Hits        55208    55207       -1     
- Misses       4024     4033       +9

JelteF · 2023-08-31T11:49:28Z

src/backend/distributed/metadata/metadata_cache.c

+static bool
+NeedsMetadataCacheInvalidation(Oid relationId)
+{
+	if (relationId == MetadataCache.distPartitionRelationId ||


I thought it was surprising that this was checking the table Oids instead of the index Oids, because the index oids are actually changed when a REINDEX CONCURRENTLY happens.

So I had expected that an invalidation would be sent on the index its oid instead of the one for the table that it's an index for. As it turns out it's more interesting than that. Invalidations are sent for many things when a REINDEX cuncurrently happens:

The oid of the table

The oid of the old index

The oid of the new index

One thing I'm worried about with the current change is that we might flush the cache more often than before, maybe even much more often depending on the workload. My suggestions:

Let's keep the current behaviour in this PR, flush the full cache on distPartitionRelationId invalidations. At least we don't regress on REINDEX SCHEMA CONCURRENLTY pg_catalog, while also not regressing on performance by flushing more often than before to handle this pretty edge-casy bug.

Let's create a second PR that fixes it for all indexes. (maybe only create an issue initially, and we can prioritise fixing the bug).

I think it would be better if that PR would not listen for invalidations for table oid, but instead only for index oids. Then when a cached index gets an invalidation message, we should not invalidate the whole cache, but instead only set the matching distXyzIndexId field to InvalidOid.

PS. The minimal example of the original issue that I could find is by running a REINDEX CONCURRENTLY twice on pg_dist_partition_logical_relid_index, i.e.:

REINDEX INDEX CONCURRENTLY pg_catalog.pg_dist_partition_logical_relid_index; REINDEX INDEX CONCURRENTLY pg_catalog.pg_dist_partition_logical_relid_index;

JelteF · 2023-09-04T10:27:16Z

src/backend/distributed/commands/utility_hook.c

-		/*
-		 * Ensure value is valid, we can't do some checks during CREATE
-		 * EXTENSION. This is important to register some invalidation callbacks.
-		 */
-		CitusHasBeenLoaded(); /* lgtm[cpp/return-value-ignored] */


Why remove this? The original comment makes it sounds like it is important.

Oh, hmm I guess this is related to moving InitializeDistCache to _PG_init. Because we register the callback there, we don't do that here anymore.

JelteF · 2023-09-04T10:33:39Z

src/backend/distributed/shared_library_init.c

+	CacheRegisterRelcacheCallback(InvalidateDistRelationCacheCallback,
+								  (Datum) 0);
+


Why was this call moved here instead of staying in InitializeDistCache

JelteF · 2023-09-04T10:34:40Z

src/backend/distributed/shared_library_init.c

@@ -122,6 +124,8 @@ CompressionTypeStr_type extern_CompressionTypeStr = NULL;
 IsColumnarTableAmTable_type extern_IsColumnarTableAmTable = NULL;
 ReadColumnarOptions_type extern_ReadColumnarOptions = NULL;

+void InvalidateDistRelationCacheCallback(Datum argument, Oid relationId);


This should be in a header instead (if we want to keep calling it here, see other comment).

JelteF

(pressed the wrong button, needs at one small change)

JelteF · 2023-09-04T11:27:59Z

src/test/regress/citus_tests/test/test_extension.py

+            try:
+                cur2.execute("SELECT citus_version();")
+                # Conn1 dropped the extension. citus_version udf
+                # cannot be found.sycopg.errors.UndefinedFunction
+                # is expected here.
+            except psycopg.errors.UndefinedFunction:
+                cur2.execute("SELECT 1;")


This should use pytest.raises instead to assert that the exception is thrown.

Suggested change

try:

cur2.execute("SELECT citus_version();")

# Conn1 dropped the extension. citus_version udf

# cannot be found.sycopg.errors.UndefinedFunction

# is expected here.

except psycopg.errors.UndefinedFunction:

cur2.execute("SELECT 1;")

with pytest.raises(psycopg.errors.UndefinedFunction):

# Conn1 dropped the extension. citus_version udf

# cannot be found.sycopg.errors.UndefinedFunction

# is expected here.

cur2.execute("SELECT citus_version();")

… that does not do CREATE EXTENSION citus but load citus.so. (#7123) For a database that does not create the citus extension by running ` CREATE EXTENSION citus;` `CitusHasBeenLoaded ` function ends up querying the `pg_extension` table every time it is invoked. This is not an ideal situation for a such a database. The idea in this PR is as follows: ### A new field in MetadataCache. Add a new variable `extensionCreatedState `of the following type: ``` typedef enum ExtensionCreatedState { UNKNOWN = 0, CREATED = 1, NOTCREATED = 2, } ExtensionCreatedState; ``` When the MetadataCache is invalidated, `ExtensionCreatedState` will be set to UNKNOWN. ### Invalidate MetadataCache when CREATE/DROP/ALTER EXTENSION citus commands are run. - Register a callback function, named `InvalidateDistRelationCacheCallback`, for relcache invalidation during the shared library initialization for `citus.so`. This callback function is invoked in all the backends whenever the relcache is invalidated in one of the backends. (This could be caused many DDLs operations). - In the cache invalidation callback,` InvalidateDistRelationCacheCallback`, invalidate `MetadataCache` zeroing it out. - In `CitusHasBeenLoaded`, perform the costly citus is loaded check only if the `MetadataCache` is not valid. ### Downsides Any relcache invalidation (caused by various DDL operations) will case Citus MetadataCache to get invalidated. Most of the time it will be unnecessary. But we rely on that DDL operations on relations will not be too frequent.

emelsimsek requested a review from JelteF August 16, 2023 10:36

emelsimsek marked this pull request as draft August 16, 2023 10:36

emelsimsek changed the title ~~Cache a new flag, extensionNotLoaded, to improve the performance of CitusHasBeenLoaded function for the databases that does not create the citus extension.~~ Cache a new flag, extensionNotLoaded, to improve the performance of CitusHasBeenLoaded function. Aug 16, 2023

JelteF reviewed Aug 17, 2023

View reviewed changes

emelsimsek force-pushed the MetadataCacheExtensionNotLoaded branch from 7d80c17 to c0ea89a Compare August 21, 2023 11:27

emelsimsek changed the title ~~Cache a new flag, extensionNotLoaded, to improve the performance of CitusHasBeenLoaded function.~~ Improve the performance of CitusHasBeenLoaded function for databases that does not CREATE citus extension but load the shared library citus.so. Aug 28, 2023

JelteF reviewed Aug 31, 2023

View reviewed changes

emelsimsek requested a review from JelteF September 4, 2023 10:21

JelteF approved these changes Sep 4, 2023

View reviewed changes

JelteF requested changes Sep 4, 2023

View reviewed changes

JelteF reviewed Sep 4, 2023

View reviewed changes

emelsimsek force-pushed the MetadataCacheExtensionNotLoaded branch from be0aeca to 8904630 Compare September 4, 2023 12:38

JelteF approved these changes Sep 5, 2023

View reviewed changes

emelsimsek marked this pull request as ready for review September 5, 2023 09:57

emelsimsek added 11 commits September 5, 2023 13:00

Testing

9aac2ca

Add comments

0f3de4f

Fix a bug

6d7ced3

Fix test failure

30d5357

Fix

883f9fc

reindent

6c229c1

Remove unnecessary code

bd290dd

Reindent

2bdb0d7

Refactor

2d805e8

Comment

a87a3fb

Review feedback

c3aa546

emelsimsek added 17 commits September 5, 2023 13:00

Fix check-vanilla test failure

2027e6b

Fix check-validate failure

0f991eb

Fix build

b260b1e

Fix check-vanilla

db7c846

Another try

bd778d3

Fix check-vanilla

54194b8

Refactor, Add test

710faf3

Fix build error

1e79689

Cleanup test

05be333

Refactor

ef95881

Refactor

7eaffb6

Add a python test

f94ee91

Remove isolation test

12c828c

Reindent

6e2120e

Remove remnants

4db8c56

Renaming

f43d530

Review feedback

d3c3fb8

emelsimsek force-pushed the MetadataCacheExtensionNotLoaded branch from 0311dd4 to d3c3fb8 Compare September 5, 2023 10:00

emelsimsek merged commit a849570 into main Sep 5, 2023
37 checks passed

emelsimsek deleted the MetadataCacheExtensionNotLoaded branch September 5, 2023 10:29

onurctirtir mentioned this pull request Oct 13, 2023

Adds 2PC distributed commands from other databases #7203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the performance of CitusHasBeenLoaded function for a database that does not do CREATE EXTENSION citus but load citus.so. #7123

Improve the performance of CitusHasBeenLoaded function for a database that does not do CREATE EXTENSION citus but load citus.so. #7123

emelsimsek commented Aug 16, 2023 •

edited

Loading

JelteF Aug 17, 2023

JelteF left a comment

JelteF Aug 17, 2023

emelsimsek Sep 4, 2023

JelteF Aug 17, 2023 •

edited

Loading

emelsimsek Aug 21, 2023 •

edited

Loading

emelsimsek Aug 29, 2023 •

edited

Loading

emelsimsek Sep 4, 2023

codecov bot commented Aug 18, 2023 •

edited

Loading

JelteF Aug 31, 2023 •

edited

Loading

JelteF Sep 4, 2023 •

edited

Loading

JelteF Sep 4, 2023

JelteF Sep 4, 2023

JelteF Sep 4, 2023

JelteF left a comment

JelteF Sep 4, 2023 •

edited

Loading

		CacheRegisterRelcacheCallback(InvalidateDistRelationCacheCallback,
		(Datum) 0);

Improve the performance of CitusHasBeenLoaded function for a database that does not do CREATE EXTENSION citus but load citus.so. #7123

Improve the performance of CitusHasBeenLoaded function for a database that does not do CREATE EXTENSION citus but load citus.so. #7123

Conversation

emelsimsek commented Aug 16, 2023 • edited Loading

A new field in MetadataCache.

Invalidate MetadataCache when CREATE/DROP/ALTER EXTENSION citus commands are run.

Downsides

JelteF Aug 17, 2023

Choose a reason for hiding this comment

JelteF left a comment

Choose a reason for hiding this comment

JelteF Aug 17, 2023

Choose a reason for hiding this comment

emelsimsek Sep 4, 2023

Choose a reason for hiding this comment

JelteF Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

emelsimsek Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

emelsimsek Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

emelsimsek Sep 4, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 18, 2023 • edited Loading

Codecov Report

JelteF Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

JelteF Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

JelteF Sep 4, 2023

Choose a reason for hiding this comment

JelteF Sep 4, 2023

Choose a reason for hiding this comment

JelteF Sep 4, 2023

Choose a reason for hiding this comment

JelteF left a comment

Choose a reason for hiding this comment

JelteF Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

emelsimsek commented Aug 16, 2023 •

edited

Loading

JelteF Aug 17, 2023 •

edited

Loading

emelsimsek Aug 21, 2023 •

edited

Loading

emelsimsek Aug 29, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading

JelteF Aug 31, 2023 •

edited

Loading

JelteF Sep 4, 2023 •

edited

Loading

JelteF Sep 4, 2023 •

edited

Loading