Skip to content
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

Omni test matrix broken for BatchedInsertReWriteEnabledTest #2528

Open
sehrope opened this issue Jun 3, 2022 · 3 comments
Open

Omni test matrix broken for BatchedInsertReWriteEnabledTest #2528

sehrope opened this issue Jun 3, 2022 · 3 comments

Comments

@sehrope
Copy link
Member

sehrope commented Jun 3, 2022

Latest error log is here: https://github.com/pgjdbc/pgjdbc/runs/6721064685?check_suite_focus=true#step:11:282

I think the break is caused by the recently merged #2525. Either that PR changed some historic behavior or the test itself is broken / relying on undocumented behavior.

@vlsi
Copy link
Member

vlsi commented Jun 3, 2022

Well, what happens there is in simple mode, every "bind" is sent as a literal, so there's no technical limitation on the number of binds in a query, so "insert query rewrite" happily rewrites insert into multi-value insert even for the case like 65535 binds.

However, I have two observations here:

  1. The issue is detected too late. We could detect it earlier during PR development. I think a randomized matrix would have detected the issue during PR CI.
  2. We probably need to limit the number of "supported binds" even in simple mode

@sehrope
Copy link
Member Author

sehrope commented Jun 3, 2022

The issue is detected too late. We could detect it earlier during PR development. I think a randomized matrix would have detected the issue during PR CI.

A randomized matrix might help randomly catch things but it's not guaranteed so not particularly intriguing.

I think it's the responsibility of the maintainer who is merging the PR to decide if the full matrix needs to be run before merging something. It's possible to manually run the action for a specific branch. I don't think it's possible to instruct it to use a PR itself, but since the PR has to come from a fork of this repo, the initiator or reviewer could run it in their fork.

Regardless, I don't think it's too bad as it is now. The omni matrix runs once a day and catching something like this after it's merged but before it's shipped in a release is fine. We really shouldn't be shipping any releases that don't pass the omni matrix anyway. Either something is broken or a test a broken, either way it needs to be addressed before publishing a version.

We probably need to limit the number of "supported binds" even in simple mode

+1 to consistency. Things that only work (or don't!) when internal flags are toggled are a recipe for odd bugs.

@vlsi
Copy link
Member

vlsi commented Jun 5, 2022

+1 to consistency. Things that only work (or don't!) when internal flags are toggled are a recipe for odd bugs.

I'm not sure about this.

I'm ok if we hit a database limit, so we can tell the users "we can't overcome the limit".
On the other hand, simple mode does not use bind variables, so there's no limit from the database besides query length. So I am not sure if we should impose the limit of 65535 parameters even for simple mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants