Add timestamp support in send and send_batch

[Neptune650]

/claim #315
Closes #315

This pull request creates two functions - send_at and send_batch_at - which receive a timestamp and convert it to seconds from now.

This also documents both functions.

[Neptune650]

@ChuckHend So is there anything else left to work on or can this be merged now?

[ChuckHend]

@ChuckHend So is there anything else left to work on or can this be merged now?

Test cases would be great!

[Neptune650]

@ChuckHend So is there anything else left to work on or can this be merged now?

Test cases would be great!

I figured it wouldn't be necessary since send with normal seconds delay isn't tested.

[ChuckHend]

You are correct, we don't have a test to ensure that the delay feature continues to work as intended, but there are tests calling the function (in the future we should add tests to ensure delay continues to work as intended on send() too). Please add test cases for the new functions that are introduced in this PR.

[Neptune650]

@ChuckHend I have added tests, tell me your thoughts.

[v0idpwn]

I think it would probably be a better API if instead send and send_batch were overloaded to support both a delay and a timestamp.

[Neptune650]

I think it would probably be a better API if instead send and send_batch were overloaded to support both a delay and a timestamp.

@v0idpwn I've already talked with ChuckHend about this

And an issue remains and that is, which function to run when you only put 2 arguments

[v0idpwn]

Sorry, missed it because it was resolved :)

[tavallaie]

if both variable are not none, you can simple raise an exception to user that those variable can not be use together.

[tavallaie]

another question if user add delay can change it to vt or vise versa? what will happened? what we except?
what scenario use delay or vt or both?

[Neptune650]

if both variable are not none, you can simple raise an exception to user that those variable can not be use together.

No, the issue is that Postgres will raise an exception if there are two overloading functions with the only difference being a default parameter.

[Neptune650]

another question if user add delay can change it to vt or vise versa? what will happened? what we except? what scenario use delay or vt or both?

What do you mean?
The user can use delays and change it to vt yes, that's always been an option. The other way around can be done too with CURRENT_TIMESTAMP. It's up to the user to use timestamps or delays.

[Neptune650]

Is there anything else I'm missing or is this PR done now?

[tavallaie]

if both variable are not none, you can simple raise an exception to user that those variable can not be use together.

No, the issue is that Postgres will raise an exception if there are two overloading functions with the only difference being a default parameter.

Sorry, but I can't understand why we can't use delay as an option beside vt with default none or zero?

[Neptune650]

if both variable are not none, you can simple raise an exception to user that those variable can not be use together.

No, the issue is that Postgres will raise an exception if there are two overloading functions with the only difference being a default parameter.

Sorry, but I can't understand why we can't use delay as an option beside vt with default none or zero?

Look, if we overload two functions like this

pgmq.send_batch( 
     queue_name text, 
     msgs jsonb[], 
     delay timestamp DEFAULT CURRENT_TIMESTAMP
 );

pgmq.send_batch( 
     queue_name text, 
     msgs jsonb[], 
     delay integer DEFAULT 0
 );

Postgres will raise an exception because it won't know which one to run if you just put the first two parameters.

[Neptune650]

@ChuckHend Fixed tests, tell me if there's anything missing now.

[v0idpwn]

@ChuckHend @tavallaie not sure what should be done with the python client after these changes :)

[v0idpwn]

@Neptune650 I pushed the following changes:

• Overload pgmq.send/pgmq.send_batch instead of adding new send_at and send_batch_at
• Refactor to remove code duplication
• Remove unnecessary timestamp -> duration -> timestamp conversion
• Take timestamp with tz instead of timestamp without tz

I think the extension code is fine now. We just need to figure what to do in the python api.

[Neptune650]

@Neptune650 I pushed the following changes:

• Overload pgmq.send/pgmq.send_batch instead of adding new send_at and send_batch_at
• Refactor to remove code duplication
• Remove unnecessary timestamp -> duration -> timestamp conversion

I think the extension code is fine now. We just need to figure what to do in the python api.

@v0idpwn All sounds good to me, I was thinking we could leave the Python function names as is, while internally calling the overloaded pgmq.send

[tavallaie]

@Neptune650 I pushed the following changes:

• Overload pgmq.send/pgmq.send_batch instead of adding new send_at and send_batch_at
• Refactor to remove code duplication
• Remove unnecessary timestamp -> duration -> timestamp conversion
• Take timestamp with tz instead of timestamp without tz

I think the extension code is fine now. We just need to figure out what to do in the python API.

I think we can add parameters of delay and ts with a default value of none; check if those, too are not none and use overloaded functions instead.

[Neptune650]

@tavallaie I have pushed the changes you suggested, please tell me what you think

[tavallaie]

@Neptune650 did you update unit tests too?

[Neptune650]

@Neptune650 did you update unit tests too?

@tavallaie There's no need, they're working fine without changes.

[tavallaie]

@Neptune650 did you update unit tests too?

@tavallaie There's no need, they're working fine without changes.

They are working because not supporting new scenarios like delay

For adding new scenarios we need new tests.

[ChuckHend]

@Neptune650 , @tavallaie thoughts on moving the python changes to a new PR and focus on getting the SQL API changes approved? Also, while the python client changes are very helpful IMO they are out of scope to the bounty that @Neptune650 is working on.

[tavallaie]

@Neptune650 , @tavallaie thoughts on moving the python changes to a new PR and focus on getting the SQL API changes approved? Also, while the python client changes are very helpful IMO they are out of scope to the bounty that @Neptune650 is working on.

Ok.

[Neptune650]

@Neptune650 , @tavallaie thoughts on moving the python changes to a new PR and focus on getting the SQL API changes approved? Also, while the python client changes are very helpful IMO they are out of scope to the bounty that @Neptune650 is working on.

@ChuckHend Sounds good to me, I can create another PR for the Python changes (I'd appreciate if it was with a separate tip but I'll send it either way)

[v0idpwn]

@Neptune650 would you please revert the Python changes, then? Then I can merge.

[Neptune650]

@v0idpwn I reverted the changes, please merge.

[tavallaie]

@Neptune650 , @tavallaie thoughts on moving the python changes to a new PR and focus on getting the SQL API changes approved? Also, while the python client changes are very helpful IMO they are out of scope to the bounty that @Neptune650 is working on.

@ChuckHend Sounds good to me, I can create another PR for the Python changes (I'd appreciate if it was with a separate tip but I'll send it either way)

If you are busy, I can do that.

[Neptune650]

@Neptune650 , @tavallaie thoughts on moving the python changes to a new PR and focus on getting the SQL API changes approved? Also, while the python client changes are very helpful IMO they are out of scope to the bounty that @Neptune650 is working on.

@ChuckHend Sounds good to me, I can create another PR for the Python changes (I'd appreciate if it was with a separate tip but I'll send it either way)

If you are busy, I can do that.

@tavallaie I've created a PR for it #333

[ChuckHend]

Thank you @Neptune650

[Neptune650]

Thank you @Neptune650

@ChuckHend No problem - please merge if ready.