Adding missing object access hook invocations

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Adding missing object access hook invocations

Mark Dilger-5
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.




Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company




v1-0001-Adding-missing-Object-Access-hook-invocations.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Mark Dilger-5


> On Mar 16, 2020, at 5:14 PM, Alvaro Herrera <[hidden email]> 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





Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Andres Freund
In reply to this post by Mark Dilger-5
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



Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Mark Dilger-5


> On Mar 17, 2020, at 11:49 AM, Andres Freund <[hidden email]> 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





Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Mark Dilger-5


> On Mar 17, 2020, at 9:33 PM, Michael Paquier <[hidden email]> 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




v2-0001-Adding-missing-Object-Access-hook-invocations.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Mark Dilger-5


> On Mar 19, 2020, at 11:17 AM, Alvaro Herrera <[hidden email]> 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





Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Mark Dilger-5


> On Mar 19, 2020, at 11:30 AM, Mark Dilger <[hidden email]> wrote:
>
>  Will post v3 shortly.




Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company




v3-0001-Adding-missing-Object-Access-hook-invocations.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Michael Paquier-2
On Thu, Mar 19, 2020 at 11:47:46AM -0700, Mark Dilger wrote:
> On Mar 19, 2020, at 11:30 AM, Mark Dilger <[hidden email]> 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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Mark Dilger-5


> On Apr 19, 2020, at 3:55 PM, Michael Paquier <[hidden email]> wrote:
>
> On Thu, Mar 19, 2020 at 11:47:46AM -0700, Mark Dilger wrote:
>> On Mar 19, 2020, at 11:30 AM, Mark Dilger <[hidden email]> 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





Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Alvaro Herrera-9
In reply to this post by Michael Paquier-2
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


Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Robert Haas
On Mon, Apr 20, 2020 at 9:40 PM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding missing object access hook invocations

Alvaro Herrera-9
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