arcapos / luapgsql Goto Github PK

Allocate the userdata (and attach metatable) upfront before calling lo_open.

Remember to move up the argument checks to before creating the userdata.

lua_pushstring can throw, which will result in PQfreemem(encrypted); never getting called.

Same issue in conn_escapeString, conn_escapeLiteral, conn_escapeIdentifier, conn_escapeBytea, conn_unescapeBytea, conn_getCopyData

Similar issue in conn_cancel

I tested out 5.3 support today, worked at a basic level by merely adding -DLUA_COMPAT_APIINTCASTS to CFLAGS.
The reason for this is the removal of luaL_checkint, use luaL_checkinteger instead.

For complete support, I think the only other thing would be supporting 64bit ints in get_sql_params in LUA_TNUMBER case:

• check lua_isinteger, if true:
• paramTypes[n] = INT8OID;
• *(uint64_t *)paramValues[n] = htobe64(lua_tointeger(L, t))

PQprepare returns NULL on failure.
You should return nil in this case.

Probably need to do the same for the other execution functions.

Now that we use lua_newuserdata() for intermediate values, I think they should be popped of the stack after they have been used.

In res_index you have the code:

    if (row < 0 || row > PQntuples(res))

This should be >=.

Hi,

While RPMifing luapgsql, I noticed that GNUMakefile is missing PGXS support. Is that intentional?

I tried adding related lines to GNUMakefile, but it fails at some point:

USE_PGXS=1 make 
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -DLINUX_OOM_ADJ=0 -I. -I./ -I/usr/pgsql-9.5/include/server -I/usr/pgsql-9.5/include/internal -D_GNU_SOURCE -I/usr/include/libxml2  -I/usr/include  -c -o luapgsql.o luapgsql.c
luapgsql.c:40:22: fatal error: libpq-fe.h: No such file or directory
 #include <libpq-fe.h>
                      ^
compilation terminated.
<builtin>: recipe for target 'luapgsql.o' failed
make: *** [luapgsql.o] Error 1

