Horton

Thread

Commits

  1. Add missing invocations to object access hooks

  1. Adding missing object access hook invocations

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-03-16T23:03:51Z

    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.

  2. Re: Adding missing object access hook invocations

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-17T00:14:36Z

    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

  3. Re: Adding missing object access hook invocations

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-03-17T00:58:51Z

    
> 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

  4. Re: Adding missing object access hook invocations

    Andres Freund <andres@anarazel.de> — 2020-03-17T18:49:41Z

    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

  5. Re: Adding missing object access hook invocations

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-03-17T19:39:35Z

    
> 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

  6. Re: Adding missing object access hook invocations

    Michael Paquier <michael@paquier.xyz> — 2020-03-18T04:33:05Z

    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

  7. Re: Adding missing object access hook invocations

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-03-18T14:50:11Z

    
> 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....

  8. Re: Adding missing object access hook invocations

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-19T18:17:16Z

    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

  9. Re: Adding missing object access hook invocations

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-03-19T18:30:02Z

    
> 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

  10. Re: Adding missing object access hook invocations

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-03-19T18:47:46Z

    
> On Mar 19, 2020, at 11:30 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
>  Will post v3 shortly.

  11. Re: Adding missing object access hook invocations

    Michael Paquier <michael@paquier.xyz> — 2020-04-19T22:55:46Z

    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

  12. Re: Adding missing object access hook invocations

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-04-19T23:43:45Z

    
> 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

  13. Re: Adding missing object access hook invocations

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-04-20T17:32:31Z

    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

  14. Re: Adding missing object access hook invocations

    Michael Paquier <michael@paquier.xyz> — 2020-04-21T01:39:50Z

    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

  15. Re: Adding missing object access hook invocations

    Robert Haas <robertmhaas@gmail.com> — 2020-05-20T17:57:31Z

    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

  16. Re: Adding missing object access hook invocations

    Michael Paquier <michael@paquier.xyz> — 2020-05-21T00:32:55Z

    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

  17. Re: Adding missing object access hook invocations

    Michael Paquier <michael@paquier.xyz> — 2020-05-23T05:21:50Z

    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

  18. Re: Adding missing object access hook invocations

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-05-24T02:31:55Z

    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