Thread

Commits

Injection points for hash aggregation.
- 38172d1856b3 18.0 landed

injection points for hash aggregation

Jeff Davis <pgsql@j-davis.com> — 2025-02-03T20:45:24Z

Attached is a patch that adds a few injection points for hash
aggregation.

A couple questions on the injection points framework:

* The patch allows forcing the partition fan-out to one. I could
imagine forcing it to a specific value, is there a way to do that?

* The injection_points extension offers the concept of a "local"
injection point. While that makes sense for the callback function, it
doesn't make sense when using the "local_var = 123" style, because that
will happen regardless of the condition, right?

* Callbacks are given very little context from the injection point
itself, so it's hard for me to imagine what a callback might do other
than logging the name of the injection point or waiting (as the
extension implements). What else would callbacks be good for?

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: injection points for hash aggregation

Jeff Davis <pgsql@j-davis.com> — 2025-02-03T23:54:27Z

On Mon, 2025-02-03 at 12:45 -0800, Jeff Davis wrote:
> * The patch allows forcing the partition fan-out to one. I could
> imagine forcing it to a specific value, is there a way to do that?

I hit "send" too quickly and this caused test failures in CI. Attaching
v2.

Changes:

  * a new injection point to force spilling at 1000 tuples so that the
test is deterministic (required some minor refactoring in
hash_agg_check_limits())
  * added a branch to guard against a shift-by-32, which could not
happen in the code before, because the number of partitions was a
minimum of 4
  * minor refactor of hash_agg_set_limits() to avoid an implicit
assumption. This is not directly related, so I added it as a separate
patch.

Regards,
	Jeff Davis

Re: injection points for hash aggregation

Michael Paquier <michael@paquier.xyz> — 2025-02-04T00:21:49Z

On Mon, Feb 03, 2025 at 12:45:24PM -0800, Jeff Davis wrote:
> A couple questions on the injection points framework:
> 
> * The patch allows forcing the partition fan-out to one. I could
> imagine forcing it to a specific value, is there a way to do that?
> 
> * The injection_points extension offers the concept of a "local"
> injection point. While that makes sense for the callback function, it
> doesn't make sense when using the "local_var = 123" style, because that
> will happen regardless of the condition, right?
>
> * Callbacks are given very little context from the injection point
> itself, so it's hard for me to imagine what a callback might do other
> than logging the name of the injection point or waiting (as the
> extension implements). What else would callbacks be good for?

Being able to pass down some runtime states to a callback is something
I've drafted a patch for, as of:
https://www.postgresql.org/message-id/Z23zcE4w1djukkva@paquier.xyz

There was no real agreement if this facility was worth having or not,
because there were no use case for it.  Well, perhaps until now based
on what you are saying.

If you want to force a callback to use a specific value or trigger
conditions based on that, you could add your own new callback in the
module injection_points and give it some custom data when attaching
the point, with a SQL function that uses InjectionPointAttach()'s
private_data.  Then you could rely on injection_point_allowed(), for
example, to decide if a callback is run or not.  So it depends on if
you want to use data known when attaching a callback or do something
based on a state given by a backend when it runs an attached callback.

Something worth mentioning.  Heikki and Peter G. have mentioned to me
at the last pgconf.dev that it could be nice to allow files to hold
their own callbacks rather than have an extension do that (don't
recall there is a footprint about that on the lists, that was an
informal offline discussion).  This would have the advantage to limit
the number of code-path-specific callbacks in the injection_points
module (or just limit the number of modules), with callbacks only
existing locally where they are used.  IS_INJECTION_POINT_ATTACHED()
is what I see as a a simplified version of this idea.
--
Michael

Re: injection points for hash aggregation

Michael Paquier <michael@paquier.xyz> — 2025-02-04T03:50:51Z

On Tue, Feb 04, 2025 at 09:21:49AM +0900, Michael Paquier wrote:
> Being able to pass down some runtime states to a callback is something
> I've drafted a patch for, as of:
> https://www.postgresql.org/message-id/Z23zcE4w1djukkva@paquier.xyz

Oops.  I've posted an incorrect link here, I meant this one instead:
https://www.postgresql.org/message-id/Za8zWmPTNJk_iJzz@paquier.xyz
--
Michael

Re: injection points for hash aggregation

Jeff Davis <pgsql@j-davis.com> — 2025-02-04T05:35:52Z

On Tue, 2025-02-04 at 12:50 +0900, Michael Paquier wrote:
> On Tue, Feb 04, 2025 at 09:21:49AM +0900, Michael Paquier wrote:
> > Being able to pass down some runtime states to a callback is
> > something
> > I've drafted a patch for, as of:
> > https://www.postgresql.org/message-id/Z23zcE4w1djukkva@paquier.xyz
> 
> Oops.  I've posted an incorrect link here, I meant this one instead:
> https://www.postgresql.org/message-id/Za8zWmPTNJk_iJzz@paquier.xyz

I replied in that thread. Thank you.

The injection point arguments are required for $SUBJECT, or at least
not yet, so we can proceed with this thread independently.

Regards,
	Jeff Davis

Re: injection points for hash aggregation

Jeff Davis <pgsql@j-davis.com> — 2025-02-11T19:48:03Z

On Mon, 2025-02-03 at 15:54 -0800, Jeff Davis wrote:
> Attaching
> v2.

I committed just the injection points part. It seemed simple and low-
risk.

Regards,
	Jeff Davis

Re: injection points for hash aggregation

Michael Paquier <michael@paquier.xyz> — 2025-02-18T03:42:57Z

On Tue, Feb 11, 2025 at 11:48:03AM -0800, Jeff Davis wrote:
> I committed just the injection points part. It seemed simple and low-
> risk.

This has been committed and there was a CF entry:
https://commitfest.postgresql.org/patch/5560/

Switched it as committed now.
--
Michael