Support bulk enqueuing of jobs which differ only in args/kwargs by oeoeaio · Pull Request #340 · que-rb/que

oeoeaio · 2022-02-25T00:01:22Z

Resolves: #333

Todo:

To PR and release subsequently:

Support enqueueing different job classes within a .bulk_enqueue block
Support enqueueing different job_options within a .bulk_enqueue block
Deduplicate common code in enqueue and bulk enqueue paths

ZimbiX

This is great work.

How about sharing your manual testing runtime results here?

lib/que/job.rb

lib/que/job.bulk_enqueue_spec.rb

lib/que/job.rb

ZimbiX · 2022-02-25T06:17:10Z

How about making the method take an option for whether to disable the notify while enqueueing the jobs?

oeoeaio · 2022-02-25T10:28:28Z

How about making the method take an option for whether to disable the notify while enqueueing the jobs?

Interesting idea. How would we do that without also impacting jobs enqueued using the normal enqueue method? The trigger is defined on the table...

ZimbiX · 2022-02-26T10:58:13Z

How would we do that without also impacting jobs enqueued using the normal enqueue method? The trigger is defined on the table...

I was thinking something like this - disable and re-enable the trigger in the call to .bulk_enqueue. If it's within a transaction, it wouldn't affect other parallel activities, right?

jasoncodes · 2022-02-27T00:40:34Z

Note that disabling/enabling triggers is a schema change, meaning it will require a ShareRowExclusiveLock. This will prevent row modifications while the transaction is open.

I’ve found adding a trigger condition which checks a custom setting works well to temporarily stop row triggers on bulk operations. Something like this:

CREATE TRIGGER que_job_notify
AFTER INSERT ON public.que_jobs FOR EACH ROW
WHEN (NOT coalesce(current_setting('que.skip_notify', true)::boolean, false))
EXECUTE FUNCTION public.que_job_notify();

Then use SET LOCAL within the transaction to skip the trigger:

BEGIN;
SET LOCAL que.skip_notify TO TRUE;
…
COMMIT;

ZimbiX · 2022-02-27T01:01:22Z

@jasoncodes Good to know! Thanks for the tip <3

Edit: The docs for disabling a trigger confirm this

ZimbiX · 2022-03-24T03:57:26Z

Rebased (via cherry-picking just the latest commit) on master now that the Ruby 3 PR has been merged

ZimbiX · 2022-07-22T04:14:12Z

@jasoncodes: @AndrewColbeck and I have had to adjust the query you provided, as we found it unfortunately errors after a transaction which sets the variable. It seems that current_setting returns empty string rather than null then, which breaks the casting to boolean:

➜ ./auto/psql
[+] Running 1/0
 ⠿ Container que-db-1  Running                                                                                                                                                                                                                                                 0.0s
psql (13.7 (Debian 13.7-1.pgdg110+1))
Type "help" for help.

que-test=# select coalesce(current_setting('que.skip_notify', true)::boolean, false);
 coalesce 
----------
 f
(1 row)

que-test=# begin; set local que.skip_notify to true; select coalesce(current_setting('que.skip_notify', true)::boolean, false); rollback;
BEGIN
SET
 coalesce 
----------
 t
(1 row)

ROLLBACK
que-test=# select coalesce(current_setting('que.skip_notify', true)::boolean, false);
ERROR:  invalid input syntax for type boolean: ""

Here's a working alternative that avoids the need to cast:

➜ ./auto/psql
[+] Running 1/0
 ⠿ Container que-db-1  Running                                                                                                                                                                                                                                                 0.0s
psql (13.7 (Debian 13.7-1.pgdg110+1))
Type "help" for help.

que-test=# select coalesce(current_setting('que.skip_notify', true), '') = 'true';
 ?column? 
----------
 f
(1 row)

que-test=# begin; set local que.skip_notify to true; select coalesce(current_setting('que.skip_notify', true), '') = 'true'; rollback;
BEGIN
SET
 ?column? 
----------
 t
(1 row)

ROLLBACK
que-test=# select coalesce(current_setting('que.skip_notify', true), '') = 'true';
 ?column? 
----------
 f
(1 row)

que-test=# begin; set local que.skip_notify to true; select coalesce(current_setting('que.skip_notify', true), '') = 'true'; commit;
BEGIN
SET
 ?column? 
