Add `updatable` option on different levels

[mkgrgis]

Final implementation like standard postgres_fdw updatable option. Tested with success. Excluded using of sqlite3_open_v2 SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE .

[mkgrgis]

@t-kataym, are any checks/tests in expected or sql directory needed here? Also before complex options for FOREIGN SERVER and FOREIGN TABLE and FOREIGN TABLE COLUMN for parsing special values to real and boolean I want to practice with simple options. What about my future 2 new simple PRs like this PR around opening connection with new options? First fullmutex option SQLITE_OPEN_NOMUTEX or SQLITE_OPEN_FULLMUTEX and second sharedcache option SQLITE_OPEN_SHAREDCACHE or SQLITE_OPEN_PRIVATECACHE ? What about checks/tests for this new options? Do we need any tests in this case?

[t-kataym]

Thank you for the PR.

are any checks/tests in expected or sql directory needed here?

Yes. Could you create tests for new feature in sql and expected directory? The mechanism of test FW is a little bit different from that of original. SQLite FDW supports testing for multiple versions of PostgreSQL. I will describe an explanation of testing in README.md.

Also before complex options for FOREIGN SERVER and FOREIGN TABLE and FOREIGN TABLE COLUMN for parsing special values to real and boolean I want to practice with simple options.

I understood it.

What about my future 2 new simple PRs like this PR around opening connection with new options?

About "readonly" option: I agree to introduce this option but have 2 requests.

1. Could you rename the option name to "updatable" like postgres_fdw.
2. Could you refer the implementation of postgres_fdw? Your implementation controls it by SQLITE_OPEN_READONLY flag when connecting to SQLite database. It can support option for only Foreign Server but not Foreign Table. Please refer postgresIsForeignRelUpdatable in postgres_fdw.c.

About new options of mutex: fullmutex option: SQLite FDW does not connect to SQLite from multi-threads using the same connection. Therefore, I think this option is unnecessary.

sharedcache option: SQLite document says "Shared-cache mode is an obsolete feature". Could you tell me why this option is required?

[mkgrgis]

Could you create tests for new feature in sql and expected directory?

Yes, I want to try after understanding exact testing comparison mechanism.

The mechanism of test FW is a little bit different from that of original. SQLite FDW supports testing for multiple versions of PostgreSQL. I will describe an explanation of testing in README.md.

I am waiting on changes in https://github.com/pgspider/sqlite_fdw/pull/60 with explanation of testing. I have seen there is tests for different PostgreSQL version, but haven't understood main comparison mechanism and exact environment for testing. Now I am having only some clusters with different PostgteSQL versions and one SQLite version.

1. Could you rename the option name to "updatable" like `postgres_fdw`.

Yes. I have changed all names with logical inversion during usage. Updatable will true by default, hence SQLITE_OPEN_READWRITE mode will default.

2. Could you refer the implementation of postgres_fdw?

It will be not easy to find this code source . I am studying what there is.

   Your implementation controls it by SQLITE_OPEN_READONLY flag when connecting to SQLite database. It can support option for only Foreign Server but not Foreign Table. Please refer `postgresIsForeignRelUpdatable` in postgres_fdw.c.

Thanks, @t-kataym ! This reference is very interesting. I think something like sqliteIsForeignRelUpdatable will clear method both to get standard PostgreSQL errors and support FOREIGN TABLE-level option updatable. I will add a function to this branch. Also this new function will be usefully as example for system of options with different levels for special real and boolean values.

About new options of mutex: fullmutex option: SQLite FDW does not connect to SQLite from multi-threads using the same connection. Therefore, I think this option is unnecessary.

I confirm. I read the documentation more carefully after my first propositions.

sharedcache option: SQLite document says "Shared-cache mode is an obsolete feature". Could you tell me why this option is required?

On the time I proposed this option I haven't researched all needed documentation about shared cache. Now I read full article and I think no need sharedcache option. Thanks for documentation researching, @t-kataym !

[mkgrgis]

@t-kataym , a new function was added, new option for foreign tables added and tested. Now I have understanding how to write options for boolean and real special values.

[mkgrgis]

@t-kataym , any chance to read about testing environment? I have try to begin description in https://github.com/mkgrgis/sqlite_fdw/commit/3060507ceb19a6fa663cb4501ad711c390f3253c.

[t-kataym]

@mkgrgis , We have described about testing and contributing. Could you refer the commit https://github.com/pgspider/sqlite_fdw/commit/75183518ffc1b6d7be942a6b99136bb36e0fc9d8 ?

Basically, tests is based on "make check". The description in README.md mentions mainly the difference from the original of PostgreSQL.

About testing environment, we don't profess a specific environment. You can use POSIX-compliant system.

[mkgrgis]

Thank to You, @t-kataym and to @bichht0608 ! Now I am reading about tests and preparing a test cases for readonly mode and for special "real" and "boolean" literals.

