search

pgspider / sqlite_fdw

SQLite Foreign Data Wrapper for PostgreSQL
Other
219 stars 37 forks source link

Add uuid support #82

Closed mkgrgis closed 1 year ago

mkgrgis commented 1 year ago

This PR include:

mkgrgis commented 1 year ago

@nxhai98 , thanks for review! My first fixing round is ended. Please review again.

nxhai98 commented 1 year ago

@mkgrgis Thank for your fixing.
I checked it again and have some feedback. Please help me to recheck.
And please help me resolve comment if it OK.

nxhai98 commented 1 year ago

@mkgrgis I ran test in my env and this crash:

test 15.0/extra/sqlite_fdw_post   ... FAILED (test process exited with exit code 2)     3543 ms
test 15.0/extra/float4            ... FAILED     1485 ms
test 15.0/extra/float8            ... FAILED (test process exited with exit code 2)     2559 ms
test 15.0/extra/int4              ... FAILED (test process exited with exit code 2)     1405 ms
test 15.0/extra/int8              ... FAILED (test process exited with exit code 2)       22 ms
test 15.0/extra/numeric           ... FAILED (test process exited with exit code 2)     4956 ms
test 15.0/extra/join              ... FAILED (test process exited with exit code 2)     6555 ms
test 15.0/extra/limit             ... FAILED (test process exited with exit code 2)     5758 ms
test 15.0/extra/aggregates        ... FAILED (test process exited with exit code 2)     2755 ms
test 15.0/extra/prepare           ... FAILED     1256 ms
test 15.0/extra/select_having     ... ok          630 ms
test 15.0/extra/select            ... FAILED      187 ms
test 15.0/extra/insert            ... ok          286 ms
test 15.0/extra/update            ... FAILED (test process exited with exit code 2)      881 ms
test 15.0/extra/timestamp         ... FAILED      230 ms
test 15.0/sqlite_fdw              ... FAILED (test process exited with exit code 2)     3781 ms
test 15.0/type                    ... FAILED (test process exited with exit code 2)     2621 ms
test 15.0/aggregate               ... FAILED      516 ms
test 15.0/selectfunc              ... FAILED     1351 ms

Is the test passed in your env?
I reverted to commit: 90dad0a and all tests passed.

mkgrgis commented 1 year ago

@mkgrgis I ran test in my env and this crash: Is the test passed in your env? I reverted to commit: 90dad0a and all tests passed.

Ah... This can cause if in you system there is no locales_all or similar package. Unicode tests always use locales metadata. In this case we are creating PostgreSQL databases with different encodings.

nxhai98 commented 1 year ago

@mkgrgis I checked crashed case and root cause may not as you said.

Call stack:

sqlite_fdw.so!sqlite_execute_dml_stmt(ForeignScanState * node) (\home\tsdv\workplace\postgresql-15.0\contrib\sqlite_fdw\sqlite_fdw.c:5401)
sqlite_fdw.so!sqliteIterateDirectModify(ForeignScanState * node) (\home\tsdv\workplace\postgresql-15.0\contrib\sqlite_fdw\sqlite_fdw.c:2656)
ForeignNext(ForeignScanState * node) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\nodeForeignscan.c:59)
ExecScanFetch(ExecScanRecheckMtd recheckMtd, ExecScanAccessMtd accessMtd, ScanState * node) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\execScan.c:133)
ExecScan(ScanState * node, ExecScanAccessMtd accessMtd, ExecScanRecheckMtd recheckMtd) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\execScan.c:199)
ExecForeignScan(PlanState * pstate) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\nodeForeignscan.c:132)
ExecProcNodeFirst(PlanState * node) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\execProcnode.c:464)
ExecProcNode(PlanState * node) (\home\tsdv\workplace\postgresql-15.0\src\include\executor\executor.h:259)
ExecModifyTable(PlanState * pstate) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\nodeModifyTable.c:3528)
ExecProcNodeFirst(PlanState * node) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\execProcnode.c:464)
ExecProcNode(PlanState * node) (\home\tsdv\workplace\postgresql-15.0\src\include\executor\executor.h:259)
ExecutePlan(EState * estate, PlanState * planstate, _Bool use_parallel_mode, CmdType operation, _Bool sendTuples, unsigned long numberTuples, ScanDirection direction, DestReceiver * dest, _Bool execute_once) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\execMain.c:1636)
standard_ExecutorRun(QueryDesc * queryDesc, ScanDirection direction, uint64 count, _Bool execute_once) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\execMain.c:363)
ExecutorRun(QueryDesc * queryDesc, ScanDirection direction, uint64 count, _Bool execute_once) (\home\tsdv\workplace\postgresql-15.0\src\backend\executor\execMain.c:307)
ProcessQuery(PlannedStmt * plan, const char * sourceText, struct ParamListInfoData * params, QueryEnvironment * queryEnv, DestReceiver * dest, QueryCompletion * qc) (\home\tsdv\workplace\postgresql-15.0\src\backend\tcop\pquery.c:160)
PortalRunMulti(struct PortalData * portal, _Bool isTopLevel, _Bool setHoldSnapshot, DestReceiver * dest, DestReceiver * altdest, QueryCompletion * qc) (\home\tsdv\workplace\postgresql-15.0\src\backend\tcop\pquery.c:1277)
PortalRun(Portal portal, long count, _Bool isTopLevel, _Bool run_once, DestReceiver * dest, DestReceiver * altdest, QueryCompletion * qc) (\home\tsdv\workplace\postgresql-15.0\src\backend\tcop\pquery.c:791)
exec_simple_query(const char * query_string) (\home\tsdv\workplace\postgresql-15.0\src\backend\tcop\postgres.c:1250)
PostgresMain(const char * dbname, const char * username) (\home\tsdv\workplace\postgresql-15.0\src\backend\tcop\postgres.c:4581)
BackendRun(Port * port) (\home\tsdv\workplace\postgresql-15.0\src\backend\postmaster\postmaster.c:4504)

Root cause: I run test on Debug mode, so new porter initialized by NULL => dmstate->resultRel is NULL an crash happen.

//sqlite_fdw.c:5401
Oid         foreignTableId = RelationGetRelid(dmstate->resultRel);

Should it be dmstate->rel ?

I fixed like that and crash does not happen. However, the test still failed in selectfunc.sql:

 SELECT sum(value3),mod(sum(value2), 2) FROM s3;
- sum  | mod 
-------+-----
- -7.2 |   0
-(1 row)
-
+ERROR:  SQLite data affinity "real" disallowed for PostgreSQL data type "bigint"
+HINT:  expected SQLite affinity "integer", incorrect value = '0.0'

Could you please help me investigate this. I am using debug mode also.

mkgrgis commented 1 year ago

@mkgrgis I checked crashed case and root cause may not as you said.

Call stack: Root cause: I run test on Debug mode, so new porter initialized by NULL => dmstate->resultRel is NULL an crash happen.

I have no crashed tests.

//sqlite_fdw.c:5401
Oid           foreignTableId = RelationGetRelid(dmstate->resultRel);

Should it be dmstate->rel ?

You may be right. I have fixed according your note. Let's observe

https://github.com/pgspider/sqlite_fdw/blob/90dad0a3ae7b8ffd129149088e094276c439ed61/sqlite_fdw.h#L266

https://github.com/pgspider/sqlite_fdw/blob/90dad0a3ae7b8ffd129149088e094276c439ed61/sqlite_fdw.h#L287

What is the difference? Is foreign table not relation for result tuples? Is this difference about RETURNING? When dmstate->rel is not equal to dmstate->resultRel? I have no answer.

I fixed like that and crash does not happen. However, the test still failed in selectfunc.sql:

 SELECT sum(value3),mod(sum(value2), 2) FROM s3;
- sum  | mod 
-------+-----
- -7.2 |   0
-(1 row)
-
+ERROR:  SQLite data affinity "real" disallowed for PostgreSQL data type "bigint"
+HINT:  expected SQLite affinity "integer", incorrect value = '0.0'

Oh... This look like wrong data in INT4_TBL. For SQLite this not problem. I'll change this to 72.

Could you please help me investigate this. I am using debug mode also.

Yes. Please give me feedback.

mkgrgis commented 1 year ago

I fixed like that and crash does not happen. However, the test still failed in selectfunc.sql:

 SELECT sum(value3),mod(sum(value2), 2) FROM s3;
- sum  | mod 
-------+-----
- -7.2 |   0
-(1 row)
-
+ERROR:  SQLite data affinity "real" disallowed for PostgreSQL data type "bigint"
+HINT:  expected SQLite affinity "integer", incorrect value = '0.0'

value2 is integer and value3 is float in SQLite. Some calculation result look like integer. Maybe something about sum?

nxhai98 commented 1 year ago

What is the difference? Is foreign table not relation for result tuples? Is this difference about RETURNING? When dmstate->rel is not equal to dmstate->resultRel? I have no answer.

resultRel using for RETURNING clause with JOIN related field: https://github.com/pgspider/sqlite_fdw/blob/90dad0a3ae7b8ffd129149088e094276c439ed61/sqlite_fdw.c#L2577

However, sqlite_fdw does not support returning yet, so resultRel is never set.

value2 and value3 is float in SQLite. Some calculation result look like integer. Why in FOREIGN TABLE value2 is int? My PR helps to catch error in tests.

mod() function return float in sqlite but mod() return bigint in PostgreSQL => this making type affinity error.

sqlite> select mod (2, 3);
2.0

It seems a new issue when push-down mod() function.

@t-kataym -san, could you please share you opinion? Should we update this test result or let it ERROR?

mkgrgis commented 1 year ago

For better diagnostics in case like our mod() type mismatch I have expanded hint in error message, @nxhai98. Please also review https://github.com/pgspider/sqlite_fdw/pull/82/commits/58ec1b68603ab05403960d417b05abc9f5a502ee.

mkgrgis commented 1 year ago

@nxhai98 , now we have only 2 problems here, both with SQLite float<-> PostgreSQL int: LATERAL JOIN https://github.com/pgspider/sqlite_fdw/pull/82#discussion_r1312701069 and mod() https://github.com/pgspider/sqlite_fdw/pull/82#issuecomment-1700770239 So, we are waiting for decision of @t-kataym -san. Could you please verify for code style my last 7 commits with some test and diagnostics refactoring?

nxhai98 commented 1 year ago

Could you please verify for code style my last 7 commits with some test and diagnostics refactoring?

@mkgrgis, I checked these commits and have no comment.

mkgrgis commented 1 year ago

@mkgrgis, I checked these commits and have no comment.

Thanks @nxhai98 ! My next PR is https://github.com/pgspider/sqlite_fdw/pull/83, you can also review it if you have such task from @t-kataym .

I am waiting on @t-kataym decision about detected data type mismatches in existed tests and this PR at all.

nxhai98 commented 1 year ago

@mkgrgis, For mod() function push-down test, please using current actual value as expected and note this issue some where (such as readme).

mkgrgis commented 1 year ago

@nxhai98 , for mod() I left old style code with TODO comment. In my PR about real data types I'll fix this problem in proper way. I am restoring in https://github.com/pgspider/sqlite_fdw/pull/82/commits/cd0e4f070db53241adf7a14542130cfbeddefbbd previous ugly data transformation for int32 and int64 only. Please verify.

mkgrgis commented 1 year ago

@t-kataym , if there will no comments to this PR, please merge this PR as squashed because of many step-by-step commits during discussion.

nxhai98 commented 1 year ago

@mkgrgis Thank for your help in https://github.com/pgspider/sqlite_fdw/pull/82#issuecomment-1708098271. I confirmed the fix.

mkgrgis commented 1 year ago

Thanks, @nxhai98, all problems are resolved. I am waiting on this PR will be merged.

nxhai98 commented 1 year ago

@mkgrgis,

I'm running test for older version and notice that your modification leads some build error and failed test on PG 11, 12, 13 and 14.

Could you please help me fix these issues?

The test failed because your PR affected to current test result similar with expected/15.0/extra/sqlite_fdw_post.out

mkgrgis commented 1 year ago

Ok, @nxhai98! I'll make multi-versional test and fix all detected problems. Also extra/encodings test will be multi-versional. Before 2023-09-08 13:00 東京 all fixes will be pushed.

mkgrgis commented 1 year ago

@nxhai98 , in my environment there are no multi-versional testing problems. Please verify.

nxhai98 commented 1 year ago

I have no more comment.

mkgrgis commented 1 year ago

@t-kataym , please review and merge squashed commit of this PR if you also have no comments.

t-kataym commented 1 year ago

@mkgrgis , Thank you for your development.
Please let me comment one point.
Could you change the function name of sqlite3xxx which you implemented? The prefix "sqlite3" should be used by only SQLite's API, I think. Even if it is a static function in FDW, FDW is better not to use it because it might cause a confusion.

mkgrgis commented 1 year ago

Could you change the function name of sqlite3_xxx which you implemented?

Yes, I think you are about borrowing code? No problems.

BTW In future PR I can write pushdowning for gen_random_uuid() based on this code.

mkgrgis commented 1 year ago

