You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@age.apache.org by "serdarmumcu (via GitHub)" <gi...@apache.org> on 2026/06/11 19:24:24 UTC
[PR] Make age extension usable from shared_preload_libraries [age]
serdarmumcu opened a new pull request, #2438:
URL: https://github.com/apache/age/pull/2438
When AGE is loaded via `shared_preload_libraries`, its hooks (`post_parse_analyze`, `set_rel_pathlist`, `object_access`) are active before `CREATE EXTENSION age` is run. This causes errors when non-Cypher queries trigger those hooks and `ag_catalog` does not yet exist.
## Changes
- Add `is_age_extension_exist()` with a relcache callback cache so that checking `pg_extension` is not repeated on every hook invocation.
- Guard `post_parse_analyze`, `set_rel_pathlist`, and `object_access` hooks with `is_age_extension_exist()` so they become no-ops when the extension is not installed.
- Refactor `ag_ProcessUtility_hook` to detect `CREATE`/`DROP EXTENSION age` and broadcast a relcache invalidation via `CacheInvalidateRelcacheByRelid(ExtensionRelationId)` so other backends update their cached extension state.
- Wrap `DROP EXTENSION` processing in `PG_TRY`/`PG_CATCH` to restore `object_access_hook` if the drop fails (e.g. dependent objects).
- Skip `_PG_init` during `pg_upgrade` (`IsBinaryUpgrade`) to avoid hook registration when the binary-upgrade machinery is running.
- Add regression tests that verify hooks do not error when `ag_catalog` schema is absent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "serdarmumcu (via GitHub)" <gi...@apache.org>.
serdarmumcu commented on PR #2438:
URL: https://github.com/apache/age/pull/2438#issuecomment-4753443458
> @serdarmumcu A regression test is failing
>
> <img alt="image" width="480" height="82" src="https://private-user-images.githubusercontent.com/1505136/609462916-c93417b8-04ad-404c-8c39-54993572f9dc.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3ODE4ODg1MDYsIm5iZiI6MTc4MTg4ODIwNiwicGF0aCI6Ii8xNTA1MTM2LzYwOTQ2MjkxNi1jOTM0MTdiOC0wNGFkLTQwNGMtOGMzOS01NDk5MzU3MmY5ZGMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI2MDYxOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNjA2MTlUMTY1NjQ2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YzIxNmYwZTU5OTUxMDAxZDI5YjRmZmVkMWUyYWEyYjU2MTJlNTdhZGZiMjZkZTQxOTkwMWY3ZmQ4MzNlYmFlOSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmcmVzcG9uc2UtY29udGVudC10eXBlPWltYWdlJTJGcG5nIn0.olT5xZNBeiy752AXfsevde15IU2C5SFdANKqauUD-OU">
> You are running **make installcheck** locally to see these, right?
>
> Also, if you can look at Copilot's suggestions and resolve them, please :)
Hi @jrgemignani , updated commit addresses everything:
Regression test fixed — used DROP SCHEMA IF EXISTS ag_catalog CASCADE with suppressed notices to handle leftover objects from prior tests. Also updated drop_label expected error messages to match current upstream.
Copilot suggestions resolved — added InvalidOid handling in cache callback, renamed is_age_extension_exist → is_age_extension_exists.
All 37 regression tests pass on PG18.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2438:
URL: https://github.com/apache/age/pull/2438#discussion_r3399190599
##########
src/backend/catalog/ag_catalog.c:
##########
@@ -45,9 +51,46 @@ void ag_ProcessUtility_hook(PlannedStmt *pstmt, const char *queryString, bool re
QueryEnvironment *queryEnv, DestReceiver *dest,
QueryCompletion *qc);
-static bool is_age_drop(PlannedStmt *pstmt);
+static bool is_age_drop(DropStmt *drop_stmt);
static void drop_age_extension(DropStmt *stmt);
+static void
+invalidate_extension_cache_callback(Datum argument, Oid relationId)
+{
+ if (relationId == ExtensionRelationId)
+ extension_cache_is_valid = false;
+}
Review Comment:
The relcache invalidation callback only clears the cache when relationId == ExtensionRelationId. Relcache callbacks can be invoked with InvalidOid to mean “invalidate everything”; in that case the extension-existence cache would remain marked valid and could become stale.
##########
src/include/catalog/ag_catalog.h:
##########
@@ -24,6 +24,8 @@
#include "utils/agtype.h"
+bool is_age_extension_exist(void);
Review Comment:
Function name is grammatically off and inconsistent with existing codebase patterns that use “*_exists” (e.g., graph_exists/label_exists). Consider renaming to is_age_extension_exists() (or age_extension_exists()) to better match the surrounding naming conventions.
##########
regress/sql/drop.sql:
##########
@@ -28,6 +28,13 @@ SELECT nspname FROM pg_catalog.pg_namespace WHERE nspname = 'drop';
SELECT tablename FROM pg_catalog.pg_tables WHERE schemaname = 'ag_catalog';
+-- When ag_catalog is missing extension hooks shouldn't fail with the
+-- ERROR schema "ag_catalog" does not exist.
+-- It might happen when 'age' is loaded but extension isn't created yet.
+DROP SCHEMA ag_catalog;
Review Comment:
This test assumes ag_catalog exists after `DROP EXTENSION age;`. To make the regression test resilient to catalog/cleanup behavior changes (or different PG versions), use `IF EXISTS` so the test still validates the “hooks don’t error when ag_catalog is absent” scenario even if the schema was already removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "serdarmumcu (via GitHub)" <gi...@apache.org>.
serdarmumcu commented on PR #2438:
URL: https://github.com/apache/age/pull/2438#issuecomment-4757532275
> See comments
@jrgemignani Fixed — all if blocks are now wrapped with {}. Thanks for the catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #2438:
URL: https://github.com/apache/age/pull/2438#issuecomment-4734944954
@serdarmumcu A regression test is failing
<img width="480" height="82" alt="image" src="https://github.com/user-attachments/assets/c93417b8-04ad-404c-8c39-54993572f9dc" />
You are running **make installcheck** locally to see these, right?
Also, if you can look at Copilot's suggestions and resolve them, please :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2438:
URL: https://github.com/apache/age/pull/2438#discussion_r3430706515
##########
src/backend/catalog/ag_catalog.c:
##########
@@ -45,8 +51,44 @@ void ag_ProcessUtility_hook(PlannedStmt *pstmt, const char *queryString, bool re
QueryEnvironment *queryEnv, DestReceiver *dest,
QueryCompletion *qc);
-static bool is_age_drop(PlannedStmt *pstmt);
-static void drop_age_extension(DropStmt *stmt);
+static bool is_age_drop(DropStmt *drop_stmt);
+
+static void
+invalidate_extension_cache_callback(Datum argument, Oid relationId)
+{
+ if (relationId == ExtensionRelationId)
+ extension_cache_is_valid = false;
+}
Review Comment:
The relcache callback can be invoked with InvalidOid to indicate a global relcache invalidation (see invalidate_label_caches in ag_cache.c). If that happens, the extension-existence cache would remain marked valid and could become stale.
##########
src/include/catalog/ag_catalog.h:
##########
@@ -24,6 +24,8 @@
#include "utils/agtype.h"
+bool is_age_extension_exist(void);
Review Comment:
Function name `is_age_extension_exist` is inconsistent with the codebase’s common `*_exists` naming (e.g., graph_exists, label_id_exists, entity_exists). Consider renaming to `is_age_extension_exists` or similar for consistency, updating the header and all call sites.
##########
regress/expected/drop.out:
##########
@@ -40,6 +40,12 @@ SELECT tablename FROM pg_catalog.pg_tables WHERE schemaname = 'ag_catalog';
-----------
(0 rows)
+-- When ag_catalog is missing extension hooks shouldn't fail with the
+-- ERROR schema "ag_catalog" does not exist.
+-- It might happen when 'age' is loaded but extension isn't created yet.
+DROP SCHEMA ag_catalog;
Review Comment:
Expected output should match the SQL change to `DROP SCHEMA IF EXISTS ag_catalog` to avoid brittle failures if ag_catalog is already missing.
##########
regress/sql/drop.sql:
##########
@@ -28,6 +28,13 @@ SELECT nspname FROM pg_catalog.pg_namespace WHERE nspname = 'drop';
SELECT tablename FROM pg_catalog.pg_tables WHERE schemaname = 'ag_catalog';
+-- When ag_catalog is missing extension hooks shouldn't fail with the
+-- ERROR schema "ag_catalog" does not exist.
+-- It might happen when 'age' is loaded but extension isn't created yet.
+DROP SCHEMA ag_catalog;
Review Comment:
To keep the regression test resilient (e.g., if ag_catalog is already absent after DROP EXTENSION behavior changes), use `DROP SCHEMA IF EXISTS` here. The test still achieves the goal of ensuring ag_catalog is missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "serdarmumcu (via GitHub)" <gi...@apache.org>.
serdarmumcu commented on PR #2438:
URL: https://github.com/apache/age/pull/2438#issuecomment-4691315012
> @serdarmumcu The build has an error -
>
> <img alt="image" width="1297" height="111" src="https://private-user-images.githubusercontent.com/1505136/606690005-dae17788-4dc0-4ba8-b750-a675c8c2d91c.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3ODEyNjc5OTgsIm5iZiI6MTc4MTI2NzY5OCwicGF0aCI6Ii8xNTA1MTM2LzYwNjY5MDAwNS1kYWUxNzc4OC00ZGMwLTRiYTgtYjc1MC1hNjc1YzhjMmQ5MWMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI2MDYxMiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNjA2MTJUMTIzNDU4WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YWVjYzZjODkxMzJkYmNjN2EwZDFiODA0MGE5NTVjMjRmZTcwZDliYjJhM2Q1OGUyYzQxNzU2ZmJlYTAyNDA5MiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmcmVzcG9uc2UtY29udGVudC10eXBlPWltYWdlJTJGcG5nIn0.6O1i_-3D0W_xTV2p__ucKTj1OwP1HF3grxPUTHBT0-E">
> This needs to be fixed.
@jrgemignani Fixed — removed the dead drop_age_extension() function and also cleaned up unrelated changes from internal branches that had leaked into the PR. The commit now only contains changes related to shared_preload_libraries support. Ready for re-review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #2438:
URL: https://github.com/apache/age/pull/2438#issuecomment-4685096633
@serdarmumcu The build has an error -
<img width="1297" height="111" alt="image" src="https://github.com/user-attachments/assets/dae17788-4dc0-4ba8-b750-a675c8c2d91c" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "serdarmumcu (via GitHub)" <gi...@apache.org>.
serdarmumcu closed pull request #2438: Make age extension usable from shared_preload_libraries
URL: https://github.com/apache/age/pull/2438
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "muhammadshoaib (via GitHub)" <gi...@apache.org>.
muhammadshoaib merged PR #2438:
URL: https://github.com/apache/age/pull/2438
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make age extension usable from shared_preload_libraries [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on code in PR #2438:
URL: https://github.com/apache/age/pull/2438#discussion_r3444865507
##########
src/backend/catalog/ag_catalog.c:
##########
@@ -183,7 +279,7 @@ static bool is_age_drop(PlannedStmt *pstmt)
String *val = (String *)obj;
char *str = val->sval;
- if (!pg_strcasecmp(str, "age"))
+ if (strcmp(str, "age") == 0)
return true;
Review Comment:
Needs to be wrapped in {}
##########
src/backend/age.c:
##########
@@ -56,6 +58,9 @@ void _PG_init(void);
void _PG_init(void)
{
+ if (IsBinaryUpgrade)
+ return;
Review Comment:
All blocks are wrapped with {} even if it is just one line of code.
##########
src/backend/catalog/ag_catalog.c:
##########
@@ -205,15 +301,11 @@ static void object_access(ObjectAccessType access, Oid class_id, Oid object_id,
if (prev_object_access_hook)
prev_object_access_hook(access, class_id, object_id, sub_id, arg);
- /* We are interested in DROP SCHEMA and DROP TABLE commands. */
- if (access != OAT_DROP)
+ if (!is_age_extension_exists())
return;
- /*
- * Age might be installed into shared_preload_libraries before extension is
- * created. In this case we must bail out from this hook.
- */
- if (!OidIsValid(get_namespace_oid("ag_catalog", true)))
+ /* We are interested in DROP SCHEMA and DROP TABLE commands. */
+ if (access != OAT_DROP)
return;
Review Comment:
Needs to be wrapped in {}
##########
src/backend/optimizer/cypher_paths.c:
##########
@@ -66,6 +67,9 @@ static void set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, Index rti,
if (prev_set_rel_pathlist_hook)
prev_set_rel_pathlist_hook(root, rel, rti, rte);
+ if (!is_age_extension_exists())
+ return;
Review Comment:
Needs to be wrapped in {}
##########
src/backend/parser/cypher_analyze.c:
##########
@@ -86,6 +87,9 @@ static void post_parse_analyze(ParseState *pstate, Query *query, JumbleState *js
prev_post_parse_analyze_hook(pstate, query, jstate);
}
+ if (!is_age_extension_exists())
+ return;
Review Comment:
Needs to be wrapped in {}
##########
src/backend/catalog/ag_catalog.c:
##########
@@ -45,8 +51,44 @@ void ag_ProcessUtility_hook(PlannedStmt *pstmt, const char *queryString, bool re
QueryEnvironment *queryEnv, DestReceiver *dest,
QueryCompletion *qc);
-static bool is_age_drop(PlannedStmt *pstmt);
-static void drop_age_extension(DropStmt *stmt);
+static bool is_age_drop(DropStmt *drop_stmt);
+
+static void
+invalidate_extension_cache_callback(Datum argument, Oid relationId)
+{
+ if (!OidIsValid(relationId) || relationId == ExtensionRelationId)
+ extension_cache_is_valid = false;
Review Comment:
Needs to be wrapped in {}
##########
src/backend/catalog/ag_catalog.c:
##########
@@ -141,39 +230,46 @@ void ag_ProcessUtility_hook(PlannedStmt *pstmt, const char *queryString,
params, queryEnv, dest, qc);
}
}
-}
-
-static void drop_age_extension(DropStmt *stmt)
-{
- /* Remove all graphs */
- drop_graphs(get_graphnames());
+ PG_CATCH();
+ {
+ if (dropping_age)
+ {
+ /*
+ * We have to restore the disabled object_access_hook if
+ * DROP EXTENSION age failed.
+ */
+ object_access_hook_init();
+ }
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- /* Remove the object access hook */
- object_access_hook_fini();
+ if (dropping_age)
+ {
+ /* reset global variables for OIDs */
+ clear_global_Oids_AGTYPE();
+ clear_global_Oids_GRAPHID();
+ clear_global_Oids_VERTEX_EDGE();
- /*
- * Run Postgres' logic to perform the remaining work to drop the
- * extension.
- */
- RemoveObjects(stmt);
+ /* Restore the object access hook */
+ object_access_hook_init();
+ }
- /* reset global variables for OIDs */
- clear_global_Oids_AGTYPE();
- clear_global_Oids_GRAPHID();
- clear_global_Oids_VERTEX_EDGE();
+ if (creating_age || dropping_age)
+ {
+ /* Notify all backends that pg_extension was modified. */
+ CacheInvalidateRelcacheByRelid(ExtensionRelationId);
+ }
}
/* Check to see if the Utility Command is to drop the AGE Extension. */
-static bool is_age_drop(PlannedStmt *pstmt)
+static bool is_age_drop(DropStmt *drop_stmt)
{
ListCell *lc;
- DropStmt *drop_stmt;
- if (!IsA(pstmt->utilityStmt, DropStmt))
+ if (!is_age_extension_exists())
return false;
Review Comment:
Needs to be wrapped in {}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org