Skip to content

pgwire: fix txn options for prepared BEGIN #87047

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
rafiss:fix-begin-implicit
Aug 30, 2022
Merged

pgwire: fix txn options for prepared BEGIN #87047
craig[bot] merged 2 commits intocockroachdb:masterfrom
rafiss:fix-begin-implicit

Conversation

@rafiss
Copy link
Copy Markdown
Collaborator

@rafiss rafiss commented Aug 29, 2022

fixes #87012

Release note (bug fix): The statement tag for SHOW command
results in the pgwire protocol no longer contain the number of returned
rows.

Release note (bug fix): Fixed a bug where the options given to the BEGIN
TRANSACTION command would be ignored if the BEGIN was a prepared
statement.

Release justification: low risk bug fix

Release note (bug fix): The statement tag for SHOW command
results in the pgwire protocol no longer contain the number of returned
rows.

Release justification: low risk bug fix
@rafiss rafiss requested review from a team and ZhouXing19 August 29, 2022 17:40
@rafiss rafiss requested a review from a team as a code owner August 29, 2022 17:40
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@rafiss rafiss changed the title pgwire: don't show RowsAffected for SHOW command pgwire: fix txn options for prepared BEGIN Aug 29, 2022
{"Type":"ReadyForQuery","TxStatus":"I"}

until noncrdb_only
until
Copy link
Copy Markdown
Collaborator

@ZhouXing19 ZhouXing19 Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious why we are no longer going to return the row number with SHOW now, is it just to be consistent with postgres's behavior?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that was the reason

@rafiss rafiss force-pushed the fix-begin-implicit branch from 0e62147 to c35ae78 Compare August 29, 2022 17:57
@rafiss rafiss requested a review from ZhouXing19 August 29, 2022 21:17

until ignore=RowDescription
ReadyForQuery
ReadyForQuery
Copy link
Copy Markdown
Collaborator

@ZhouXing19 ZhouXing19 Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should add BEGIN AS OF SYSTEM TIME test in batch_stmt too? I see it in implicit_txn but not here. Any particular reason for that?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, let me see if that can be done. i didn't do it initially, since the AS OF SYSTEM TIME can't change after the txn has performed reads/writes

Release note (bug fix): Fixed a bug where the options given to the BEGIN
TRANSACTION command would be ignored if the BEGIN was a prepared
statement.

Release justification: low risk bug fix.
@rafiss rafiss force-pushed the fix-begin-implicit branch from c35ae78 to d6009e3 Compare August 30, 2022 13:47
@rafiss rafiss requested a review from ZhouXing19 August 30, 2022 13:47
Copy link
Copy Markdown
Collaborator

@ZhouXing19 ZhouXing19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@rafiss
Copy link
Copy Markdown
Collaborator Author

rafiss commented Aug 30, 2022

tftr!

bors r=ZhouXing19

@craig craig bot merged commit f4b491f into cockroachdb:master Aug 30, 2022
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 30, 2022

Build succeeded:

@rafiss rafiss deleted the fix-begin-implicit branch August 30, 2022 17:13
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

Successfully merging this pull request may close these issues.

Transaction not read only if BEGIN READ ONLY is executed on advanced protocol

3 participants