Done, @t-kataym ! Please verify if all is OK. Your note about naming style was very instructive, thanks!

mkgrgis commented 1 year ago

In my environment linter gives me no usefully messages now, the PR branch is ready to squash and merge.

mkgrgis commented 1 year ago

@t-kataym , PostgreSQL 16 is available now in PGDG repository. Will this PR included in sqlite_fdw release for PG16? I want to get after merge some stable git point for other PRs around data types. This PR gives me a base for mixed affinity transformations and will be reference implementation for other cases: date, bool etc.

t-kataym commented 1 year ago

@mkgrgis , Thank you for fixing. I confirmed your fix.

We are in progress to support postgres16 and would like to release sqlite_fdw supporting it as soon as possible. But if we accept this PR now, it might affect to the release schedule. Could you please let me accept this PR after new sqlite_fdw was released? It means that you need rebase.

mkgrgis commented 1 year ago

Could you please let me accept this PR after new sqlite_fdw was released? It means that you need rebase

I have understand release schedule, @t-kataym, thanks. This PR touches some function signatures in sqlite_fdw.c and look like adaptation to PostgreSQL 16 will be hard if in release for PostgreSQL 16 will be old function signatures.

But if we accept this PR now, it might affect to the release schedule.

This is understandable, but could you please confirm my PR have some PostgreSQL 16 - related problems? If there are no such problems release schedule will not affected. Was there any build experiments with my PR in combination with unknown for me git mainstream changes ? I want to know was the problems of my PR with unknown new release-candidate of sqlite_fdw and PostgreSQL 16 little, moderate, big, very big or there was no specific problems?

t-kataym commented 1 year ago

Your PR is no problem. We almost complete the work for releasing new sqlite_fdw supporting 16. I would appreciate your understanding.

mkgrgis commented 1 year ago

Ok, @t-kataym. No problem if rebase of this PR will be mainly git not any hard code changes. Thanks for clarification.

t-kataym commented 1 year ago

@mkgrgis , Thank you for your understanding.
We have just released new sqlite_fdw.

mkgrgis commented 1 year ago

@t-kataym , thanks to you and @jopoly for the new release! Resolving conflicts was not hard for me, only README.md, 10 lines in sqlite_query.c and 15 lines in sqlite_fdw.c. Now in my environment i can not detect any testing problems on PostgeSQL 16.0, 15.3, 14.8 and 13.1. Please verify and merge squashed PR if all is ok.

mkgrgis commented 1 year ago

12.15 tests also fixed and now there are no problems in my testing results.

mkgrgis commented 1 year ago

@t-kataym, any plans to merge this PR? Could you please write approximately date of FDW team to return to this PR?

nxhai98 commented 1 year ago

Hello @mkgrgis,

I tested your branch after rebase/merge, and have failed test in PG13.11:

--- /postgresql-13.11/contrib/sqlite_fdw/expected/13.11/extra/aggregates.out
+++/postgresql-13.11/contrib/sqlite_fdw/results/13.11/extra/aggregates.out 
@@ -3453,11 +3453,11 @@
   end loop;
 end;
 $d$;
-NOTICE:  drop cascades to table t1c
 NOTICE:  drop cascades to 3 other objects
 DETAIL:  drop cascades to table minmaxtest1
 drop cascades to table minmaxtest2
 drop cascades to table minmaxtest3
+NOTICE:  drop cascades to table t1c
 --Testcase 586:
 DROP SERVER sqlite_svr CASCADE;
 --Testcase 587:

Is this test OK with your env?

mkgrgis commented 1 year ago

Hello, @nxhai98 ! Thanks for testing. This difference can be reproduced in some of my environments, but in some not. Look like this depends on SQLite minor version. Delete object order during DELETE ... CASCADE is not essential, hence I prefer as correct order in pgspider tests, that is equal to order in some of my testing environments. In PostgreSQL 11 tests also there was this problem before this version have deleted from our tests.

t-kataym commented 1 year ago

@mkgrgis We confirmed the PR. Is it no problem to merge it?

mkgrgis commented 1 year ago

@t-kataym , no problem. This will a stable mainstream base for other my PRs about data types. Could you please squash this 51 commit to one during merge process?

t-kataym commented 1 year ago

@mkgrgis Thank you! I merged the PR.

mkgrgis commented 1 year ago

ありがとう to pgspider team! This is the my first big contribution. Thank you for teaching me in C language and testing @t-kataym and @nxhai98 !