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.

Sign up for GitHub

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

Fix the issue that PIVOT execution not return row affected line #2071

Merged
merged 2 commits into from
Nov 28, 2023
Merged

Fix the issue that PIVOT execution not return row affected line #2071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf94b82
Fix the issue that PIVOT execution not return row affected line
Nov 28, 2023
c58216c
change comment
Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions contrib/babelfishpg_tsql/src/pl_exec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4693,6 +4693,12 @@ exec_stmt_execsql(PLtsql_execstate *estate,
return ret;
}

if (expr->plan && expr->plan->oneshot)
{
SPI_freeplan(expr->plan);
expr->plan = NULL;
}
Comment on lines +4696 to +4700
Copy link
Contributor

Choose a reason for hiding this comment

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

This code isn't pivot specific anymore so any generic expr->plan->oneshot will go here. Has that been tested?

Copy link
Contributor Author

@forestkeeper forestkeeper Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Yes, it has passed all known cases
  2. As for the oneshot variable, is it specific in this case ? My understand is yes, since in PG execution whether oneshot is defined at spi_prepare stage, and it'll release in plan execution ends. But our usage is define at end of execution, and free plan at the begining of query before preparing. So they don't have overlap in this case.


if (expr->plan == NULL)
{
/*
Expand Down Expand Up @@ -5056,11 +5062,10 @@ exec_stmt_execsql(PLtsql_execstate *estate,
/* If query affects IDENTITY_INSERT relation then update sequence */
pltsql_update_identity_insert_sequence(expr);

/* If current plan constains a pivot operator, we remove the plan */
/* If current plan constains a pivot operator, we set it as execute oneshot */
if (is_pivot)
{
SPI_freeplan(expr->plan);
expr->plan = NULL;
expr->plan->oneshot = true;
}

/* Expect SPI_tuptable to be NULL else complain */
Expand Down
2 changes: 2 additions & 0 deletions test/JDBC/expected/pivot-vu-verify.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,8 @@ PIVOT (
) AS pvt;
SELECT TOP 10 * FROM pivot_insert_into ORDER by 1, 2;
GO
~~ROW COUNT: 197~~
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't TOP 10 return only 10 rows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This row count is corresponding to the insert select, and it insert 197 lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one :

INSERT INTO pivot_insert_into
SELECT  ManufactureID, EmployeeID, [2] AS STORE2, [3] AS STORE3, [4] AS STORE4, [5] AS STORE5, [6] AS STORE6
FROM
(
    SELECT ManufactureID, EmployeeID, ItemID, StoreID
    FROM StoreReceipt
)AS srctable
PIVOT (
    COUNT (ItemID)
    FOR StoreID IN ([2], [3], [4], [5], [6])
) AS pvt;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The TOP 10 is another statement

Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't TOP 10 return only 10 rows?

There's a long-standing issue with SELECT-INTO that it does not report the row count (though @@rowcount is correct). See BABEL-540


~~START~~
int#!#int#!#int#!#int#!#int#!#int#!#int
1200#!#200#!#0#!#0#!#0#!#0#!#0
Expand Down
Loading