-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement message headers #338
Conversation
Implement message headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks straightforward, but please:
- Add an upgrade script that can upgrade the SQL from the previous version to this version's change. Details here.
- Add tests for the new functions and behaviors.
Thanks!
Of course, this is an API proposal, not ready yet :) |
Ah, got it, didn't realize, thanks for clarifying. |
(Consider converting this to a Draft PR) |
-- send: 4 args with integer delay | ||
CREATE FUNCTION pgmq.send( | ||
queue_name TEXT, | ||
msg JSONB, | ||
headers JSONB, | ||
delay INTEGER | ||
) RETURNS SETOF BIGINT AS $$ | ||
SELECT * FROM pgmq.send(queue_name, msg, headers, clock_timestamp() + make_interval(secs => delay)); | ||
$$ LANGUAGE sql; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider replacing the variants with just two using defaults:
-- send: 4 args with integer delay
CREATE FUNCTION pgmq.send(
queue_name TEXT,
msg JSONB,
headers JSONB DEFAULT = NULL,
delay INTEGER DEFAULT = 0
) RETURNS SETOF BIGINT AS $$
SELECT * FROM pgmq.send(queue_name, msg, headers, clock_timestamp() + make_interval(secs => delay));
$$ LANGUAGE sql;
CREATE FUNCTION pgmq.send(
queue_name TEXT,
msg JSONB,
headers JSONB DEFAULT = NULL,
delay TIMESTAMP WITH TIME ZONE = clock_timestamp(),
) RETURNS SETOF BIGINT AS $$
SELECT * FROM pgmq.send(queue_name, msg, headers, delay);
$$ LANGUAGE sql;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not equivalent. I'm accepting pgmq.send(queue_name, msg, integer/timestamp) too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theory, I think your suggestion results in a breaking API change? The 3-argument calls would become ambiguous.
I tried the suggestion locally and then ran tests...
+ERROR: function pgmq.send(unknown, unknown, jsonb) is not unique
+LINE 1: SELECT * from pgmq.send('test_unlogged_queue', '{"hello": "w...
+ ^
+HINT: Could not choose a best candidate function. You might need to add explicit type casts.
If this is the case, I would like hold on the breaking API changes v2.0 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
SELECT * FROM pgmq.send_batch(queue_name, msgs, NULL, clock_timestamp() + make_interval(secs => delay)); | ||
$$ LANGUAGE sql; | ||
|
||
-- send batch: 3 args with timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, try using DEFAULT
expressions in parameter lists to reduce the number of variants.
i can help for merging this? |
We do need some tests! |
I think it's complete now |
END; | ||
$$ LANGUAGE plpgsql; | ||
|
||
-- Update existing queues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ I tested locally, including upgrading from v1.4.5
and did not see any issues.
@theory requested changes though, so would like to give him a chance to respond before merging.
Closes #203
Needed for #294