Initial `RETURNING` implementation #107

mkgrgis · 2024-12-24T04:44:26Z

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

expected/17.0/extra/returning.out

lamdn1409 · 2024-12-24T10:18:48Z

expected/17.0/extra/returning.out

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


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

Reverted to standard behaviour for testing data transfer of different data types. Please check again.

--Testcase 47: SELECT * FROM "type_DATE"; col ------------ 2021-02-23 2021-03-08 9999-12-30 (3 rows) --Testcase 49: INSERT INTO "type_DATE"(col) VALUES ('2021/03/08') RETURNING col; col ------------ 2021-03-08 (1 row) --Testcase 54: SELECT * FROM "type_DATE"; col ------------ 2021-02-23 2021-03-08 9999-12-30 2021-04-23 2021-03-09 9999-12-29 (6 rows)

Do you know why the duplicated value "2021-03-08" was not inserted?

Do you know why the duplicated value "2021-03-08" was not inserted?

This seems doNothing problem, but I did doNothing check across our FDW and haven't found wrong logic.

@lamdn1409 , I think there is some problem inside of sqliteIterateForeignScan, because postgresIterateForeignScan contains other very simple algorithm.
Also sqlite_make_tuple_from_result_row sometimes have values and nulls not after palloc, but always after palloc in postgres_fdw's make_tuple_from_result_row. Also our SQLite FDW doesn't contain fetch_more_data .
Hence I cannot compare SQLite FDW code with Postgres FDW example and possible optimal code structure for fetching RETURNING data. Could you please share your thought about possible sqliteIterateForeignScan changes for RETURNING support?

sql/17.0/extra/returning.sql

lamdn1409 · 2024-12-24T10:40:11Z

sql/17.0/extra/returning.sql

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


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

Will TC 60 ..100 enough?

Remain some points we need to verify, however, please fix all bugs of the current test. I checked the test output, some tests work not correctly.

For example, the test below returns 0 row.

--Testcase 07: UPDATE "type_STRING" SET col = '_' || substr(col, 2) RETURNING *; col ----- (0 rows)

@lamdn1409 , could you please diagnose if here is sqlite_store_returning_result problems? Please refer https://github.com/mkgrgis/sqlite_fdw/blob/69a2e7221ca0f0ed9b297c00835a1352b0c4562d/sqlite_fdw.c#L5420

Resolved after 2385a8e , but there are other problems with RETURNING not for UPDATE. Please verify if we can resolve this discussion.

sql/17.0/extra/returning.sql

lamdn1409 · 2024-12-24T10:50:53Z

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

deparse.c

lamdn1409 · 2024-12-24T10:59:29Z

deparse.c

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


Could you add test case to cover this condition?

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?

Yes, your understanding is correct.

Please verify 45db11c. TCs was added to extra/insert tests, because previously regressed functions belongs only to INSERT support and only restored in this PR.

Please don't modify the existing tests in extra/, these tests were ported from the core test (src/test/regress) of PostgreSQL. We keep them in sync with the core test for maintainability.

I checked some tests you added, there is no RETURNING clause.
Could you add test cases that satisfy the condition "col1 > 20", including RETURNING clause?
Please make sure the test cover added code.

+--Testcase 16: +create view inserttest01_view_wco as +select * from inserttest01 where col1 > 20 +with check option; +--Testcase 17: +create view inserttest01_view as +select * from inserttest01 where col1 > 20; +--Testcase 18: ok +insert into inserttest01_view values(10, 50, 'uuuuu'); +--Testcase 19: no! +insert into inserttest01_view_wco values(10, 50, 'uuuuu');

@lamdn1409 , should similar TCs be placed only at returning test in 2 TCs with and without RETURNING?

Moved to extra/returning and reverted in 3cb5ceb , please confirm @lamdn1409.

lamdn1409 · 2024-12-24T11:01:51Z

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?

mkgrgis · 2024-12-24T19:50:46Z

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.

mkgrgis · 2024-12-25T03:18:40Z

@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 · 2024-12-25T04:57:31Z

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

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

mkgrgis · 2024-12-25T06:08:36Z

resume about code problem?

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

mkgrgis · 2024-12-25T07:08:51Z

@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.

lamdn1409 · 2024-12-25T08:36:23Z

@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.

mkgrgis · 2024-12-25T09:45:57Z

please check the routine PlanDirectModify,

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

mkgrgis · 2024-12-30T03:33:38Z

@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.

sqlite_fdw.c

lamdn1409 · 2025-01-02T04:07:03Z

@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?

I have a comment #107 (comment)

mkgrgis · 2025-01-02T09:24:23Z

@lamdn1409 , now we have only problem for lower versions of PostgreSQL which is in not successfully tests. Some rc = SQLITE_ROW algorithm branch is active instead of common behaviour. Have you got any versions where is the problem?

Here is log around of error

DEBUG:  sqlite_fdw : sqliteBeginDirectModify
DEBUG:  starting remote transaction on connection 0x55f72fe249e8
DEBUG:  sqlite_fdw do_sql_command BEGIN
DEBUG:  sqlite_fdw : sqlite_prepare_wrapper 
SQL ->> UPDATE main."type_STRING" SET `col` = ('_' || substr(`col`, 2)) RETURNING `col`
DEBUG:  sqlite_fdw : sqliteIterateDirectModify
DEBUG:  sqlite_fdw : sqlite_execute_dml_stmt, foreignTableOid = 18496
DEBUG:  sqlite_fdw xact_callback 2
DEBUG:  closing remote transaction on connection 0x55f72fe249e8
DEBUG:  abort transaction
DEBUG:  sqlite_fdw: finalize UPDATE main."type_STRING" SET `col` = ('_' || substr(`col`, 2)) RETURNING `col`
DEBUG:  sqlite_fdw do_sql_command ROLLBACK
ERROR:  Failed to execute remote SQL
HINT:  SQLite error 'another row available', SQLite result code 100

mkgrgis · 2025-01-02T09:39:48Z

@lamdn1409 , please check first review comments again. I think most of them should be resolved.

lamdn1409 · 2025-01-03T02:06:42Z

@lamdn1409 , now we have only problem for lower versions of PostgreSQL which is in not successfully tests. Some rc = SQLITE_ROW algorithm branch is active instead of common behaviour. Have you got any versions where is the problem?

Here is log around of error

@mkgrgis I think there are differences about RETURNING implementation among postgres_fdw's versions. We often use the macro PG_VERSION_NUM to distinguish code of different versions.

lamdn1409 · 2025-01-03T02:08:20Z

@lamdn1409 , please check first review comments again. I think most of them should be resolved.

@mkgrgis OK. I'm checking them now. Then I'm gonna start 2nd review.

mkgrgis · 2025-01-03T06:42:10Z

Thanks for 2nd review, @lamdn1409! Let's summarise current problems:

different behaviour for PostgreSQL 14- and PostgreSQL 15+
void RETURNING results Initial RETURNING implementation #107 (comment)
duplicate INSERT problem Initial RETURNING implementation #107 (comment)

Anything other?

Initial RETURNING implementation

cb754ac

mkgrgis mentioned this pull request Dec 24, 2024

RETURNING support #56

Open