Support PostgreSQL: interface/test-cases/ci-testing #113

SunBeau · 2024-05-07T14:39:01Z

No description provided.

wolkykim · 2024-05-13T03:43:30Z

Sorry for the late reply. I'm reviewing. Give me some time. I will be back on this soon. Thank you.

wolkykim · 2024-05-14T16:18:49Z

src/extensions/qdatabase.c

+    // set auto-commit
+    /* do nothing */


can we remove these comments?

src/extensions/qdatabase.c

wolkykim · 2024-05-14T16:31:26Z

src/extensions/qdatabase.c

+        /* get affected rows */
+        if ((affected = pgsql_affected_rows(db->pgsql)) < 0) affected = -1;
+    }
+
    Q_MUTEX_LEAVE(db->qmutex);
    return affected;
 #else


Maybe we can furder reduce the code duplication like the following example:

#if defined(Q_ENABLE_MYSQL) || defined(Q_ENABLE_PGSQL) common codes... #ifdef Q_ENABLE_MYSQL if (mysql_query(db->mysql, query) == 0) { if ((affected = mysql_affected_rows(db->mysql)) < 0) affected = -1; } #elif defined(Q_ENABLE_PGSQL) if (pgsql_query(db->pgsql, query, PQ_WRITE) == 0) { if ((affected = pgsql_affected_rows(db->pgsql)) < 0) affected = -1; } #endif common codes... #endif

src/extensions/qdatabase.c

wolkykim · 2024-05-14T16:36:17Z

src/extensions/qdatabase.c

+    // store
+    qdbresult_t *result = (qdbresult_t *)malloc(sizeof(qdbresult_t));
+    if (result == NULL) return NULL;
+    result->pgsql = db->pgsql;


Is it enough to store only pgresult, not the pgsql?

fetchtype is supported in new commits

wolkykim · 2024-05-14T16:42:43Z

src/extensions/qdatabase.c

+    /* assign methods */
+    result->get_str = result_get_str;
+    result->get_str_at = result_get_str_at;
+    result->get_int = result_get_int;
+    result->get_int_at = result_get_int_at;
+    result->get_next = result_get_next;


looks like these are common and can ignore code duplication.
I'm not trying to remove 100% of code duplication but some level that isn't very hard to do and still provides good code readability. Can you review the change from that perspective once again?

wolkykim · 2024-05-14T16:45:19Z

src/extensions/qdatabase.c

+    Q_MUTEX_ENTER(db->qmutex);
+    if (qtype_cast(qmutex_t*, db->qmutex)->count != 1) {
+        Q_MUTEX_LEAVE(db->qmutex);
+        return false;
+    }


again, I would place this in the common code section

wolkykim · 2024-05-14T16:55:02Z

src/internal/qlibpq.h

+
+/* Wrap the low-level functions of the libpq library */
+
+#ifndef QLIBPQ_H


I'd merge this into qdatabase.c

wolkykim · 2024-05-14T16:56:44Z

tests/test_qdatabase_pgsql.c