----------
 t
(1 row)

COMMIT
que-test=# select coalesce(current_setting('que.skip_notify', true), '') = 'true';
 ?column? 
----------
 f
(1 row)

jasoncodes · 2022-07-22T04:20:22Z

Good catch. I can confirm this empty string behaviour on at least PG 14.x.

Here’s an alternate solution to consider: nullif(current_setting('que.skip_notify', true), '')::boolean

ZimbiX · 2022-07-22T06:04:42Z

Here’s an alternate solution to consider

Thanks for that idea. We tried it out, but it doesn't work - I think because casting null to boolean doesn't:

que-test=# select null::boolean;
 bool 
------
 
(1 row)

que-test=# select null::boolean is null;
 ?column? 
----------
 t
(1 row)

Sorry, I thought casting null to boolean was working before 😅

We've found these two options to be working:

WHEN (nullif(current_setting('que.skip_notify', true), '') is null)
WHEN (NOT coalesce(current_setting('que.skip_notify', true), '') = 'true')

We think we prefer the second form, given it's checking the value is set to something truthy rather than just set to something.

@jasoncodes

…mance Implementation is based on [a tip from @jasoncodes](que-rb#340 (comment))! Co-Authored-By: Andrew Colbeck <andrewcolbeck@gmail.com>

@jasoncodes

…ormance Implementation is based on [a tip from @jasoncodes](que-rb#340 (comment))! Co-Authored-By: Andrew Colbeck <andrewcolbeck@gmail.com>

ZimbiX · 2022-07-22T06:52:29Z

I've just realised there's a second trigger, que_state_notify, which would also be triggered when inserting each job record. I've briefly looked into that previously, and iirc, it wasn't actually used by anything. So we could think about removing it. Regardless, we should investigate it.

➜ ./auto/psql
[...]
que-test=# \d que_jobs
[...]
Triggers:
    que_job_notify AFTER INSERT ON que_jobs FOR EACH ROW WHEN (NOT COALESCE(current_setting('que.skip_notify'::text, true), ''::text) = 'true'::text) EXECUTE FUNCTION que_job_notify()
    que_state_notify AFTER INSERT OR DELETE OR UPDATE ON que_jobs FOR EACH ROW EXECUTE FUNCTION que_state_notify()

I think I'd still like to add an option in bulk enqueueing for whether to disable the notify (disabling it by default, I reckon).

The bulk_enqueue interface is rather verbose currently:

MyJob.bulk_enqueue(
  [
    { args: ['job1_arg1'], kwargs: { job1_kwarg1: 'x' } },
    { args: ['job2_arg1'], kwargs: { job2_kwarg1: 'x' } },
  ],
)

It could be good to improve that here or subsequently. I was wondering about something like:

Que::Job.with_bulk_enqueue do
  MyJob.enqueue('job1_arg1', job1_kwarg1: 'x')
  MyJob.enqueue('job2_arg1', job2_kwarg1: 'x')
end

jasoncodes · 2022-07-22T10:53:51Z

I think because casting null to boolean doesn't

Ah, true. The trigger condition must return true and I missed the NOT when proposing the above.

We think we prefer […] checking the value is set to something truthy rather than just set to something.

Agreed entirely. Comparing to 'true' as a string doesn’t feel quite right to me but is technically fine. This string comparison is what I was trying to avoid. One final version for your consideration:

WHEN (nullif(current_setting('que.skip_notify', true), '')::boolean IS DISTINCT FROM true)

I read this one as “run the trigger when when skip_notify is not true”.

jasoncodes · 2022-07-22T11:26:07Z

I think I'd still like to add an option in bulk enqueueing for whether to disable the notify (disabling it by default, I reckon).

I think disabled by default makes sense.

The bulk_enqueue interface is rather verbose currently.

Agreed. Not being able to use the normal Job enqueue methods is less than ideal too.

I was wondering about something like:

Que::Job.with_bulk_enqueue do
  MyJob.enqueue('job1_arg1', job1_kwarg1: 'x')
  MyJob.enqueue('job2_arg1', job2_kwarg1: 'x')
end

Yes, please! This would be is a much nicer interface and would allow users to adopt bulk enqueuing without a lot of changes to application code.

One could divert the insert_job statements into a thread-local array and flush that array in a single INSERT statement at the end of the with_bulk_enqueue block. This should reduce the need for much of the additional implementation code for this feature by re-using almost all of the existing enqueue code.

The main API difference I can think of is que_attrs won’t be available for use in the return value from enqueue. I’d probably return a job object with an empty hash when enqueue is called in bulk mode. This should ensure things like ActiveJob’s API still work correctly (but without provider_job_id being set).

ZimbiX · 2022-07-23T17:34:24Z

I've reworked the interface to use basically what I proposed, but still called .bulk_enqueue. In this form, we might as well add support for enqueueing jobs of different job classes / job_options; and doing so is probably not hard.

To keep track of everything that's left, I've just added a todo list to the first comment.

ZimbiX · 2022-07-23T17:43:40Z

One could divert the insert_job statements into a thread-local array and flush that array in a single INSERT statement at the end of the with_bulk_enqueue block. This should reduce the need for much of the additional implementation code for this feature by re-using almost all of the existing enqueue code.

Yeah, that's what I'd been thinking too 😉

The main API difference I can think of is que_attrs won’t be available for use in the return value from enqueue. I’d probably return a job object with an empty hash when enqueue is called in bulk mode. This should ensure things like ActiveJob’s API still work correctly (but without provider_job_id being set).

Ah, I hadn't thought of that! Cheers; done. I'm not really familiar with the ActiveJob part of Que - do you reckon we should add a test for that here?

ZimbiX · 2022-07-23T17:50:20Z

Agreed entirely. Comparing to 'true' as a string doesn’t feel quite right to me but is technically fine. This string comparison is what I was trying to avoid.

Yeah, but the settings are strings, so we can't really avoid string stuff afaik, having to use either string parsing or string comparison, of which I presume the performance is similar.

One final version for your consideration:

WHEN (nullif(current_setting('que.skip_notify', true), '')::boolean IS DISTINCT FROM true)

Interesting alternative. I think what we went with is slightly more readable, personally, so I'll stick with that. Thanks though

To avoid a ShareRowExclusiveLock, which would prevent row modifications while the transaction is open (que-rb#340 (comment))

@jasoncodes

…ormance Implementation is based on [a tip from @jasoncodes](que-rb#340 (comment))! Co-Authored-By: Andrew Colbeck <andrewcolbeck@gmail.com>

To avoid a ShareRowExclusiveLock, which would prevent row modifications while the transaction is open (que-rb#340 (comment))

ZimbiX · 2022-08-23T15:19:47Z

Rebased to avoid changelog conflict

Co-Authored-By: Andrew Colbeck <andrewcolbeck@gmail.com>

…s used It was consistently failing when run with: ```bash DATABASE_URL=postgres://que:que@localhost:5697/que-test TESTOPTS="--seed=50554 -v" USE_RAILS=true be rake spec ``` Co-Authored-By: Andrew Colbeck <andrewcolbeck@gmail.com>

@jasoncodes

…ormance Implementation is based on [a tip from @jasoncodes](que-rb#340 (comment))! Co-Authored-By: Andrew Colbeck <andrewcolbeck@gmail.com>

… 'PROCEDURE' for compatibility with older PostgreSQL

Allows fast-failing a test run using: ```bash TESTOPTS=--fail-fast bundle exec rake spec ```

…calls to .enqueue

It disables notify by default for performance

To avoid a ShareRowExclusiveLock, which would prevent row modifications while the transaction is open (que-rb#340 (comment))

This does not seem to be used

…eased section

…queue rather than .bulk_enqueue I added a matcher, `assert_raises_with_message` to make the expectation cleaner. I reckon this should have been part of minitest, like it is in RSpec

… args, and do so only at the end of the .bulk_enqueue block

…ecessary serialisation, making it ~19% faster

Co-Authored-By: Jerome Paul <jerome.paul@greensync.com.au>

Co-authored-by: Brendan Weibrecht <brendan@weibrecht.net.au>

[ActiveJob always adds job options to the Que.enqueue call](https://github.com/rails/rails/blob/main/activejob/lib/active_job/queue_adapters/que_adapter.rb#L25) but that is not supported. Furthermore ActiveJob does not have a dedicated bulk enqueue interface. Co-authored-by: Brendan Weibrecht <brendan@weibrecht.net.au>

@arguments

Using Que to manually enqueue an ActiveJob job is not just as simple being able to specify your job class in the `job_class` job option in `Que.enqueue` - there's a lot more to it, e.g.: ``` [2] pry(#<running jobs via ActiveJob>)> TestJobClass.perform_later(1, 2) => #<TestJobClass:0x00007fb081f4bcb0 @arguments=[1, 2], @exception_executions={}, @Executions=0, @job_id="227ffa28-57b2-4517-99ef-a53a4136c01a", @priority=nil, @provider_job_id=136557356, @queue_name="default", @Timezone=nil> [3] pry(#<running jobs via ActiveJob>)> jobs_dataset.all => [{:priority=>100, :run_at=>2022-08-25 06:25:15.406266 +0000, :id=>136557356, :job_class=>"ActiveJob::QueueAdapters::QueAdapter::JobWrapper", :error_count=>0, :last_error_message=>nil, :queue=>"default", :last_error_backtrace=>nil, :finished_at=>nil, :expired_at=>nil, :args=> [{:job_id=>"227ffa28-57b2-4517-99ef-a53a4136c01a", :locale=>"en", :priority=>nil, :timezone=>nil, :arguments=>[1, 2], :job_class=>"TestJobClass", :executions=>0, :queue_name=>"default", :enqueued_at=>"2022-08-25T06:25:15Z", :provider_job_id=>nil, :exception_executions=>{}}], :data=>{}, :job_schema_version=>2, :kwargs=>{}}] ``` Co-Authored-By: Jerome Paul <jerome.paul@greensync.com.au>

Co-Authored-By: Jerome Paul <jerome.paul@greensync.com.au>

ZimbiX · 2022-08-25T06:50:28Z

🚀

oeoeaio changed the title ~~Bulk enqueue~~ Support bulk enqueuing of jobs which differ only in args/kwargs Feb 25, 2022

ZimbiX requested changes Feb 25, 2022

View reviewed changes

lib/que/job.rb Outdated Show resolved Hide resolved

lib/que/job.bulk_enqueue_spec.rb Outdated Show resolved Hide resolved

lib/que/job.rb Outdated Show resolved Hide resolved

oeoeaio force-pushed the bulk-enqueue branch from af7cdb1 to 3f77f26 Compare March 17, 2022 09:34

ZimbiX force-pushed the bulk-enqueue branch from 3f77f26 to dab078b Compare March 24, 2022 03:57

ZimbiX force-pushed the bulk-enqueue branch from dab078b to 465ec96 Compare July 19, 2022 07:31

ZimbiX force-pushed the bulk-enqueue branch from e8daa44 to 996736d Compare July 22, 2022 06:10

ZimbiX force-pushed the bulk-enqueue branch from 996736d to 9bc68cb Compare July 22, 2022 06:11

ZimbiX added a commit to greensync/que that referenced this pull request Jul 23, 2022

Docs: Switch mass job deletion instructions to using que.skip_notify

b022545

To avoid a ShareRowExclusiveLock, which would prevent row modifications while the transaction is open (que-rb#340 (comment))

ZimbiX mentioned this pull request Aug 23, 2022

Remove unnecessary trigger that notifies about job state transitions #372

Open

ZimbiX added a commit to greensync/que that referenced this pull request Aug 23, 2022

Docs: Switch mass job deletion instructions to using que.skip_notify

e19bad3

To avoid a ShareRowExclusiveLock, which would prevent row modifications while the transaction is open (que-rb#340 (comment))

ZimbiX force-pushed the bulk-enqueue branch from 6f49e2f to 24e722a Compare August 23, 2022 15:17

ZimbiX and others added 21 commits August 25, 2022 16:10

Bulk enqueue: Support omitting args/kwargs in bulk_enqueue

d2f8a2a

Co-Authored-By: Andrew Colbeck <andrewcolbeck@gmail.com>

Bulk enqueue: Disable que_job_notify trigger in bulk_enqueue for perf…

6775950

…ormance Implementation is based on [a tip from @jasoncodes](que-rb#340 (comment))! Co-Authored-By: Andrew Colbeck <andrewcolbeck@gmail.com>

que_job_notify trigger: Switch SQL from 'FUNCTION' back to deprecated…

acb730e

… 'PROCEDURE' for compatibility with older PostgreSQL

Add gem: minitest-fail-fast

24fdade

Allows fast-failing a test run using: ```bash TESTOPTS=--fail-fast bundle exec rake spec ```

Bulk enqueue: Rework interface to use block form instead with normal …

b7edc93

…calls to .enqueue

Bulk enqueue: Namespace thread-local variable

b5baa88

Bulk enqueue: Add option to provide control over whether notify happens

2dd226b

It disables notify by default for performance

Bulk enqueue: Remove unnecessary line

3577fe5

Bulk enqueue: Document usage

5fa6d72

Docs: Switch mass job deletion instructions to using que.skip_notify

556a8b4

To avoid a ShareRowExclusiveLock, which would prevent row modifications while the transaction is open (que-rb#340 (comment))

Bulk enqueue: Allow not enqueueing any jobs within a .bulk_enqueue block

4f796aa

Bulk enqueue: Skip notify in que_state_notify trigger as well

4e806ed

This does not seem to be used

Changelog: Add bulk enqueue and que_state_notify deprecation to unrel…

8432e80

…eased section

Bulk enqueue: Add test for raising when job_options are passed to .en…

065a6e8

…queue rather than .bulk_enqueue I added a matcher, `assert_raises_with_message` to make the expectation cleaner. I reckon this should have been part of minitest, like it is in RSpec

Bulk enqueue: Fix executing jobs in synchronous mode with the correct…

906b415

… args, and do so only at the end of the .bulk_enqueue block

Bulk enqueue: Reorganise serialisation & deserialisation to avoid unn…

1a01099

…ecessary serialisation, making it ~19% faster

Changelog: Note for 2.0.0 not to upgrade straight to Que 2

13244da

Co-Authored-By: Jerome Paul <jerome.paul@greensync.com.au>

Changelog: Add database migration instructions to unreleased section

c1a84cf

Co-Authored-By: Jerome Paul <jerome.paul@greensync.com.au>

Run the ActiveRecord specs as well when running scripts/test

4766433

Co-authored-by: Brendan Weibrecht <brendan@weibrecht.net.au>

ZimbiX force-pushed the bulk-enqueue branch from ff1e6ad to 3034fb8 Compare August 25, 2022 06:21

ZimbiX and others added 2 commits August 25, 2022 16:41

Bulk enqueue: Note in docs that ActiveJob is not supported

297c6cc

Co-Authored-By: Jerome Paul <jerome.paul@greensync.com.au>

ZimbiX approved these changes Aug 25, 2022

View reviewed changes

ZimbiX merged commit f0dab0b into que-rb:master Aug 25, 2022

ZimbiX deleted the bulk-enqueue branch August 25, 2022 06:50

oeoeaio mentioned this pull request Dec 20, 2022

Update migration examples in README to specify the latest version (7) #387

Merged

dtcristo mentioned this pull request Mar 4, 2024

Support bulk enqueue with differing class and options #420

Open

Comments

Conversation

oeoeaio commented Feb 25, 2022 • edited by ZimbiX Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZimbiX left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZimbiX commented Feb 25, 2022

Uh oh!

oeoeaio commented Feb 25, 2022

Uh oh!

ZimbiX commented Feb 26, 2022

Uh oh!

jasoncodes commented Feb 27, 2022

Uh oh!

ZimbiX commented Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZimbiX commented Mar 24, 2022

Uh oh!

ZimbiX commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasoncodes commented Jul 22, 2022

Uh oh!

ZimbiX commented Jul 22, 2022

Uh oh!

ZimbiX commented Jul 22, 2022

Uh oh!

jasoncodes commented Jul 22, 2022

Uh oh!

jasoncodes commented Jul 22, 2022

Uh oh!

ZimbiX commented Jul 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZimbiX commented Jul 23, 2022

Uh oh!

ZimbiX commented Jul 23, 2022

Uh oh!

ZimbiX commented Aug 23, 2022

Uh oh!

ZimbiX commented Aug 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oeoeaio commented Feb 25, 2022 •

edited by ZimbiX

Loading

ZimbiX left a comment •

edited

Loading

ZimbiX commented Feb 27, 2022 •

edited

Loading

ZimbiX commented Jul 22, 2022 •

edited

Loading

ZimbiX commented Jul 23, 2022 •

edited

Loading