though everything seems to be normal :(

I'm adding these to GNUMakefile:

ifdef USE_PGXS
PG_CONFIG = /usr/pgsql-9.5/bin/pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
else
subdir = contrib/luapgsql
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif

Any comments?

Regards, Devrim

Continuing from #6 (comment)

One other API changing modification to make is around return values. There are functions that return 0 or 1, or -1 and 0. Using actual booleans would make sense.

e.g.

• conn_flush
• conn_sendQuery
• conn_sendQueryParams
• conn_sendPrepare
• conn_sendQueryPrepared
• conn_sendDescribePrepared
• conn_sendDescribePortal
• conn_setSingleRowMode

int PQputCopyEnd(PGconn *conn, const char *errormsg);
Ends the COPY_IN operation successfully if errormsg is NULL. If errormsg is not NULL then the COPY is forced to fail, with the string pointed to by errormsg used as the error message. (One should not assume that this exact error message will come back from the server, however, as the server might have already failed the COPY for its own reasons. Also note that the option to force failure does not work when using pre-3.0-protocol connections.)

To fail a copy you need to be able to pass an errormsg

Fix should be to hoist all the freeing above the luaL_getmetatable call.

Same issue in conn_prepare, conn_execPrepared

Hello,

I'd like to port Lapidus over to Lua and stop wrapping pg_recvlogical by connecting to PostgreSQL directly using the Streaming Replication Protocol.

I'm going to see if I can figure this out as the code is very readable. I only muck around in other people's C code, I don't write much of it from scratch, so should you choose to review/accept it be brutal on my PR (I could use the feedback).

Please let me know if you're interested or have any pointers.

Kudos on luapgsql. I look forward to using it!

Refers to null terminated string. Should just be lua string.

It will be very usefull if you would add support binary data to retrieve, for instance, number values.
I found the code below useful for me.

In this case I use getvalue_binary to retrieve long long value from database. It is needed to add support for simple int, float, double values, etc. This is just a scatch. Futher more, update static int conn_execParams(lua_State *L) method. I have not a lot of time to unserstand how you feed the data to this method.

static int
conn_exec_binary(lua_State *L)
{
    PGresult **res;

    res = lua_newuserdata(L, sizeof(PGresult *));
    *res = PQexecParams(pgsql_conn(L, 1), luaL_checkstring(L, 2),
                       0,       /* one param */
                       NULL,    /* int8[] OID */
                       NULL,
                       NULL,
                       NULL,
                       1);      /* ask for binary results */
    luaL_getmetatable(L, RES_METATABLE);
    lua_setmetatable(L, -2);
    return 1;
}

use

{ "exec", conn_exec },
{ "exec_binary", conn_exec_binary },
{ "getvalue_binary", res_getvalue_binary },

instead of

{ "exec", conn_exec },

and

uint64_t
ntoh64(const uint64_t *input)
{
    uint64_t rval;
    uint8_t *data = (uint8_t *)&rval;

    data[0] = *input >> 56;
    data[1] = *input >> 48;
    data[2] = *input >> 40;
    data[3] = *input >> 32;
    data[4] = *input >> 24;
    data[5] = *input >> 16;
    data[6] = *input >> 8;
    data[7] = *input >> 0;

    return rval;
}

uint64_t
hton64(const uint64_t *input)
{
    return (ntoh64(input));
}
static int
res_getvalue_binary(lua_State *L)
{
    lua_pushnumber(L, ntoh64(((uint64_t *)
        PQgetvalue(*(PGresult **)luaL_checkudata(L, 1, RES_METATABLE),
        luaL_checkinteger(L, 2) - 1, luaL_checkinteger(L, 3) - 1))));
    return 1;
}

An example of usage:

local = sql = string.format(" SELECT d1, d2, type FROM ddates WHERE d1 < %s ORDER BY id DESC LIMIT 1 " , tostring(os.time() * 1000))
res = conn:exec_binary(sql);
local t = res:getvalue_binary(1,1);

For small amount of rows it doesn't matter to use binary of text data. But for a lot of tons of content it can give emprovment.

res = conn:execParams('insert into public.test(bin, json) values ($a::bytea, $1::jsonb)', json.encode(test_table),testbin)

ERROR: invalid byte sequence for encoding "UTF8": 0xad
ERROR: invalid byte sequence for encoding "UTF8": 0xdd
ERROR: invalid byte sequence for encoding "UTF8": 0xf1

I am trying to install luapgsql via LuaRocks on my Windows 10 machine. Unfortunately it keeps failing.

C:\> luarocks install --local luapgsql PQ_DIR=C:/pgsql PQ_INCDIR=C:/pgsql/include
Installing https://luarocks.org/luapgsql-1.6.1-1.rockspec
mingw32-gcc -O2 -c -o luapgsql.o -IC:/lua/include luapgsql.c -IC:/pgsql/include
luapgsql.c: In function 'get_param':
luapgsql.c:509:34: warning: implicit declaration of function 'htobe64' [-Wimplicit-function-declaration]
    *(uint64_t *)paramValues[n] = htobe64(swap.i);
                                  ^~~~~~~
mingw32-gcc -shared -o pgsql.dll luapgsql.o -LC:/pgsql/lib -lpq C:/lua/lua53.dll -lm
C:/pgsql/lib/libpq.dll: file not recognized: File format not recognized
collect2.exe: error: ld returned 1 exit status

Error: Build error: Failed compiling module pgsql.dll

My system

• Windows 10
• MinGW (installed in C:\MinGW)
• Lua 5.3 (installed in C:\lua)
• LuaRocks 2.4.3 (installed in C:\LuaRocks)
• PostgreSQL 9.6.5 (installed in C:\pgsql incl. include files in C:\pgsql\include)

What am I missing?

This module is completely undocumented and I can't for the life of me figure out how to use... Any of it. Could you please whip up a quick markdown file explaining what functions are available, and how to use them?

Thanks!

Hi,

What do you think of changing library name from pgsql.so to luapgsql.so ? pgsql.so is very generic, and installing it under library dir may cause confusion.

Thanks!

Regards, Devrim

setNoticeProcessor and setNoticeReceiver set themselves in a global context; meaning all PGconns in one lua state end up sharing the same callbacks. This isn't acceptable for libraries that use luapgsql underneath.

I'm trying to debug some things at the moment, having luapgsql bind PQtrace and PQuntrace would be of great help.

PQescape has a single callsite (in conn_escape). IMO it could be improved upon (and possibly even removed entirely):

• Remove syslog calls (and hence dependency)
• Remove strlcpy calls (and hence dependency on libbsd on linux)
• Remove conn == NULL branch, as the callsite always provides a connection object
• Use lua memory management functions so we get correct error handling and cleanup
• No longer use a static buffer (char buf[1024];) in conn_escape

Hello,

I don't see the luarocks rockspec file in the repo, but given that it was uploaded on luarocks by one of the contributors, I assume it is under your control. Currently, the luarocks manifest has a Lua < 5.4 constraint, but as far as I can tell (having used it on a couple of my projects with Lua 5.4), it works fine with the new version. The test files in the repo also execute properly. Would it be possible to upload a new version to luarocks with support for 5.4?

Thanks,
Martin

The documentation states that the new setNoticeXX functions return the previous notice receiver or processor, but the C code (bindings) returns nothing (0). Also, you do not release the previous lock set with luaL_ref if the functions are called again. There are no luaL_unref calls in the code.

I am using your bindings in an environment where the setNoticeXX functions do not work and deadlocks the solution. I would appreciate it if you can add code for optionally removing these two functions by using ifdefs.

e.g. currently:

> p = require "pgsql"
> c = p.connectdb()
stdin:1: bad argument #1 to 'connectdb' (string expected, got pgsql connection methods)
stack traceback:
    [C]: in function 'pgsql.connectdb'
    stdin:1: in main chunk
    [C]: in ?

This is because in pgsql_connectdb you call pgsql_conn_new before calling luaL_checkstring.

Order should be:

1. Check all arguments (using e.g. luaL_checkstring)
2. Allocate all space needed for results
3. Make the actual call

I just realised that after calling :finish, *conn is NULL.
This isn't checked before calling any of the libpq functions.

e.g. this segfaults:

pg = require"pgsql"; 
conn = pg.connectdb("dbname=test")
conn:finish()
conn:isnonblocking()

I propose the creation of a helper function that validates a connection object (untested):

static PGconn*
pgsql_checkconn(lua_State *L, int n) {
    PGconn** conn = luaL_checkudata(L, n, CONN_METATABLE);
    luaL_argcheck(L, NULL != *conn, n, "connection pointer is NULL");
    return *conn;
}

Adding this in luapgsql.c is a bit tacky, but it gets it to compiling and building:

#ifdef __APPLE__
  #include <libkern/OSByteOrder.h>
  #define htobe64(x) OSSwapHostToBigInt64(x)
#else
  #include <endian.h>
#endif

I can fork and make a pull request if you like, but this is pretty minor.

They can be changed per server.
They must be fetched per connection with SELECT 'typename'::regtype::oid;
They may/should be cached (per connection)

This will be tricky to do non-blockingly.

http://www.postgresql.org/docs/current/static/libpq-notice-processing.html

Each of these functions returns the previous notice receiver or processor function pointer, and sets the new value. If you supply a null function pointer, no action is taken, but the current pointer is returned.

Is it possible to present query result row as associative array, where key is column name and value is column value for current row?

see http://php.net/manual/en/function.pg-fetch-assoc.php
and http://norman.github.io/lua-postgres/modules/postgres.result.html#fetch_assoc

PQprepare takes a list of oids; I'd like to pass these directly.
Currently in luapgsql you need to create an example piece of data for each type.
e.g. conn:prepare("mystatement", [[insert into example(col1, col2) values ($1, $2)]], 1, "two")) where 1 and "two" are values invented just to get the types correct.