+ *****************************************************************************/
+/* This code is written and updated by following people and released under
+ * the same license as above qLibc license.
+ * Copyright (c) 2015 Zhenjiang Xie - https://github.com/Charles0429


This comment doesn't apply to your test. We can remove it.

wolkykim · 2024-05-14T16:57:04Z

tests/test_qdatabase_pgsql.c

+const char *default_password   = "123456";
+const bool  default_autocommit =  true;
+
+QUNIT_START("Test qdatabase_pgsql.c");


I like the test. Thanks.

wolkykim · 2024-05-14T16:57:33Z

tests/test_qdatabase_pgsql.c

+    default_db->free(default_db);
+}
+
+QUNIT_END();


missing new line?

wolkykim

Thanks for a great PR. I think this is a great addition to qLibc. Just left some of my thoughts. Please take a look.

SunBeau · 2024-05-18T16:17:58Z

Please review new commits

wolkykim · 2024-05-22T08:03:19Z

src/extensions/qdatabase.c

@@ -176,6 +397,12 @@ qdb_t *qdb(const char *dbtype, const char *addr, int port, const char *username,
        return NULL;
    }

+#ifdef Q_ENABLE_PGSQL
+    if (autocommit == false) {
+        return NULL;


Since this function can return NULL for more than a single reason, should we set errno ? like EINVAL for users to be able to determine what failed?

wolkykim · 2024-05-22T08:04:58Z

src/extensions/qdatabase.c

-        const char *password, const char *database, bool autocommit)
+qdb_t *qdb(const char *dbtype,
+           const char *addr, int port, const char *database,
+           const char *username, const char *password, bool autocommit)
 {
    // check db type
 #ifdef Q_ENABLE_MYSQL
    if (strcmp(dbtype, "MYSQL")) return NULL;


maybe we should set errno = ENOTSUP

wolkykim · 2024-05-22T08:05:12Z

src/extensions/qdatabase.c

 {
    // check db type
 #ifdef Q_ENABLE_MYSQL
    if (strcmp(dbtype, "MYSQL")) return NULL;
+#elif defined(Q_ENABLE_PGSQL)
+    if (strcmp(dbtype, "PGSQL")) return NULL;


here too errno = ENOTSUP

wolkykim · 2024-05-22T08:05:55Z

src/extensions/qdatabase.c

+ *  The pgsql server and libpq library does not provide a setting for autocommit.<br>
+ *  The following instructions are from the official manual(https://www.postgresql.org/docs/15/sql-begin.html):<br>
+ *  BEGIN initiates a transaction block, that is, all statements after a BEGIN command
+ *  will be executed in a single transaction until an explicit COMMIT or ROLLBACK is given.
+ *  By default (without BEGIN), PostgreSQL executes transactions in “autocommit” mode,
+ *  that is, each statement is executed in its own transaction
+ *  and a commit is implicitly performed at the end of the statement
+ *  (if execution was successful, otherwise a rollback is done).


wolkykim

It looks good to me. Just left some trivia comments about errno.

SunBeau · 2024-05-22T12:15:39Z

Thanks for your review, I've set errno.

wolkykim · 2024-05-22T12:41:19Z

src/extensions/qdatabase.c

+    return -1;
+}
+
+static inline int pgsql_num_rows(pgsql_t* pgsql)


I find this strange. Since this function is scoped to handle the result, not the connection.
Ideally, I think the prototype should be "pgsql_num_rows(PGresult * pgresult)"

wolkykim · 2024-05-22T12:41:54Z

src/extensions/qdatabase.c

+    return -1;
+}
+
+static inline int pgsql_num_fields(pgsql_t* pgsql)


same thoughts on here as well, pgsql_num_fields(PGresult * pgresult)

wolkykim · 2024-05-22T13:13:22Z

src/extensions/qdatabase.c

+    }
+
+    int cursor = pgsql->cursor;
+    if (++cursor >= pgsql->rows) {


Not, if (++cursor < pgsql->rows) ???

BTW, does it need to check the cursor number before it tries to fetch a row? Why not just attempt to fetch a row and return false when it's empty like below?

if (pgsql_fetch_row(pgsql) != 0) { return false; } pgsql->cursor++

wolkykim · 2024-05-22T13:09:03Z

src/extensions/qdatabase.c

+    /* get row num  */
+    int row_num = -1;
+    if (pgsql->rows == 1) {
+        row_num = 0;
+    } else {
+        row_num = pgsql->cursor;
+    }
+
+    /* get value */
+    const char *value = PQgetvalue(pgsql->pgresult, row_num, idx - 1);


Can this block be simplified as below?

const char *value = PQgetvalue(pgsql->pgresult, pgsql-rows - 1, idx - 1);

wolkykim · 2024-05-22T13:11:01Z

