Обсуждение: 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 actualinvocation. I think this just barely qualifies as a bug. It's debatable because whether it is a bug depends on theuser's expectations and whether not invoking the hook in these cases is defensible. Does anybody have any recollectionof an intentional choice not to invoke in these locations? Patch attached. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
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 actualinvocation. I think this just barely qualifies as a bug. It's debatable because whether it is a bug depends on theuser's expectations and whether not invoking the hook in these cases is defensible. Does anybody have any recollectionof 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 noactual invocation. I think this just barely qualifies as a bug. It's debatable because whether it is a bug depends onthe user's expectations and whether not invoking the hook in these cases is defensible. Does anybody have any recollectionof 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 thenoise. — 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 puttingthe 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.... — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
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
> On Mar 19, 2020, at 11:30 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > Will post v3 shortly. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
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