As part of this there should be a table exposed pgsql.oid with all known oids.

Possibly also a function getdefaultoid given an object.

Need to include <sys/endian.h> under FreeBSD (at least in 12) otherwise it'll fail with htobe64 undefined.

(for readability)

see brianc/node-postgres#268

res_clear should probably have used testudata instead of checkudata

Same with conn_finish, notify_clear, pgsql_lo_clear

The current noticeReceiver and noticeProcessor use lua functions that may fail and longjmp out.

One option is to lua_pcall out to a second function.

• This could make it easier to add coroutine support (via lua_pcallk)

Alternatively, only use functions that can't fail.

• use a unique pointer and lua_rawgetp instead of lua_pushstring+lua_rawget
  • This is new in lua 5.2+
• preallocate the PGresult *
• never call luaL_error
  • A PQnoticeReceiver is by definition called for warnings or below, so it's not supposed to fail

At the moment you place a pointer to a largeObject in the userdata.
Instead, put largeObject in the userdata itself.

e.g. in conn_sendQueryParams

If the first argument is not a pgsql connection, then it will throw at line 821. However at this point paramTypes, paramValues, etc have already been allocated.

Fix is to move those arguments check to the start of the function.

Same issue is in other places, including conn_execParams, conn_prepare, conn_execPrepared, conn_sendPrepare, conn_sendQueryPrepared,

Should use pushlstring to save a strlen.

Also, I'm curious why you return the length as a 2nd return at all?

It looks like lines 504-508 in luapgsql.c should be:

for (n = 0, sqlParams = 0; n < nParams; n++) {
    get_sql_params(L, 4 + n, sqlParams, NULL, NULL, NULL, NULL,
        &count);
    sqlParams += count;
}

(that is, the curly braces are missing)

• PGRES_POLLING_OK
• PGRES_POLLING_READING
• PGRES_POLLING_WRITING
• PGRES_POLLING_FAILED

Returned from pgsql.connectPoll ( conn ) (which should really be a connection method btw)

PQgetCopyData returns:

• an integer >0 when a row is available (the integer is the row's length) + a row's data.
  This integer should be used as the length rather than using the row as a null terminated string
• 0 if data is not ready yet
• -1 if it's time to call PQgetResult to get the result code.

At the moment luapgsql only doesn't allow distinguishing 0 from -1,
Also, it doesn't use the length when pushing the row into lua.

asprintf(&paramValues[n], "%f", v) incorrectly renders inf for math.huge. It should render Infinity

Issue is at https://github.com/mbalmer/luapgsql/blob/054741811a92cfd8173be9d41a2b59f778309da4/luapgsql.c#L410.
Postgres docs: http://www.postgresql.org/docs/current/static/datatype-numeric.html

To allow explicit collection, please expose res_clear (currently a __gc metamethod) as res:clear()

Looking at the code in and around get_sql_params, there does not seem to be much checking for calloc, strdup, etc failing.

Most of the calloc callsites can probably be safely replaced with calls to lua_newuserdata, so that you get OOM handling for free from lua (it will longjmp out on failure).

Others such as paramValues[n] = strdup(lua_tostring(L, t)); will need to be fixed on a case by case basis.

Sometimes you would like to know if a connection object is valid before you attempt an operation on it.

Something like lua's io.type where it returns "pgsql connection" if open or "closed pgsql connection" when closed.

Related to daurnimator/cqueues-pgsql#1

luapgsql.adoc:

connectdb(conninfo) -- ... This function opens a new database connection using the parameters taken from the string conninfo.

It's unclear whether conninfo is structure or string. If it only may be the string please rename it to connectionString. And provide some usage examples please.

thx .first

how do i build and install this module?

it seems that luarocks is installing an older version 0.5-1, with code from syn.zadzmo.org?

im using mac, with lua and luarocks installed with homebrew.

Hi Marc,

The current rockspec available up on luarocks doesn't work for me.
I spent some time trying to get it down to the minimum required:

package = "luapgsql"
version = "scm-3"
source = {
   url = "git://github.com/mbalmer/luapgsql";
}
description = {
   summary = "A Lua Binding for PostgreSQL";
   homepage = "http://github.com/mbalmer/luapgsql";
   license = "3-clause BSD";
}
dependencies = {
   "lua >= 5.1, < 5.3";
}
external_dependencies = {
   POSTGRESQL = {
      header = "postgres_fe.h";
   };
   PQ = {
      header = "libpq-fe.h";
      library = "pq";
   };
   platforms = {
      linux = {
         -- for strlcpy
         LIBBSD = {
            header = "bsd/bsd.h";
            library = "bsd";
         };
      };
   };
}
build = {
   type = "builtin";
   modules = {
      pgsql = {
         sources = "luapgsql.c";
         incdirs = { "$(POSTGRESQL_INCDIR)" };
         libdirs = { "$(POSTGRESQL_LIBDIR)" };
         libraries = { "pq" };
      };
   };
   platforms = {
      linux = {
         modules = {
            pgsql = {
               incdirs = { [2] = "$(LIBBSD_INCDIR)"; };
               libdirs = { [2] = "$(LIBBSD_LIBDIR)"; };
               libraries = { [2] = "bsd"; };
               defines = { "_GNU_SOURCE" }; -- for asprintf
            };
         };
      };
   };
}

The header = "postgres_fe.h"; should be correct, as that is the actual #include in the code: https://github.com/mbalmer/luapgsql/blob/master/luapgsql.c#L36

This works on my arch linux system, with the postgres server installed. You can see an example invocation here:

$ sudo luarocks install luapgsql-scm-3.rockspec POSTGRESQL_INCDIR=/usr/include/postgresql/server
Using luapgsql-scm-3.rockspec... switching to 'build' mode
Cloning into 'luapgsql'...
remote: Counting objects: 13, done.
remote: Compressing objects: 100% (11/11), done.
remote: Total 13 (delta 0), reused 7 (delta 0)
Receiving objects: 100% (13/13), 99.58 KiB | 0 bytes/s, done.
Checking connectivity... done.
gcc -O2 -fPIC -I/usr/include -c luapgsql.c -o luapgsql.o -D_GNU_SOURCE -I/usr/include/postgresql/server -I/usr/include
gcc -shared -o pgsql.so -L/usr/lib luapgsql.o -L/usr/local/lib -L/usr/lib -Wl,-rpath,/usr/local/lib: -Wl,-rpath,/usr/lib: -lpq -lbsd
Updating manifest for /usr/lib/luarocks/rocks-5.2
luapgsql scm-3 is now built and installed in /usr (license: 3-clause BSD)

I was wondering which of the bsd bits need to remain:

• does libpq need to be linked as -lpostgres?
• why was postgres.h required?

libbsd still needs to be included because of strlcpy.