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

Jump to bottom

Initial `RETURNING` implementation #107

Open

mkgrgis wants to merge 16 commits into pgspider:master from mkgrgis:returning

Contributor

mkgrgis commented Dec 24, 2024 •

edited

Loading

SQL RETURNING have been introduced in SQLite 3.35.0.

SQLite's syntax for RETURNING is modelled after PostgreSQL.

I haven't noticed any special behaviour for RETURNING in SQLite not mapped to PostgreSQL RETURNING behaviour.

This PR made as mechanical edition according postgres_fdw code structures including naming conventions. This was enough for full ISO:SQL INSERT ... RETURNING and UPDATE ... RETURNING implementation. In case of DELETE ... RETURNING there are some problems and warning message was implemented.

Implemented after #56


          Initial RETURNING implementation

cb754ac

mkgrgis mentioned this pull request

RETURNING support #56

Open

lamdn1409 reviewed

View reviewed changes

expected/17.0/extra/returning.out

+              --Testcase 06:
+               EXPLAIN (VERBOSE, COSTS OFF)
+              UPDATE "type_STRING" SET col = '_' || substr(col, 2) RETURNING *;

Contributor

lamdn1409 Dec 24, 2024

You described "RETURNING have been introduced in SQLite 3.35.0", so we expect RETURNING clause will be pushed down to SQLite. Could you implement this point?

Contributor Author

mkgrgis Dec 25, 2024

I have begun in d339f8c , but removing a pushing down stopper is not enough, please see #107 (comment).

lamdn1409 reviewed

View reviewed changes

expected/17.0/extra/returning.out Outdated

+              --Testcase 09:
+               EXPLAIN (VERBOSE, COSTS OFF)
+              DELETE FROM "type_STRING" RETURNING *;
+              WARNING:  DELETE ... RETURNING doesn't return values (not implemented)

Contributor

lamdn1409 Dec 24, 2024

WARNING: DELETE ... RETURNING doesn't return values (not implemented)

SQLite FDW already supported Direct Modification, I think RETURNING clause can be pushed down along with DELETE command. Could you implement this point?

Contributor Author

mkgrgis Dec 25, 2024

I am trying to implement universal (IUD) pushing down now.

Contributor Author

mkgrgis Dec 30, 2024

Implemented except to TupleDesc processing after RETURNING for some TCs, please verify by EXPLAIN results.

lamdn1409 reviewed

View reviewed changes

expected/17.0/extra/returning.out

+              --Testcase 11:
+               EXPLAIN (VERBOSE, COSTS OFF)
+              INSERT INTO "type_STRING"(col) VALUES ('string') ON CONFLICT DO NOTHING RETURNING *;

Contributor

lamdn1409 Dec 24, 2024

Currently, SQLiteFDW does not support ON CONFLICT DO NOTHING clause, I think you should separate this feature to another Pull Request or remove this its test and implementation.

Contributor Author

mkgrgis Dec 25, 2024

@lamdn1409 , why this is a problem? I thinks this is just INSERT function tested and small internal detail or this touch some other code layers?

lamdn1409 reviewed

View reviewed changes

expected/17.0/extra/returning.out

+              --Testcase 35:
+               EXPLAIN (VERBOSE, COSTS OFF)
+              INSERT INTO "type_STRING"(col) VALUES ('string') RETURNING *;

Contributor

lamdn1409 Dec 24, 2024

Similar to #107 (comment), INSERT ... RETURNING can be pushed down to SQLite.

Contributor Author

mkgrgis Dec 30, 2024

Please verify EXPLAIN commands. is this enough in current TCs?

lamdn1409 reviewed

View reviewed changes

expected/17.0/extra/returning.out Outdated

+              --Testcase 49:
+              INSERT INTO "type_DATE"(col) VALUES ('2021.02.23') RETURNING col;
+              ERROR:  Failed to execute remote SQL

Contributor

lamdn1409 Dec 24, 2024

Could you add comment for the failed test cases below? What is your test purpose?

lamdn1409 reviewed

View reviewed changes

sql/17.0/extra/returning.sql Outdated

+              --Testcase 46:
+              INSERT INTO "type_BLOB"(col) VALUES (bytea('\xDEADBEEF')) RETURNING *;
+              --Testcase 47:
+              INSERT INTO typetest VALUES(1,'a', 'b', 'c','2017.11.06 12:34:56.789', '2017.11.06 12:34:56.789' ) RETURNING *;

Contributor

lamdn1409 Dec 24, 2024

Could you move this test case to the end of the test file (after testcase 62 and before testcase 47)?

Contributor Author

mkgrgis Dec 25, 2024