src/extensions/qdatabase.c

+
+#if defined(Q_ENABLE_PGSQL)
+    pgsql_t* pgsql = result->pgsql;
+    if (pgsql == NULL || pgsql->cols <= 0) {


looks like it is not really necessary to check pgsql->cols <= 0 as PGGetVallue() will handle out of range argument.

wolkykim · 2024-05-22T14:09:55Z

src/extensions/qdatabase.c

+    if (pgsql->pgresult) {
+        PQclear(pgsql->pgresult);
+        pgsql->pgresult = NULL;
+    }


we can just call pgsql_query_result() instead?

wolkykim · 2024-05-22T14:24:00Z

src/extensions/qdatabase.c

+    int rows;
+    int cols;
+    int cursor;


why store these in pgsql struct? Should they be in the result struct?

wolkykim · 2024-05-22T14:27:27Z

src/extensions/qdatabase.c

+    int rows;
+    int cols;
+    int cursor;
+} pgsql_t;


We have qdb_s struct already for this. Having a separate struct doesn't seem to be necessary.

wolkykim · 2024-05-22T14:28:41Z

include/qlibc/extensions/qdatabase.h

+
+#ifdef Q_ENABLE_PGSQL
+    /* private variables for pgsql database - do not access directly */
+    void *pgsql;


pgsql_t's members can be defined here rather than a separate inline struct.

PGresult *pgresult; int rows; int cols; int cursor;

wolkykim · 2024-05-22T14:33:09Z

include/qlibc/extensions/qdatabase.h

+
+#ifdef Q_ENABLE_PGSQL
+    /* private variables for pgsql database - do not access directly */
+    void  *pgsql;


I suggest we remove pgsql struct.

char *emsg; PGconn *pgconn;

src/extensions/qdatabase.c

wolkykim · 2024-05-22T14:46:28Z

I happened to leave a lot of comments. Please take a review.

SunBeau · 2024-06-01T16:26:20Z

I've been busy at work recently, so I spent some time today to complete the code update.
I have tried my best to keep the functionality consistent with MySQL.
To provide interfaces in the form of functions like pgsql_xyz(), some structures are necessary.

wolkykim · 2024-06-01T20:01:52Z

I read this. PostgreSQL actually treats every SQL statement as being executed within a transaction. If you do not issue a BEGIN command, then each individual statement has an implicit BEGIN and (if successful) COMMIT wrapped around it. A group of statements surrounded by BEGIN and COMMIT is sometimes called a *transaction block*. Note

…

On Sun, Jun 2, 2024, 1:33 AM SunBeau ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/extensions/qdatabase.c <#113 (comment)>: > Q_MUTEX_LEAVE(db->qmutex); return false; } +#if defined(Q_ENABLE_MYSQL) qdbresult_t *result; result = db->execute_query(db, "START TRANSACTION"); Regardless of whether autocommit is true or false, if you want to execute multiple statements in one transaction, you should start transaction. — Reply to this email directly, view it on GitHub <#113 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABUKQMO4XBJP62EXXT2ZGXTZFHZWRAVCNFSM6AAAAABHLEYLWSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJSGEZTQMRTGE> . You are receiving this because you commented.Message ID: ***@***.***>

SunBeau · 2024-06-04T10:49:01Z

Mysql

create table test(id int);

SET autocommit = OFF;
insert into test values (1);
select * from test; -- can get 1
-- exit，"id = 1" will be discarded, because "autocommit = OFF"

-- login again
select * from test; -- can not get 1
SET autocommit = ON;
insert into test values (2);
select * from test; -- can get 2
-- exit，"id = 2" will be commit

-- login again
select * from test; -- can get 2

-- A group of statements in one transaction
begin;
update test set id = 3 where id = 2;
insert into test values (4);
insert into test values (5);
rollback; -- if want to do this, "begin" is necessary

Pgsql

create table test(id int);

