Adding missing object access hook invocations
Hackers,
While working on object access hooks, I noticed several locations where I would expect the hook to be invoked, but no actual invocation. I think this just barely qualifies as a bug. It's debatable because whether it is a bug depends on the user's expectations and whether not invoking the hook in these cases is defensible. Does anybody have any recollection of an intentional choice not to invoke in these locations?
Patch attached.
Attachments:
v1-0001-Adding-missing-Object-Access-hook-invocations.patchapplication/octet-stream; name=v1-0001-Adding-missing-Object-Access-hook-invocations.patch; x-unix-mode=0644Download
From 2759f8deefc89e40eb6a299540961aa5e8fd6b61 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Mon, 16 Mar 2020 08:53:32 -0700
Subject: [PATCH v1] Adding missing Object Access hook invocations.
There appears to be no reason for skipping the invocation of the
object access hook infrastructure for the following commands, yet
they were lacking the InvokeObject{PostCreate,PostAlter,Drop}Hook
calls that would be expected:
ALTER RULE
ALTER USER MAPPING
CREATE ACCESS METHOD
CREATE STATISTICS
DROP STATISTICS
DROP ACCESS METHOD
There doesn't seem to be any good regression test coverage for
when and how the object_access_hook is invoked, so no additional
coverage is included here, either. Adding regression test
coverage seems like another patch to be submitted separately.
---
src/backend/commands/amcmds.c | 5 +++++
src/backend/commands/foreigncmds.c | 3 +++
src/backend/commands/statscmds.c | 4 ++++
src/backend/rewrite/rewriteDefine.c | 2 ++
4 files changed, 14 insertions(+)
diff --git a/src/backend/commands/amcmds.c b/src/backend/commands/amcmds.c
index 7546378bbb..1ecad090ee 100644
--- a/src/backend/commands/amcmds.c
+++ b/src/backend/commands/amcmds.c
@@ -18,6 +18,7 @@
#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/indexing.h"
+#include "catalog/objectaccess.h"
#include "catalog/pg_am.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
@@ -107,6 +108,8 @@ CreateAccessMethod(CreateAmStmt *stmt)
recordDependencyOnCurrentExtension(&myself, false);
+ InvokeObjectPostCreateHook(AccessMethodRelationId, amoid, 0);
+
table_close(rel, RowExclusiveLock);
return myself;
@@ -134,6 +137,8 @@ RemoveAccessMethodById(Oid amOid)
CatalogTupleDelete(relation, &tup->t_self);
+ InvokeObjectDropHook(AccessMethodRelationId, amOid, 0);
+
ReleaseSysCache(tup);
table_close(relation, RowExclusiveLock);
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index f197869752..a399ab4de9 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -1343,6 +1343,9 @@ AlterUserMapping(AlterUserMappingStmt *stmt)
CatalogTupleUpdate(rel, &tp->t_self, tp);
+ InvokeObjectPostAlterHook(UserMappingRelationId,
+ umId, 0);
+
ObjectAddressSet(address, UserMappingRelationId, umId);
heap_freetuple(tp);
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 988cdba6f5..08d0da1109 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -374,6 +374,8 @@ CreateStatistics(CreateStatsStmt *stmt)
relation_close(datarel, RowExclusiveLock);
+ InvokeObjectPostCreateHook(StatisticExtRelationId, statoid, 0);
+
/*
* Invalidate relcache so that others see the new statistics object.
*/
@@ -568,6 +570,8 @@ RemoveStatisticsById(Oid statsOid)
CatalogTupleDelete(relation, &tup->t_self);
+ InvokeObjectDropHook(StatisticExtRelationId, statsOid, 0);
+
ReleaseSysCache(tup);
table_close(relation, RowExclusiveLock);
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index afc78b3316..9989df1107 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -1003,6 +1003,8 @@ RenameRewriteRule(RangeVar *relation, const char *oldName,
CatalogTupleUpdate(pg_rewrite_desc, &ruletup->t_self, ruletup);
+ InvokeObjectPostAlterHook(RewriteRelationId, ruleOid, 0);
+
heap_freetuple(ruletup);
table_close(pg_rewrite_desc, RowExclusiveLock);
--
2.21.1 (Apple Git-122.3)
On 2020-Mar-16, Mark Dilger wrote:
Hackers,
While working on object access hooks, I noticed several locations where I would expect the hook to be invoked, but no actual invocation. I think this just barely qualifies as a bug. It's debatable because whether it is a bug depends on the user's expectations and whether not invoking the hook in these cases is defensible. Does anybody have any recollection of an intentional choice not to invoke in these locations?
Hmm, possibly the create-time calls are missing.
I'm surprised about the InvokeObjectDropHook calls though. Doesn't
deleteOneObject already call that? If we have more calls elsewhere,
maybe they are redundant. I think we should only have those for
"shared" objects.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mar 16, 2020, at 5:14 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2020-Mar-16, Mark Dilger wrote:
Hackers,
While working on object access hooks, I noticed several locations where I would expect the hook to be invoked, but no actual invocation. I think this just barely qualifies as a bug. It's debatable because whether it is a bug depends on the user's expectations and whether not invoking the hook in these cases is defensible. Does anybody have any recollection of an intentional choice not to invoke in these locations?
Hmm, possibly the create-time calls are missing.
It looks to me that both the create and alter calls are missing.
I'm surprised about the InvokeObjectDropHook calls though. Doesn't
deleteOneObject already call that? If we have more calls elsewhere,
maybe they are redundant. I think we should only have those for
"shared" objects.
Yeah, you are right about the drop hook being invoked elsewhere for dropping ACCESS METHOD and STATISTICS. Sorry for the noise.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
On 2020-03-16 16:03:51 -0700, Mark Dilger wrote:
While working on object access hooks, I noticed several locations
where I would expect the hook to be invoked, but no actual invocation.
I think this just barely qualifies as a bug. It's debatable because
whether it is a bug depends on the user's expectations and whether not
invoking the hook in these cases is defensible. Does anybody have any
recollection of an intentional choice not to invoke in these
locations?
I am strongly against treating this as a bug, which'd likely imply
backpatching. New hook invocations are a noticable behavioural change,
and very plausibly will break currently working extensions. That's fine
for a major version upgrade, but not for a minor one, unless there are
very good reasons.
Andres
On Mar 17, 2020, at 11:49 AM, Andres Freund <andres@anarazel.de> wrote:
On 2020-03-16 16:03:51 -0700, Mark Dilger wrote:
While working on object access hooks, I noticed several locations
where I would expect the hook to be invoked, but no actual invocation.
I think this just barely qualifies as a bug. It's debatable because
whether it is a bug depends on the user's expectations and whether not
invoking the hook in these cases is defensible. Does anybody have any
recollection of an intentional choice not to invoke in these
locations?I am strongly against treating this as a bug, which'd likely imply
backpatching. New hook invocations are a noticable behavioural change,
and very plausibly will break currently working extensions. That's fine
for a major version upgrade, but not for a minor one, unless there are
very good reasons.
I agree that this does not need to be back-patched. I was debating whether it constitutes a bug for the purpose of putting the fix into v13 vs. punting the patch forward to the v14 cycle. I don't have a strong opinion on that.
Thoughts?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Mar 17, 2020 at 12:39:35PM -0700, Mark Dilger wrote:
I agree that this does not need to be back-patched. I was debating
whether it constitutes a bug for the purpose of putting the fix into
v13 vs. punting the patch forward to the v14 cycle. I don't have a
strong opinion on that.
I don't see any strong argument against fixing this stuff in v13,
FWIW.
--
Michael
On Mar 17, 2020, at 9:33 PM, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Mar 17, 2020 at 12:39:35PM -0700, Mark Dilger wrote:
I agree that this does not need to be back-patched. I was debating
whether it constitutes a bug for the purpose of putting the fix into
v13 vs. punting the patch forward to the v14 cycle. I don't have a
strong opinion on that.I don't see any strong argument against fixing this stuff in v13,
FWIW.
Here is the latest patch. I'll go add this thread to the commitfest app now....
Attachments:
v2-0001-Adding-missing-Object-Access-hook-invocations.patchapplication/octet-stream; name=v2-0001-Adding-missing-Object-Access-hook-invocations.patch; x-unix-mode=0644Download
From 2759f8deefc89e40eb6a299540961aa5e8fd6b61 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Mon, 16 Mar 2020 08:53:32 -0700
Subject: [PATCH v2] Adding missing Object Access hook invocations.
There appears to be no reason for skipping the invocation of the object
access hook infrastructure for the following commands, yet they were
lacking the InvokeObjectPostCreateHook and InvokeObjectPostAlterHook
calls that would be expected:
ALTER RULE
ALTER USER MAPPING
CREATE ACCESS METHOD
CREATE STATISTICS
There doesn't seem to be any good regression test coverage for
when and how the object_access_hook is invoked, so no additional
coverage is included here, either. Adding regression test
coverage seems like another patch to be submitted separately.
---
src/backend/commands/amcmds.c | 5 +++++
src/backend/commands/foreigncmds.c | 3 +++
src/backend/commands/statscmds.c | 4 ++++
src/backend/rewrite/rewriteDefine.c | 2 ++
4 files changed, 14 insertions(+)
diff --git a/src/backend/commands/amcmds.c b/src/backend/commands/amcmds.c
index 7546378bbb..1ecad090ee 100644
--- a/src/backend/commands/amcmds.c
+++ b/src/backend/commands/amcmds.c
@@ -18,6 +18,7 @@
#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/indexing.h"
+#include "catalog/objectaccess.h"
#include "catalog/pg_am.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
@@ -107,6 +108,8 @@ CreateAccessMethod(CreateAmStmt *stmt)
recordDependencyOnCurrentExtension(&myself, false);
+ InvokeObjectPostCreateHook(AccessMethodRelationId, amoid, 0);
+
table_close(rel, RowExclusiveLock);
return myself;
@@ -134,6 +137,8 @@ RemoveAccessMethodById(Oid amOid)
CatalogTupleDelete(relation, &tup->t_self);
+ InvokeObjectDropHook(AccessMethodRelationId, amOid, 0);
+
ReleaseSysCache(tup);
table_close(relation, RowExclusiveLock);
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index f197869752..a399ab4de9 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -1343,6 +1343,9 @@ AlterUserMapping(AlterUserMappingStmt *stmt)
CatalogTupleUpdate(rel, &tp->t_self, tp);
+ InvokeObjectPostAlterHook(UserMappingRelationId,
+ umId, 0);
+
ObjectAddressSet(address, UserMappingRelationId, umId);
heap_freetuple(tp);
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 988cdba6f5..08d0da1109 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -374,6 +374,8 @@ CreateStatistics(CreateStatsStmt *stmt)
relation_close(datarel, RowExclusiveLock);
+ InvokeObjectPostCreateHook(StatisticExtRelationId, statoid, 0);
+
/*
* Invalidate relcache so that others see the new statistics object.
*/
@@ -568,6 +570,8 @@ RemoveStatisticsById(Oid statsOid)
CatalogTupleDelete(relation, &tup->t_self);
+ InvokeObjectDropHook(StatisticExtRelationId, statsOid, 0);
+
ReleaseSysCache(tup);
table_close(relation, RowExclusiveLock);
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index afc78b3316..9989df1107 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -1003,6 +1003,8 @@ RenameRewriteRule(RangeVar *relation, const char *oldName,
CatalogTupleUpdate(pg_rewrite_desc, &ruletup->t_self, ruletup);
+ InvokeObjectPostAlterHook(RewriteRelationId, ruleOid, 0);
+
heap_freetuple(ruletup);
table_close(pg_rewrite_desc, RowExclusiveLock);
--
2.21.1 (Apple Git-122.3)
On 2020-Mar-18, Mark Dilger wrote:
Here is the latest patch.
So you insist in keeping the Drop hook calls?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mar 19, 2020, at 11:17 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2020-Mar-18, Mark Dilger wrote:
Here is the latest patch.
So you insist in keeping the Drop hook calls?
My apologies, not at all. I appear to have attached the wrong patch. Will post v3 shortly.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Show quoted text
On Mar 19, 2020, at 11:30 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
Will post v3 shortly.
Attachments:
v3-0001-Adding-missing-Object-Access-hook-invocations.patchapplication/octet-stream; name=v3-0001-Adding-missing-Object-Access-hook-invocations.patch; x-unix-mode=0644Download
From 90119a3ea6dfa6ca28d02c30dbdce370e8f0a3a6 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Thu, 19 Mar 2020 11:35:09 -0700
Subject: [PATCH v3] Adding missing Object Access hook invocations.
There appears to be no reason for skipping the invocation of the
object access hook infrastructure for the following commands, yet they
were lacking the InvokeObjectPost{Create,Alter}Hook calls that would
be expected:
ALTER RULE
ALTER USER MAPPING
CREATE ACCESS METHOD
CREATE STATISTICS
There doesn't seem to be any good regression test coverage for when
and how the object_access_hook is invoked, so no additional coverage
is included here, either. Adding regression test coverage seems like
another patch to be submitted separately.
---
src/backend/commands/amcmds.c | 3 +++
src/backend/commands/foreigncmds.c | 3 +++
src/backend/commands/statscmds.c | 2 ++
src/backend/rewrite/rewriteDefine.c | 2 ++
4 files changed, 10 insertions(+)
diff --git a/src/backend/commands/amcmds.c b/src/backend/commands/amcmds.c
index 7546378bbb..b884bfa0b0 100644
--- a/src/backend/commands/amcmds.c
+++ b/src/backend/commands/amcmds.c
@@ -18,6 +18,7 @@
#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/indexing.h"
+#include "catalog/objectaccess.h"
#include "catalog/pg_am.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
@@ -107,6 +108,8 @@ CreateAccessMethod(CreateAmStmt *stmt)
recordDependencyOnCurrentExtension(&myself, false);
+ InvokeObjectPostCreateHook(AccessMethodRelationId, amoid, 0);
+
table_close(rel, RowExclusiveLock);
return myself;
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index f197869752..a399ab4de9 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -1343,6 +1343,9 @@ AlterUserMapping(AlterUserMappingStmt *stmt)
CatalogTupleUpdate(rel, &tp->t_self, tp);
+ InvokeObjectPostAlterHook(UserMappingRelationId,
+ umId, 0);
+
ObjectAddressSet(address, UserMappingRelationId, umId);
heap_freetuple(tp);
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 988cdba6f5..d30059d043 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -374,6 +374,8 @@ CreateStatistics(CreateStatsStmt *stmt)
relation_close(datarel, RowExclusiveLock);
+ InvokeObjectPostCreateHook(StatisticExtRelationId, statoid, 0);
+
/*
* Invalidate relcache so that others see the new statistics object.
*/
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index afc78b3316..9989df1107 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -1003,6 +1003,8 @@ RenameRewriteRule(RangeVar *relation, const char *oldName,
CatalogTupleUpdate(pg_rewrite_desc, &ruletup->t_self, ruletup);
+ InvokeObjectPostAlterHook(RewriteRelationId, ruleOid, 0);
+
heap_freetuple(ruletup);
table_close(pg_rewrite_desc, RowExclusiveLock);
--
2.21.1 (Apple Git-122.3)
On Thu, Mar 19, 2020 at 11:47:46AM -0700, Mark Dilger wrote:
On Mar 19, 2020, at 11:30 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
Will post v3 shortly.
Thanks for sending a new version of the patch and removing the bits
about object drops. Your additions to src/backend/ look fine to me,
so I have no objections to commit it. The only module we have in core
that makes use of object_access_hook is sepgsql. Adding support for
it could be done in a separate commit for AMs, stats and user mappings
but we would need a use-case for it. One thing that I can see is that
even if we test for ALTER put_your_object_type_here foo RENAME TO in
the module and that your patch adds one InvokeObjectPostAlterHook()
for ALTER RULE, we don't have support for rules in sepgsql (see
sepgsql_object_access for OAT_POST_CREATE). So that's fine.
Unfortunately, we are past feature freeze so this will have to wait
until v14 opens for business to be merged, and I'll take care of it.
Or would others prefer to not wait one extra year for those changes to
be released?
Please note that there is a commit fest entry, though you forgot to
add your name as author of the patch:
https://commitfest.postgresql.org/28/2513/
--
Michael
On Apr 19, 2020, at 3:55 PM, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Mar 19, 2020 at 11:47:46AM -0700, Mark Dilger wrote:
On Mar 19, 2020, at 11:30 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
Will post v3 shortly.
Thanks for sending a new version of the patch and removing the bits
about object drops. Your additions to src/backend/ look fine to me,
so I have no objections to commit it. The only module we have in core
that makes use of object_access_hook is sepgsql. Adding support for
it could be done in a separate commit for AMs, stats and user mappings
but we would need a use-case for it. One thing that I can see is that
even if we test for ALTER put_your_object_type_here foo RENAME TO in
the module and that your patch adds one InvokeObjectPostAlterHook()
for ALTER RULE, we don't have support for rules in sepgsql (see
sepgsql_object_access for OAT_POST_CREATE). So that's fine.Unfortunately, we are past feature freeze so this will have to wait
until v14 opens for business to be merged, and I'll take care of it.
Or would others prefer to not wait one extra year for those changes to
be released?
I don't intend to make any special pleading for this to go in after feature freeze. Let's see if others feel differently.
Thanks for the review!
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2020-Apr-20, Michael Paquier wrote:
Unfortunately, we are past feature freeze so this will have to wait
until v14 opens for business to be merged, and I'll take care of it.
Or would others prefer to not wait one extra year for those changes to
be released?
I think it's fine to put this in at this time. It's not a new feature.
The only thing this needs is to go through a new release cycle so that
people can adjust to the new hook invocations as necessary.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Apr 20, 2020 at 01:32:31PM -0400, Alvaro Herrera wrote:
I think it's fine to put this in at this time. It's not a new feature.
The only thing this needs is to go through a new release cycle so that
people can adjust to the new hook invocations as necessary.
Okay. Any other opinions? I am in a 50/50 state about that stuff.
--
Michael
On Mon, Apr 20, 2020 at 9:40 PM Michael Paquier <michael@paquier.xyz> wrote:
Okay. Any other opinions? I am in a 50/50 state about that stuff.
I don't really see any reason why this couldn't be committed even at
this late date, but I also don't care that much. I suspect the number
of extension authors who are likely to have to make any code changes
is small. It's anybody's guess whether those people would like these
changes (because now they can support all of these object types even
in v13, rather than having to wait another year) or dislike them
(because it breaks something). I would actually be more inclined to
bet on the former rather than the latter, but unless somebody speaks
up, it's all just speculation.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, May 20, 2020 at 01:57:31PM -0400, Robert Haas wrote:
I don't really see any reason why this couldn't be committed even at
this late date, but I also don't care that much. I suspect the number
of extension authors who are likely to have to make any code changes
is small. It's anybody's guess whether those people would like these
changes (because now they can support all of these object types even
in v13, rather than having to wait another year) or dislike them
(because it breaks something). I would actually be more inclined to
bet on the former rather than the latter, but unless somebody speaks
up, it's all just speculation.
Thanks for the input, Robert. So, even if we are post-beta1 it looks
like there are more upsides than downsides to get that stuff done
sooner than later. I propose to get that applied in the next couple
of days, please let me know if there are any objections.
--
Michael
On Thu, May 21, 2020 at 09:32:55AM +0900, Michael Paquier wrote:
Thanks for the input, Robert. So, even if we are post-beta1 it looks
like there are more upsides than downsides to get that stuff done
sooner than later. I propose to get that applied in the next couple
of days, please let me know if there are any objections.
Hearing nothing, done. Thanks all for the discussion.
--
Michael
On 2020-May-23, Michael Paquier wrote:
On Thu, May 21, 2020 at 09:32:55AM +0900, Michael Paquier wrote:
Thanks for the input, Robert. So, even if we are post-beta1 it looks
like there are more upsides than downsides to get that stuff done
sooner than later. I propose to get that applied in the next couple
of days, please let me know if there are any objections.Hearing nothing, done. Thanks all for the discussion.
Thanks!
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services