Done, please verify.

lamdn1409 reviewed

View reviewed changes

sql/17.0/extra/returning.sql

+              SELECT * FROM type_JSON;
+              --Testcase 62
+              DELETE FROM type_JSON RETURNING *;

Contributor

lamdn1409 Dec 24, 2024

According to the specification https://www.sqlite.org/lang_returning.html, could you please add more test cases regarding RETURNING to verify the following points?

INSERT/DELETE/UPDATE combine RETURNING

RETURNING expr
RETURNING expr1, expr2,...
RETURNING expr AS column-alias
RETURNING expr1 AS column-alias1, expr2 AS column-alias2,...
The RETURNING clause does not report any additional database changes caused by foreign key constraints or triggers.
The output order for the RETURNING rows is arbitrary and is not necessarily related to the order in which the rows were processed internally.
07 items of Limitations And Caveats

Contributor Author

mkgrgis Dec 25, 2024

Will TC 60 ..100 enough?

lamdn1409 reviewed

View reviewed changes

sql/17.0/extra/returning.sql Outdated

+              --Testcase 62
+              DELETE FROM type_JSON RETURNING *;
+              --Testcase 47:

Contributor

lamdn1409 Dec 24, 2024

According to the test of postgres_fdw, please add test cases to verify the following points.

RETURNING table, table.*
RETURNING constant
DELETE/UPDATE + WHERE + RETURNING
DELETE/UPDATE + JOIN + WHERE + RETURNING

Contributor Author

mkgrgis Dec 25, 2024

RETURNING constant
DELETE/UPDATE + WHERE + RETURNING

Done. I'll prepare data for other tests.

Contributor Author

mkgrgis Dec 30, 2024

Please check TC 100-110 https://github.com/mkgrgis/sqlite_fdw/blob/e4592d21a8034a11b96cdde9439117ebd47cd89b/expected/17.0/extra/returning.out#L780

Contributor

lamdn1409 commented Dec 24, 2024

Could you add description about the feature to README.md file?

lamdn1409 reviewed

View reviewed changes

deparse.c Outdated Show resolved Hide resolved

lamdn1409 reviewed

View reviewed changes

deparse.c

+              /*
+               * Add a RETURNING clause, if needed, to an INSERT/UPDATE/DELETE.
+               */
+              static void

Contributor

lamdn1409 Dec 24, 2024

Could you add definition for this function (prototype)?

Contributor Author

mkgrgis Dec 25, 2024

Please verify, @lamdn1409 !

Contributor

lamdn1409 Dec 25, 2024

Please rename the function with the prefix sqlite_.

Contributor Author

mkgrgis Dec 25, 2024

Done, please confirm, @lamdn1409. I see this prefix.

lamdn1409 reviewed

View reviewed changes

deparse.c

+              	Bitmapset  *attrs_used = NULL;
+              	elog(DEBUG3, "sqlite_fdw : %s", __func__);
+              	if (trig_after_row)

Contributor

lamdn1409 Dec 24, 2024

Could you add test case to cover this condition?

Contributor Author

mkgrgis Dec 30, 2024

@lamdn1409 , I don't understand how can I test this trig_after_row flag. What this is about in our context?

lamdn1409 reviewed

View reviewed changes

deparse.c

+              			bms_make_singleton(0 - FirstLowInvalidHeapAttributeNumber);
+              	}
+              	if (withCheckOptionList != NIL)

Contributor

lamdn1409 Dec 24, 2024

Could you add test case to cover this condition?

Contributor Author

mkgrgis Dec 30, 2024

Is this about view after tested table https://www.postgresql.org/docs/17/sql-createview.html + https://www.postgresql.jp/document/16/html/fdw-callbacks.html#FDW-CALLBACKS-UPDATE ? Will this https://neon.tech/postgresql/postgresql-views/postgresql-views-with-check-option relevant TCs?

Contributor

lamdn1409 commented Dec 24, 2024

SQL RETURNING have been introduced in SQLite 3.35.0.

SQLite's syntax for RETURNING is modelled after PostgreSQL.

I haven't noticed any special behaviour for RETURNING in SQLite not mapped to PostgreSQL RETURNING behaviour.

This PR made as mechanical edition according postgres_fdw code structures including naming conventions. This was enough for full ISO:SQL INSERT ... RETURNING and UPDATE ... RETURNING implementation. In case of DELETE ... RETURNING there are some problems and warning message was implemented.

Implemented after #56

@mkgrgis Thanks for your contribution. I'm happy to work with you to support this feature. I have some review comments, could you please check them?

Contributor Author

mkgrgis commented Dec 24, 2024