[mkgrgis]

We have described about testing and contributing. Could you refer the commit 7518351 ?

Yes. I can refer, @t-kataym , thanks. I also added a notes about testing based on your answer to README.md in https://github.com/pgspider/sqlite_fdw/pull/60/commits/cca5b5e9f57b0dc4925e72afcc23d2db39069c5f.

[mkgrgis]

@t-kataym , for future boolean option in init_core.sql I find bool_test table, for future "real" option I find FLOAT4_TBL and FLOAT8_TBL. Could you recommend me a table for checks for updatable? Must I create a new table?

My check plan: In SQLite there is 1 row

• Insert 1 row -> success
• Add server option to false
• Insert 1 row -> error
• Set server option to true
• Insert 1 row -> success
• Drop server option
• Insert 1 row -> success
• Add table option to false
• Insert 1 row -> error
• Set table option to true
• Insert 1 row -> success
• Drop table option
• Insert 1 row -> success
• Check table content.

Is this a good check plan?

[bichht0608]

(https://github.com/pgspider/sqlite_fdw/blob/master/sql/init_data/init_core.sql) I find bool_test table, for future "real" option I find FLOAT4_TBL and FLOAT8_TBL. Could you recommend me a table for checks for updatable? Must I create a new table?

My check plan: In SQLite there is 1 row

• Insert 1 row -> success
• Add server option to false
• Insert 1 row -> error
• Set server option to true
• Insert 1 row -> success
• Drop server option
• Insert 1 row -> success
• Add table option to false
• Insert 1 row -> error
• Set table option to true
• Insert 1 row -> success
• Drop table option
• Insert 1 row -> success
• Check table content.

Is this a good check plan?

I have some comments for your check plan with 'updatable' as below:

1. You should create new table to test because this testing will modify test data. So if you use existed table test, it can be affect to old test.
2. For checking plan, the current plan have only:
   • Create test for insert, how about delete and update?
   • Create test with option server is true and option table is true/false. How about other combinations: option server is false and option table is true; option server is false and option table is false?

How do you think?

[mkgrgis]

Thanks, @bichht0608 ! Your ideas look like very simple and usefully. I'll try to implement my testing plan with your correctives and additions.

[mkgrgis]

@bichht0608 , please review my draft https://github.com/pgspider/sqlite_fdw/pull/59/commits/9e35f658851d2824ca84a4290a59d8b93fc107d6 (SQL only, no out file). Have I mistaken anywhere?

[mkgrgis]

bool updatable have been removed from connection.c. First version of SQL tests have been added in https://github.com/pgspider/sqlite_fdw/pull/59/commits/390b4419dbfa82ab2a2557a3d7036f1ceca2243c.

[bichht0608]

@mkgrgis thanks for the update sql file, it is fine. I confirmed it.

[mkgrgis]

Thanks, @bichht0608 ! How can I help after your testing review? Who will generate .out file for psql and extend this test to previous PostgreSQL versions? In my tests there are no hard out result specifics, only executed or declined SQL commands.

[bichht0608]

@mkgrgis -san Thanks for the making sql file and ask me for next steps.

I think you are person in charge for making .out file. You can make this file by run sql file. There are 2 options happen:

• If the actual result is correct (it means the actual result is matched with your specification) --> You can use this actual as expected output.
• Otherwise, if there are any incorrect result, it maybe a bug and you need to fix it. After complete to create expectation file, please send us request to review this expectation.

For update the test for remained version of postgres, I think it is unnecessary, you can update for only version 15 and we will maintain this test for 15 and higher version in future.

How do you think?

[mkgrgis]

How do you think?

I think all Your explanations is ok, @bichht0608! Please read output file from my last commit and try to execute tests in automated environment. Unfortunately I have problems with full-automated test environment, but successfully can get expected result from PosgreSQL 15.2 psql console and add it in the file. My command for partial test integrated here in my commits was cat /tmp/RO_RW_part_pg15.0.sql | LANGUAGE=C psql --echo-all -d contrib_regression;.

Maybe I have mistake between "_core" and zero-siffixed SQLite test databases with RO/RW test table. Please let me know if there is error.

[bichht0608]

@mkgrgis , I checked your test (sql, expectation and init data). I have some comments as below:

1. Can not build source code, it maybe cause by lacking remove all "bool updatable" variable, I add this varibale in connection.c as a workaround--> I can build source code sqlite_fdw.
2. Maybe you has wrong in init data, you should move init data for this test to init.sql file instead of init_core.sql
3. After fix (1) and (2), I can run test by make check and see some results/expected are wrong:

• The new implementation changed result of old test in sqlite_fdw_post. sql. You need to investigate this change

  --- /home/vagrant/workplace/postgresql-15.2/contrib/sqlite_fdw/expected/15.2/extra/sqlite_fdw_post.out  2023-04-06 07:48:34.545428316 +0000
  +++ /home/vagrant/workplace/postgresql-15.2/contrib/sqlite_fdw/results/15.2/extra/sqlite_fdw_post.out   2023-04-06 07:58:03.788428316 +0000
  @@ -151,7 +151,7 @@
  ALTER SERVER sqlite_svr OPTIONS (SET database 'no such database');
  --Testcase 7:
  SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1;  -- should fail
  -ERROR:  SQL error during prepare: no such table: main.T 1 SELECT `C 1`, `c3`, `c4` FROM main."T 1" ORDER BY `c3` ASC NULLS LAST, `C 1` ASC NULLS LAST LIMIT 1
  +ERROR:  failed to open SQLite DB. rc=14 path=no such database

• You get expectation by psq, so expectation is not match with current format when run by make check as example below:

  -- SERVER true TABLE true
  --Testcase 256:
  ALTER SERVER sqlite_svr OPTIONS (ADD updatable 'true');
  -ALTER SERVER
  --Testcase 257:
  ALTER FOREIGN TABLE RO_RW_test OPTIONS (SET updatable 'true');
  -ALTER FOREIGN TABLE
  --Testcase 258:
  INSERT INTO RO_RW_test (i, a, b, c) VALUES (8, 'M', 5.02, 8); -- OK
  -INSERT 0 1

• Expectation of TC is wrong:

  
  --Testcase 242:
  INSERT INTO RO_RW_test (i, a, b, c) VALUES (4, 'F', 0.005, 5); -- OK
  -ERROR:  failed to execute remote SQL: rc=19 UNIQUE constraint failed: RO_RW_test.i  

• sql=INSERT INTO main."ro_rw_test"(i, a, b, c) VALUES (?, ?, ?, ?)

• Need to check the reason for difference this result

  
  --Testcase 276:
  SELECT * FROM RO_RW_test;

• i | a | b | c
  -----+---+-------+----

• 4 | F | 0.005 | 5

• 12 | R | 6.18 | 11

• 9 | O | 3.21 | 9

• i | a | b | c +---+---+-------+---

• | A | 1.001 | 0

• 4 | F | 0.005 | 5

• 9 | O | 3.21 | 9 (3 rows)

One more point, you run test and get expectation on Postgres 15.2 and current our sqlite fdw have supported 15.0, not 15.2, So I think you should add test and run with 15.0 version as current support on our GitHub.

Could you please check my comments and feedback me if have any unclear point

[mkgrgis]

Could you please check my comments and feedback me if have any unclear point

Yes. Thanks for this git-branch researching, @bichht0608 !

1. Can not build source code, it maybe cause by lacking remove all "bool updatable" variable, I add this varibale in connection.c as a workaround--> I can build source code sqlite_fdw.

It s about https://github.com/pgspider/sqlite_fdw/pull/59#discussion_r1118410804 and https://github.com/pgspider/sqlite_fdw/pull/59/commits/f916b87e0f3f690ba7ed7097c1618ed76f3bc7e4. I will try to rewrite this code fragment with both Your and @t-kataym recommendations.

Maybe I have mistake between "_core" and zero-siffixed SQLite test databases with RO/RW test table. Please let me know if there is error.

2. Maybe you has wrong in init data, you should move init data for this test to init.sql file instead of init_core.sql

I am sorry, @bichht0608 -san. It was really my SQLite database mismatch. I have moved initial commands for RO/RW test table to init.sql that equals to zero-siffixed SQLite test database.

3. After fix (1) and (2), I can run test by make check and see some results/expected are wrong:

   • The new implementation changed result of old test in sqlite_fdw_post. sql. You need to investigate this change

--- /home/vagrant/workplace/postgresql-15.2/contrib/sqlite_fdw/expected/15.2/extra/sqlite_fdw_post.out  2023-04-06 07:48:34.545428316 +0000
+++ /home/vagrant/workplace/postgresql-15.2/contrib/sqlite_fdw/results/15.2/extra/sqlite_fdw_post.out   2023-04-06 07:58:03.788428316 +0000
@@ -151,7 +151,7 @@
 ALTER SERVER sqlite_svr OPTIONS (SET database 'no such database');
 --Testcase 7:
 SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1;  -- should fail
-ERROR:  SQL error during prepare: no such table: main.T 1 SELECT `C 1`, `c3`, `c4` FROM main."T 1" ORDER BY `c3` ASC NULLS LAST, `C 1` ASC NULLS LAST LIMIT 1
+ERROR:  failed to open SQLite DB. rc=14 path=no such database

I also have this difference. Now I have no ideas what is wrong with main.T 1, in fact with T 1. Can You ensure there is no such problem in master git-branch with fresh testing databases and PostgreSQL 15.0 ? If no, this problem can have source in

• difference between PostgreSQL 15.0 and 15.2, but there were no such my changes;
• my changes in this branch;
• not fresh testing databases.

• You get expectation by psql, so expectation is not match with current format when run by make check as example below:

-- SERVER true TABLE true
 --Testcase 256:
 ALTER SERVER sqlite_svr OPTIONS (ADD updatable 'true');
-ALTER SERVER
 --Testcase 257:
 ALTER FOREIGN TABLE RO_RW_test OPTIONS (SET updatable 'true');
-ALTER FOREIGN TABLE
 --Testcase 258:
 INSERT INTO RO_RW_test (i, a, b, c) VALUES (8, 'M', 5.02, 8); -- OK
-INSERT 0 1

Can You give me a command of the simplest analogue of testing framework like here https://github.com/mkgrgis/sqlite_fdw/tree/readme-fix-and-additions#testing-framework there is cat Some_test_script.sql | LANGUAGE=C psql --echo-all -d contrib_regression > Some_test_script.actual.out; diff Some_test_script.out Some_test_script.actual.out; , @bichht0608 ? This command will be very usefully in case of problems with reproducing full testing framework for beginners in testing such as I.

• Expectation of TC is wrong:

--Testcase 242:
 INSERT INTO RO_RW_test (i, a, b, c) VALUES (4, 'F', 0.005, 5); -- OK
-ERROR:  failed to execute remote SQL: rc=19 UNIQUE constraint failed: RO_RW_test.i  
- sql=INSERT INTO main."ro_rw_test"(`i`, `a`, `b`, `c`) VALUES (?, ?, ?, ?)

It's my catching against not fresh testing databases. Please see below.

• Need to check the reason for difference this result

 --Testcase 276:
 SELECT * FROM RO_RW_test;
- i  | a |   b   | c  
-----+---+-------+----
-  4 | F | 0.005 |  5
- 12 | R |  6.18 | 11
-  9 | O |  3.21 |  9
+ i | a |   b   | c 
+---+---+-------+---
+   | A | 1.001 | 0
+ 4 | F | 0.005 | 5
+ 9 | O |  3.21 | 9
 (3 rows)

Oh, it's simple problem. I have special created my tests only for fresh testing database after creating, see https://github.com/mkgrgis/sqlite_fdw/blob/6800f926dcfa077b02bd304e1f5897589c16ee1e/sql/init_data/init.sql#L62. If you run tests again on this data, you will get this difference. Is this test style very strong? I can move INSERT INTO RO_RW_test (a, b, c) VALUES ('A',1.001, 0); to other place and tests will be immutable (stable not only for fresh test databases). There were no testing documentation, hence I don't know about preferred testing style - immutable or strong.

One more point, you run test and get expectation on Postgres 15.2 and current our sqlite fdw have supported 15.0, not 15.2, So I think you should add test and run with 15.0 version as current support on our GitHub.

I know about this problem. I am studying about version selecting around of apt program and apt.postgresql.org. Unfortunately I am apt beginner too.

[bichht0608]

@mkgrgis -san,

Firstly, I have some result want to notify you:

1. I run your test on both of Postgres 15.0 and Postgres 15.2. The result is same, it means these issues as I raised above also happen with Postgres 15.0
2. I also run sqlite_fdw of pgspider repo with Postgres 15.0. All result is passed as evidence below
3. I also check and see that you created new table with your test and this table have not related to others test of sqlite_fdw file. So using fresh database or existed database is not affect to test result. I think. By all analyze and result above, I think you should check issues by debug your source code and found root cause. It can cause by your source code.

By your feedback, I imaged that you have not known how to run test by make check (it means run all test). So I think, I should to make guideline in details to help you: how to run test. After you can re-procedure issues as I raised on your environment, you can debug and continue discuss later. Following is guideline for run all test of sqlite_fdw with make check:

1. Install Postgres 15.0 and build: You should download source code of Postgres then: There are 2 modes to build Postgres: release and Debug
   • With Release, you can build by command:

cd Postgres_15_folder
rm -rf install
mkdir install
./configure --prefix=$(pwd)/install 
make clean && make && make instal

• With Debug mode, you can build by command:

cd Postgres_15_folder
rm -rf install
mkdir install
./configure --prefix=$(pwd)/install --enable-cassert --enable-debug CFLAGS="-ggdb -g -O0 -fno-omit-frame-pointer" 
make clean && make && make install

In my test, I build with Release mode

2. checkout sqlite_fdw in contrib folder (/postgresql-15.0/contrib/sqlite_fdw) and build

make clean
make
make install

3. Run test by test.sh

   ./test.sh

   After run test, the makecheck will summary result of test, you can check raw data of result is results folder.

If you have any difficult with procedure above, please tell me.

[mkgrgis]

Thanks, @bichht0608 ! You have wrote very important information about testing.

By your feedback, I imaged that you have not known how to run test by make check (it means run all test). So I think, I should to make guideline in details to help you: how to run test.

Yes, this will also interesting for @sinelinea from https://github.com/pgspider/sqlite_fdw/pull/67.

I really use only make installcheck, not make check. Also I don't use PostgreSQL from source code, only PostgreSQL from apt deb server of PGDG. About my unsuccessfully way You can read in log fail.log.txt and in a draft of documentation about testing https://github.com/mkgrgis/sqlite_fdw/tree/readme-fix-and-additions#tests.

By all analyze and result above, I think you should check issues by debug your source code and found root cause. It can cause by your source code.

Yes, it's the main version for me now. I'll come back after studying about PostgreSQL compilation from source code and trying to use sqlite_fdw in source code tree, especially in /postgresql-15.0/contrib/sqlite_fdw. Thanks to Your main milestones, @bichht0608 I hope to have testing success at the end of the next week. Also I'll add information from your message to README.md in a part about testing.

[mkgrgis]

I am having successfully multiversuional testing system for sqlite_fdw now. I'll check tests for this branch some days later.

[mkgrgis]

@bichht0608, I have never deal with C PostgreSQL testing system, but now I have reproduced this system in both multi - versional and one - versional forms. Please verify my conception for one - versional testing in https://github.com/mkgrgis/sqlite_fdw/commit/fd930626b9114d41876b2b3c9ec7a46508037b58 and write here any about my 2 latest commits to this branch after checking. I have made this commits after my first full testing cycle.

Can you help me about getting exactly 3.38.5 version of SQLite ? Now I have installed nearest library from https://pkgs.org/search/?q=libsqlite because on SQLite site there is only https://sqlite.org/releaselog/3_38_5.html and no needed version on https://sqlite.org/download.html . I think https://github.com/sqlite/sqlite/releases/tag/version-3.38.5 is normal source ? Should mkso.sh give me needed .so?

[bichht0608]

@mkgrgis,

Thanks for the update. I have some comments as below for Readme file:

• I feel that the update is so long and very details. It is unnecessary for all the description in Readme file.
• You can descript in general for how many test type (models) and how to run test for each model. For script to setup environment or clone source code or run test, we can make a script file in source code, no need to show all in Readme file.
• For multi version test, it is similar as single version test, so we only need to descript in details for single version test. For multi version, we can refer single version.

[mkgrgis]

Thanks, @bichht0608 ! If you confirm my testing description is technically OK, I'll try to reduce long and very details description. I am thinking about how to better make a script file in source code. I have idea: get_testing_environment.sh and steps as parameter: get_multiversion and get_latest, test_sqlite_fdw with github author and branch or without for generic pgspider version. Is this a good idea?

Can you confirm this branch is OK after testing?

[bichht0608]

@mkgrgis, I checked your script to run test for 1 version. Almost is OK without command below:

rm -r -v "sqlite_fdw"; rm -r -v "postgresql/contrib/sqlite_fdw";

I think your purpose is delete both sqlite_fdw folder and content inside this folder. So you should use "rm -rf" command.

For a script file as your idea, I think it is OK. Please go ahead with your idea.

[mkgrgis]

I checked your script to run test for 1 version. Almost is OK without command below ...

@t-kataym , I have understood this PR is ready to merge isn't it?

@bichht0608 , please give me advice about SQLite part of testing environment. I have asked your help:

Can you help me about getting exactly 3.38.5 version of SQLite ? Now I have installed nearest library from https://pkgs.org/search/?q=libsqlite because on SQLite site there is only description https://sqlite.org/releaselog/3_38_5.html and no needed version on https://sqlite.org/download.html . I think https://github.com/sqlite/sqlite/releases/tag/version-3.38.5 is normal source ? Should mkso.sh give me needed .so?

[bichht0608]

@mkgrgis , thanks for quick response Maybe I need to re-explain you. I said that the script that you mentioned to run test for one version is OK but it not means that the Readme file content is OK. I still have comments for Readme file as https://github.com/pgspider/sqlite_fdw/pull/59#issuecomment-1577840510.

For this point:

Can you help me about getting exactly 3.38.5 version of SQLite ? Now I have installed nearest library from https://pkgs.org/search/?q=libsqlite because on SQLite site there is only description https://sqlite.org/releaselog/3_38_5.html and no needed version on https://sqlite.org/download.html . I think https://github.com/sqlite/sqlite/releases/tag/version-3.38.5 is normal source ? Should mkso.sh give me needed .so?

Following is our command to install SQLite, this is example for version 3.32.3, could you please use it as reference for version 3.38.5.

wget "https://github.com/sqlite/sqlite/archive/version-3.32.3.tar.gz" -P /tmp/
cd /tmp
tar -xzvf version-3.32.3.tar.gz
cd /tmp/sqlite-version-3.32.3
./configure
make
make install
cp sqlite3 /usr/bin/

[mkgrgis]

@mkgrgis , thanks for quick response Maybe I need to re-explain you. I said that the script that you mentioned to run test for one version is OK but it not means that the Readme file content is OK.

Yes, but I was not about README.md content. I was about this PR https://github.com/pgspider/sqlite_fdw/pull/59 based on mkgrgis:draft_updatable_option There is no testing info in this PR, please see https://github.com/mkgrgis/sqlite_fdw/tree/draft_updatable_option#tests.

My draft of README.md about testing https://github.com/mkgrgis/sqlite_fdw/commit/fd930626b9114d41876b2b3c9ec7a46508037b58 is in other PR.

I still have comments for Readme file as #59 (comment).

And PR with README.md is still in progress.

Can you help me about getting exactly 3.38.5 version of SQLite ?

Many thanks @bichht0608 ! I'll add your note to README.md or script in separate PR. Now I have confirmed process for getting exact testing environment!

[t-kataym]

@t-kataym , I have understood this PR is ready to merge isn't it?

As I have reviewed (https://github.com/pgspider/sqlite_fdw/pull/59#discussion_r1109525867) could you revert the change in connection.c?
It is unnecessary to control the flag for sqlite3_open_v2 on sqlite_fdw, and "pragma case_sensitive_like=1" is not related to this PR.

[mkgrgis]

@t-kataym, do you means revert changes of connection.c in this PR at all? I have understood in https://github.com/pgspider/sqlite_fdw/pull/59#discussion_r1109525867 you was only about updatable variable as part of connection structure. This variable is local now.

"pragma case_sensitive_like=1" is not related to this PR.

I only reproduce current code without changes with this SQL PRAGMA. https://github.com/pgspider/sqlite_fdw/blob/54781c0781ae317dafd81173ac5fe807e24a1b85/connection.c#L208 Where have I mistaked?

It is unnecessary to control the flag for sqlite3_open_v2

Why? For security reasons there was idea: server-levelled updatable='false' disallows write operations at all both on PostgreSQL and SQLite levels. Two guaranties against SQLite DB changes combines in safer configuration than only one. I think there is SQLite advantage in native read-only mode. In postgres_fdw or firebird_fdw there is only client levelled read-only mode, but SQLite can this on server level. In postgres_fdw we can disallow changes on server level, but allow on table level. In this PR there is no similar behaviour. Is this wrong?

[t-kataym]

@mkgrgis ,

"pragma case_sensitive_like=1" is not related to this PR.

I only reproduce current code without changes with this SQL PRAGMA.

I'm sorry. It's my mistake. The current implementation does that. Please ignore this comment.

do you means revert changes of connection.c in this PR at all?

Yes, I meant so.

Why? For security reasons there was idea: server-levelled updatable='false' disallows write operations at all both on PostgreSQL and SQLite levels. Two guaranties against SQLite DB changes combines in safer configuration than only one. I think there is SQLite advantage in native read-only mode. In postgres_fdw or firebird_fdw there is only client levelled read-only mode, but SQLite can this on server level. In postgres_fdw we can disallow changes on server level, but allow on table level. In this PR there is no similar behaviour. Is this wrong?

I'm sorry, I don't understand your comment well.
Updatable or not is controlled by sqliteIsForeignRelUpdatable.
So it is unnecessary to control by sqlite connection as sqlite3_open_v2() in connection.c.
I think there is no case that sqlite_fdw executes a modify query using read-only connection because PostgreSQL core raises an error based on sqliteIsForeignRelUpdatable and modification APIs on sqlite_fdw are not called.

[mkgrgis]

@t-kataym

I'm sorry, I don't understand your comment well.

It's because SQLite testing and algorithmic style is stronger than in our PostgreSQL community. SQLite implementation and testing based on some aviation standards. This is from interview with SQLite creator https://corecursive.com/066-sqlite-with-richard-hipp/.

Updatable or not is controlled by sqliteIsForeignRelUpdatable.

Yes. It's my main implementation. This will work for all normal and tested cases.

I think there is no case that sqlite_fdw executes a modify query using read-only connection because PostgreSQL core raises an error based on sqliteIsForeignRelUpdatable and modification APIs on sqlite_fdw are not called.

Yes, there is no normal and tested cases. What about potential or future CVE in PostgreSQL core or in SQLite? I think readonly mode will be popular also for security reasons for some users of our fdw, isn't it?

For example, in SQLite there was CVE-2022-35737 with 22 years age! So, double implementation of database-levelled updatable behaviour through sqlite3_open_v2() look like very useful and don't add some big CPU time or RAM usage, but can help against CVE or testing mistakes now and in future. Isn't it, @t-kataym ?

[t-kataym]

Thank you for your explanation, @mkgrgis .

I would like to hear the opinion of member, @sonpntsdv

[son-phamngoc]

Hello @mkgrgis , thanks for your support. @t-kataym I would like to share my opinion. In my opinion, if there is any CVE, we need to analyze so many things: the root cause, possibility, reproduction, suitable solution ... We cannot be sure that read-only mode can solve or prevent that CVE. Leaving an uncertain code for an uncertain case is not suitable, I think. At least, we need a clear basis to add it.

And I also have a concern about this code. I'm thinking about the following case: Assuming that we have a foreign table with "updatable" is false. - Step 1: User select from that table. Because updatable is false, sqlite connection will be opened as read-only. This connection is cached into hash table. - Step 2: User alters the table, change "updatable" to true. In my understanding, because FOREIGN TABLE is altered, so sqlitefdw_inval_callback is not called. It is called only when SERVER or USER MAPPING is changed. It means entry->invalidated does not change. It is still "false". - Step 3: User insert/update/delete on the table. sqlite_fdw searches the connection in hash table and retrieves an entry. This entry is not null, and invalidated is false, so this connection is valid and can be used. sqlite_fdw uses it without making new connection. Because it is read-only connection, I think it will cause error when executing a modification query. If an uncertain code affects to original behavior, we cannot accept it.

This is just my concern. I haven't debug to verify it yet. If I have any mistake, please tell me.

[mkgrgis]

Thanks @son-phamngoc ! Your message is about central problem of this PR ! 💯

We cannot be sure that read-only mode can solve or prevent that CVE.

Really, not all possible CVE. But I think we can prevent some possible CVEs around of unwanted writing operations. Because two CVE about this both in SQLite and PostgreSQL are less probable than only one anywhere.

Really, as you confirmed, idea of implementation for this PR was to synchronise SQLite file mode and PostgreSQL updatable.

You reproduce a case with ALTER TABLE and show problem for usually ...IsForeignRelUpdatable like in postgres_fdw or firebird_fdw. But in my PR if server-levelled option updatable set to false table-levelled option is ignored. See https://github.com/mkgrgis/sqlite_fdw/blob/ea4b15fa7fb6bf13aa0f8d7b7783ccb2696687b8/sqlite_fdw.c#LL5604C2-L5604C2 Hence in Step 3 from your example there were no error: both SQLite readonly and PostgreSQL sqliteIsForeignRelUpdatable should disallow "DIU" (delete, insert, update). Isn't it?

[son-phamngoc]

But in my PR if server-levelled option updatable set to false table-levelled option is ignored.

In my example, 'updatable' of the server is not set. I only set 'updatable' for foreign table-level. You can imagine like this:

CREATE SERVER sqlite_svr ...;
CREATE FOREIGN TABLE ... OPTIONS (updatable 'false');

At this loop, because 'updatable' option is not set, so updatable is still true.

foreach(lc, server->options)
{
    DefElem    *def = (DefElem *) lfirst(lc);

    if (strcmp(def->defname, "updatable") == 0)
        updatable = defGetBoolean(def);
}

Therefore, at next loop, the options of table-level is not ignored.

if (updatable)
{
    foreach(lc, table->options)
    {
        DefElem    *def = (DefElem *) lfirst(lc);

        if (strcmp(def->defname, "updatable") == 0)
            updatable = defGetBoolean(def);
    }
}

[mkgrgis]

@son-phamngoc , I don't understand your last message.

First

Assuming that we have a foreign table with "updatable" is false.

• Step 1: User select from that table. Because updatable is false, sqlite connection will be opened as read-only.

Is here updatable is false about server-levelled option?

and second one

In my example, 'updatable' of the server is not set. I only set 'updatable' for foreign table-level. You can imagine like this:

CREATE SERVER sqlite_svr ...;
CREATE FOREIGN TABLE ... OPTIONS (updatable 'false');

Is this cases compatible? Is this the same example or different examples?

In my test cases there is no answers from SQLite, hence all access modes supported by sqliteIsForeignRelUpdatablewithout conflicts with SQLITE_OPEN_READONLY. I think you want to give me a problem case with my code but I can't understand my algorithmic error.

@t-kataym , also with SQLITE_OPEN_READONLY I meant this read-only mode gives error if database file not exist. This is also usefully for security reasons to deny creating database if there is no SQLite database file.

[son-phamngoc]

Is here updatable is false about server-levelled option?

No, I mean updatable is false at table-leveled option. It is like this, only set at table-leveled option.

CREATE SERVER sqlite_svr ...; CREATE FOREIGN TABLE ... OPTIONS (updatable 'false');

Is this cases compatible? Is this the same example or different examples?

Yes, this case is possible, when we want to restrict only 1 foreign table from updating. However, it seems my step 1 above is not correct. Sorry about that.

But in my PR if server-levelled option updatable set to false table-levelled option is ignored.

I see that you ignored table-leveled option at sqlite_make_new_connection, so for my step 1, it does not detect that updatable of table-leveled is false. Updatable is still true, and it opens a SQLITE_OPEN_READWRITE connection, even though it is a SELECT query. Therefore, at step 3, there is no error.

[mkgrgis]

Yes, @son-phamngoc, table-levelled updatable implemented only in PostgreSQL style, because in SQLite there is no per-table access rules. In this case my code can not prevent any possible PostgreSQL CVE. Also SQLITE_OPEN_READWRITE isn't a SQLite mode protected in my PR against potential CVE. Hence there is no problems.

Please, your opinion @son-phamngoc , @t-kataym ?

[t-kataym]

@mkgrgis , @son-phamngoc Thank you for the discussion.
Please let me comment about the specification first because it might affect to the complexity of implementation of connection.c.

Now I understood that the specification of this PR is different from that of postgres_fdw.
If updatable option is specified to both foreign server and foreign table, postgres_fdw uses the option value of foreign table and ignores that of foreign server.
With this PR, sqlite_fdw ignores the option value of foreign table if the option of foreign server is not updatable.

The difference is Pattern2 as follows.	Pattern	ForeignServer	ForeignTable	postgres_fdw	This PR
1	false	false	Not updatable	Not updatable
2	false	true	Updatable	Not updatable
3	false	Not specified	Not updatable	Not updatable
4	true	false	Not updatable	Not updatable
5	true	true	Updatable	Updatable
6	true	Not specified	Updatable	Updatable
7	Not specified	false	Not updatable	Not updatable
8	Not specified	true	Updatable	Updatable
9	Not specified	Not specified	Updatable	Updatable

Is my understanding correct?

If so, we cannot accept this specification.
We would like to use the same specification as postgres_fdw. There are 2 reasons.

1. This project is one of PGSpider project and we suppose that user uses multiple FDWs. So we would like to reduce the difference between FDWs as long as possible in order to reduce a trouble risk.
2. The usability of postgres_fdw's specification is higher, I think. Let's consider the case that there are a lot of tables in SQLite and user wants to set updatable for a few tables. With your specification, it is required to specify updatable=false for foreign server and updatable=false for a lot of tables. But, with specification of postgres_fdw, user can configure by specifying updatable=false for a foreign server and updatable=true for a few tables.

If sqlite_fdw follows postgres_fdw's specification, we cannot ignore son-phamngoc's concern.
And I agree with son-phamngoc's comment as below.

Leaving an uncertain code for an uncertain case is not suitable, I think. At least, we need a clear basis to add it.

[mkgrgis]

@t-kataym , thanks! Now I understand full context and usability aspect with higher priority in this project than preventing CVE.

Now I understood that the specification of this PR is different from that of postgres_fdw.

Yes. I have written about this in first comments to this PR.

The difference is Pattern2 as follows. Is my understanding correct?

Yes, completely correct.

There are 2 reasons.

Thanks. I'll correct this PR code and tests for reproducing postgres_fdw logic if server updatable is false but table updatable is true. In this case really no changes in connection.c are needed.

[t-kataym]

Thank you for your reply and understanding.

[mkgrgis]

@t-kataym , C code and tests fixed to standard postgres_fdw behaviour. Please check.

[mkgrgis]

@t-kataym what about option force_readonly in a separate PR based on SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE ? Now i can write well-tested code thanks to You, @bichht0608 and @son-phamngoc .

[mkgrgis]

@t-kataym , as result of our long discussion I have created an new well-tested PR https://github.com/pgspider/sqlite_fdw/pull/78 with adding two options: usual updatable like in postgres_fdw and force_readonly option which controls low-level access to SQLite database file. See description of the new PR. You can ensure that it saves all our results from this PR and can close this PR if all is OK and if you'll think the new PR is better than this.

[t-kataym]

@mkgrgis , Thank you for your modification on this PR. We will confirm it.

About force_readonly option, I will comment on #78.

[t-kataym]

@son-phamngoc Thank you for your confirmation.

@mkgrgis The review has completed. We are ready to accept the PR. Is it no problem to merge the PR?

[mkgrgis]

Thanks, @son-phamngoc !

Thanks, @t-kataym !

I think there is no problems to merge this PR after squashing (merger can set checkbox look like "squash all commits before merge") because here are a lot of little commits for https://github.com/pgspider/sqlite_fdw/network . Also you can review https://github.com/pgspider/sqlite_fdw/pull/78 where all contributions of this PR are given, but also added other option.

Which variant have more reason for you, @t-kataym : merge this squashed PR or review https://github.com/pgspider/sqlite_fdw/pull/78 based on this PR with additional contribution? If https://github.com/pgspider/sqlite_fdw/pull/78 have no problems please write here. If there will any discussion around of https://github.com/pgspider/sqlite_fdw/pull/78 merging of this PR is better.

[t-kataym]

@mkgrgis , Thank you so much for your contribution.

I cannot accept #78 now, will comment on #78.
So please le me merge this PR first.