-- SET autocommit = OFF; -- This is not supported
insert into test values (1); -- "id = 1" will be commit
select * from test; -- can get 1
-- exit

-- login again
select * from test; -- can get 1
-- SET autocommit = ON; -- This is not supported
insert into test values (2); -- "id = 2" will be commit
select * from test; -- can get 1&2
-- exit

-- login again
select * from test; -- can get 1&2

-- A group of statements in one transaction
begin; -- This is always necessary
delete from test where id = 1;
update test set id = 3 where id = 2;
insert into test values (4);
insert into test values (5);
rollback;

wolkykim · 2024-06-04T19:10:23Z

Thanks for the update. Can you please indicate in each of the comments how it was handled? I see the updates but hard to track what was addressed.

wolkykim · 2024-08-16T05:40:17Z

This PR still needs to address feedback and undergo more thorough testing. Since there hasn't been any recent activity, I'm closing this PR for now. Please feel free to reopen it when it's ready for another look. I appreciate your contribution and efforts.

Support PostgreSQL: interface/test-cases/ci-testing

b8a2425

SunBeau mentioned this pull request May 7, 2024

Support PostgreSQL #112

Closed

wolkykim reviewed May 14, 2024

View reviewed changes

src/extensions/qdatabase.c Outdated

Comment on lines 373 to 374

// set auto-commit

/* do nothing */

Copy link

Owner

wolkykim May 14, 2024

Choose a reason for hiding this comment

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

can we remove these comments?

wolkykim reviewed May 14, 2024

View reviewed changes

src/extensions/qdatabase.c Outdated Show resolved Hide resolved

wolkykim reviewed May 14, 2024

View reviewed changes

src/extensions/qdatabase.c Show resolved Hide resolved

wolkykim reviewed May 14, 2024

View reviewed changes

tests/test_qdatabase_pgsql.c Outdated

default_db->free(default_db);

}

QUNIT_END();

Copy link

Owner

wolkykim May 14, 2024

Choose a reason for hiding this comment

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

missing new line?

wolkykim requested changes May 14, 2024

View reviewed changes

SunBeau added 4 commits May 19, 2024 00:07

merge qlibpq.h into qdatabase.c

b97c581

reduce code duplication

4ffead4

change a little code

48534f3

support fetchtype

8ab8d84

wolkykim reviewed May 22, 2024

View reviewed changes

wolkykim requested changes May 22, 2024

View reviewed changes

set errno

01fca68

wolkykim requested changes May 22, 2024

View reviewed changes

wolkykim reviewed May 22, 2024

View reviewed changes

src/extensions/qdatabase.c Outdated Show resolved Hide resolved

optimize and fix bug

dda555b

wolkykim closed this Aug 16, 2024


		/* Wrap the low-level functions of the libpq library */

		#ifndef QLIBPQ_H

Support PostgreSQL: interface/test-cases/ci-testing #113

Support PostgreSQL: interface/test-cases/ci-testing #113

Conversation

SunBeau commented May 7, 2024

wolkykim commented May 13, 2024 via email

Choose a reason for hiding this comment

wolkykim May 14, 2024 • edited Loading

Choose a reason for hiding this comment

wolkykim May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolkykim left a comment

Choose a reason for hiding this comment

SunBeau commented May 18, 2024

wolkykim May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolkykim left a comment

Choose a reason for hiding this comment

SunBeau commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolkykim commented May 22, 2024

SunBeau commented Jun 1, 2024

wolkykim commented Jun 1, 2024 via email

SunBeau commented Jun 4, 2024 • edited Loading

wolkykim commented Jun 4, 2024

wolkykim commented Aug 16, 2024

wolkykim May 14, 2024 •

edited

Loading

wolkykim May 14, 2024 •

edited

Loading

wolkykim May 22, 2024 •

edited

Loading

SunBeau commented Jun 4, 2024 •

edited

Loading