Hello, @lamdn1409 ! Thanks for interesting review! Because this was mechanical edit by postgres_fdw, I wasn't ready to think about pushing down. Now I begin special small research. Maybe pushing down resolve our DELETE... problem in this PR.


          Remove RETURNING pushing down stopper, follow postgres_fdw naming

d339f8c

Contributor Author

mkgrgis commented Dec 25, 2024

@lamdn1409, could you please help me to do a resume about code problem?
Inside of sqlite_deparse{IUD}Sql there is sqlite_deparseReturningList, inside of sqlite_deparseReturningList there is sqlite_deparseTargetList with (buf, root, rtindex, rel, true, ... parameters. This true is is_returning, but inside of sqlite_deparseTargetList https://github.com/mkgrgis/sqlite_fdw/blob/d339f8ce9fea91fe647ead4d79196472723968b1/deparse.c#L2139 is not effective. This code doesn't work as in postgres_fdw. Now I cannot understand why and have no versions about the problem.

mkgrgis added 2 commits

December 25, 2024 07:27


          Add TCs

8f02481


          Add prorotype + add to README + Tcs

04a9ef8

Contributor Author

mkgrgis commented Dec 25, 2024

Could you add description about the feature to README.md file?

Yes, no problems. Now there is short sentence after 04a9ef8

mkgrgis added 2 commits

December 25, 2024 07:59


          Fix ;

d7070a7


          README, add condAttr function, reduce doubled code sqlite_fdw.c

63b23f9

Contributor Author

mkgrgis commented Dec 25, 2024

resume about code problem?

Look like there is something in sqlitePlanForeignModify, but I still cannot pin the problem code lines.

Contributor Author

mkgrgis commented Dec 25, 2024

@lamdn1409 , look like there is some problems with regressed and deleted FdwModifyPrivateHasReturning, /* Integer list of attribute numbers retrieved by RETURNING */ in enum FdwModifyPrivateIndex. Also SqliteFdwDirectModifyState can be affected, but It's still hard to pin concrete problem code lines for me.

mkgrgis requested a review from lamdn1409

December 25, 2024 07:09

Contributor

lamdn1409 commented Dec 25, 2024

@lamdn1409, could you please help me to do a resume about code problem? Inside of sqlite_deparse{IUD}Sql there is sqlite_deparseReturningList, inside of sqlite_deparseReturningList there is sqlite_deparseTargetList with (buf, root, rtindex, rel, true, ... parameters. This true is is_returning, but inside of sqlite_deparseTargetList https://github.com/mkgrgis/sqlite_fdw/blob/d339f8ce9fea91fe647ead4d79196472723968b1/deparse.c#L2139 is not effective. This code doesn't work as in postgres_fdw. Now I cannot understand why and have no versions about the problem.

@mkgrgis INSERT/DELETE/UPDATE ... RETURNING will be pushed down, so Direct Modification mechanism is activated , please check the routine PlanDirectModify, RETURNING clause should be deparsed at this step.

Contributor Author

mkgrgis commented Dec 25, 2024

please check the routine PlanDirectModify,

Thanks, @lamdn1409 ! I am seeing some unchanged code, not unified with postgres_fdw. Will be unified in some hours.


          Add sqlite_store_returning_result

fc53e36

Contributor Author

mkgrgis commented Dec 25, 2024

@lamdn1409 , I need a little help near https://github.com/mkgrgis/sqlite_fdw/blob/fc53e3639486d010f8988a371d2e70de5e4f1f8a/sqlite_fdw.c#L5105 I am confused how to integrate make_tuple_from_result_row in this place. Could you give some variant?


          Fix spell

67390c9

Contributor

lamdn1409 commented Dec 26, 2024

@lamdn1409 , I need a little help near https://github.com/mkgrgis/sqlite_fdw/blob/fc53e3639486d010f8988a371d2e70de5e4f1f8a/sqlite_fdw.c#L5105 I am confused how to integrate make_tuple_from_result_row in this place. Could you give some variant?

@mkgrgis I think result rows should be fetched in sqlite_execute_dml_stmt().

Contributor Author

mkgrgis commented Dec 26, 2024

@mkgrgis I think result rows should be fetched in sqlite_execute_dml_stmt().

@lamdn1409 , I don't understand this place of data fetching. Now we have inside of sqlite_execute_dml_stmt()

if (rc != SQLITE_DONE)
	{
		sqlitefdw_report_error(ERROR, dmstate->stmt, dmstate->conn, NULL, rc);
	}

Hence, in case or RETURNING this should be error (rc==SQLITE_ROW), but no error is observed. Could you please explain me why?

Contributor Author

mkgrgis commented Dec 26, 2024

@lamdn1409 , I have found get_returning_data in postgres_fdw and trying to adopt this to SQLite C functions.

Contributor

lamdn1409 commented Dec 26, 2024

Hence, in case or RETURNING this should be error (rc==SQLITE_ROW), but no error is observed. Could you please explain me why?
@mkgrgis You need to handle the case SQLITE_ROW to fetch data rows, then make tuple slot. Please refer to the similar implementation of IterateForeignScan.

Contributor Author

mkgrgis commented Dec 26, 2024

You need to handle the case SQLITE_ROW to fetch data rows, then make tuple slot. Please refer to the similar implementation of IterateForeignScan.

Ok. I think this is main stream of our implementation.

get_returning_data in postgres_fdw and trying to adopt this to SQLite C functions.

Unfortunately I have unsuccessfully result now.

Contributor Author

mkgrgis commented Dec 26, 2024

@mkgrgis You need to handle the case SQLITE_ROW to fetch data rows, then make tuple slot.

@lamdn1409 , why there is no SQLITE_ROW error in current implementation? SQLITE_ROW != SQLITE_DONE, hence this is sqlitefdw_report_error in qlite_execute_dml_stmt() isn't it?

Contributor

lamdn1409 commented Dec 26, 2024

why there is no SQLITE_ROW error in current implementation?
@mkgrgis SQLITE_ROW is not an error code. It indicates that data row is fetched, please check the specification of sqlite_step().

Contributor Author

mkgrgis commented Dec 26, 2024

@mkgrgis SQLITE_ROW is not an error code. It indicates that data row is fetched, please check the specification of sqlite_step().

Yes, but discussed code fragment #107 (comment) treated all codes rc != SQLITE_DONE as error. Hence we should have sqlitefdw_report_error(... now. Isn't it?

Contributor

lamdn1409 commented Dec 27, 2024

Yes, but discussed code fragment #107 (comment) treated all codes rc != SQLITE_DONE as error. Hence we should have sqlitefdw_report_error(... now. Isn't it?

@mkgrgis I debugged your code, the error is reported as follows.

postgres=# UPDATE "type_STRING" SET col = '_' || substr(col, 2) RETURNING *;
ERROR:  Failed to execute remote SQL
HINT:  SQLite error 'another row available', SQLite result code 100
CONTEXT:  SQL query: UPDATE main."type_STRING" SET `col` = ('_' || substr(`col`, 2)) RETURNING `col`

To debug, you need to build PostgreSQL 17.0 in debug mode. Use the following options.

./configure --prefix=<path_to_postgresql_build_folder> --enable-cassert --enable-debug CFLAGS="-ggdb -O0 -g3 -fno-omit-frame-pointer"

For your reference, https://wiki.postgresql.org/wiki/Developer_FAQ#What_debugging_features_are_available.3F

Contributor Author

mkgrgis commented Dec 27, 2024

Thanks, @lamdn1409 ! Now I understand

HINT: SQLite error 'another row available', SQLite result code 100

as result of sqlite_execute_dml_stmt()

./configure --prefix=<path_to_postgresql_build_folder> --enable-cassert --enable-debug CFLAGS="-ggdb -O0 -g3 -fno-omit-frame-pointer"
For your reference, https://wiki.postgresql.org/wiki/Developer_FAQ#What_debugging_features_are_available.

I think this is very good information for "Contributing" in README.

After #107 (comment)

@mkgrgis INSERT/DELETE/UPDATE ... RETURNING will be pushed down, so Direct Modification mechanism is activated , please check the routine PlanDirectModify, RETURNING clause should be deparsed at this step.

I am still confused about place and possible content of missing RETURNING implementation. Does I adequate understand there is only postgres_fdw example, but nothing in

mkgrgis added 8 commits

December 27, 2024 09:30


          Borrow some code from postgres_fdw

2071fc1


          Fix PG <15

14ce43b


          Borrow postgres_fdw code

0b6eec5


          2nd borrowing postgres_fdw code


          Restore Attinmeta (by original rel, not RETURNING)

b697b8b


          Fix get_returning_data (no expected result)

05f387d


          Add TCs UPD/DEL USING, fix PG14-

7b42e17


          Style and comment fixes

e4592d2

Contributor Author

mkgrgis commented Dec 30, 2024

@lamdn1409 , I have carefully read postgres_fdw and restored 5-6 fragments of RETURNING implementation code, but this is still not enough. Now main problem is TupleDesc for a table, not for RetrivedAttr list of RETURNING attributes, see TC 74. Could you please diagnose where is missing code?
Other small problem is some Pg15- difference, see current failing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet