Bind PQtrace and PQuntrace

[daurnimator]

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

[mbalmer]

Am 25.02.15 um 21:22 schrieb daurnimator:

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

Should :trace() take a filename as argument or a (Lua) file object?

[daurnimator]

Should :trace() take a filename as argument or a (Lua) file object?

a lua file object would be preferable.

In 5.2 (undocumented) and 5.3 you have luaL_Stream

Lua 5.1, didn't export the file handle defintion, but it's known you can cast the userdata to a FILE*.

[mbalmer]

Am 25.02.15 um 21:42 schrieb daurnimator:

Should :trace() take a filename as argument or a (Lua) file object?

a lua file object would be preferable.

In 5.2 (undocumented) and 5.3 you have |luaL_Stream| http://www.lua.org/manual/5.3/manual.html#luaL_Stream

Lua 5.1, didn't export the file handle defintion, but it's known you can cast the userdata to a |FILE*|.

ok, I will take a look.

[mbalmer]

I just added conn:trace() and conn:untrace(). Simply casting the Lua filehandle to FILE *, maybe I will add a check for fp != NULL. Looks like Lua can set it to NULL when the file is closed.

[mbalmer]

Done.

[daurnimator]

maybe I will add a check for *fp != NULL. Looks like Lua can set it to NULL when the file is closed.

Should probably throw an error if it is NULL. Use luaL_argcheck?

[mbalmer]

Am 27.02.15 um 17:59 schrieb daurnimator:

maybe I will add a check for *fp != NULL. Looks like Lua can set it
to NULL when the file is closed.

Should probably throw an error if it is |NULL|. Use |luaL_argcheck|?

Right!

[daurnimator]

So I just gave this a try. If you don't :untrace(), libpq segfaults.

I think this is caused by lua garbage collecting (and hence closing) the file descriptor. This seems to be confirmed by this also segfaulting before seeing A:

pg=require"pgsql";
conn = pg.connectdb("dbname=test");
assert(conn:status() == pg.CONNECTION_OK, conn:errorMessage())
fd = assert(io.open("trace", "w"));
conn:trace(fd);
fd:close();
res = assert(conn:exec("SELECT 1"), conn:errorMessage())
print("A")

I'm trying to think of how we can prevent this:

• Attach the file object to the PGconn userdata (using lua_setuservalue or such); this fixes the regular case of cleanup order and file descriptor leaving scope
• fdopen(dup(fileno(fd*))) the FILE* and give an exclusive handle to libpq

[mbalmer]

What I am thinking of is to "somehow" record the closef pointer in the luaL_Stream structure and replace it by a function that would call first PQuntrace and then the original close function. So that closing the file that was handed to :trace() would do an implicit :untrace().

And, maybe the C code should call luaL_ref() on the filehandle, to make sure it does not get GCed.

Then you could pbly do things like:

function enableTracing(conn, filename) local f = io.open(filename, 'w') conn:trace(f) end

[daurnimator]

What I am thinking of is to "somehow" record the closef pointer in the luaL_Stream structure and replace it by a function that would call first PQuntrace and then the original close function. So that closing the file that was handed to :trace() would do an implicit :untrace().

This is a great solution for lua 5.2 and 5.3.

To get it to work with lua 5.1, which uses environments, we'd need to swap out the __close function in the environment of the file handle.

[mbalmer]

Not sure if that can work. What if you call :trace() a second time, with a different file handle? Maybe we should not add to much magic. Maybe the problem should be handled at the libpq() level.

[daurnimator]

What if you call :trace() a second time, with a different file handle?

well I think libpq only allows one trace handle at a time anyway. So call :untrace in :trace()? (which should cleanup the close swap-out it did)

[mbalmer]

There is only one trace handle, yes. But if we add magic to call :untrace() when a file gets closed, this will cause mayhem: f = io.open('trace', 'w') db:trace(f) f2 = io.open('trace2', 'w') db:trace(f2) f:close()

Right now, I think there is no proper solution to this problem other than not calling close (or have garbage collected) a file handle that has been passed to trace() before calling :untrace().

[daurnimator]

There is only one trace handle, yes. But if we add magic to call :untrace() when a file gets closed, this will cause mayhem: f = io.open('trace', 'w') db:trace(f) f2 = io.open('trace2', 'w') db:trace(f2) f:close()

Why will that cause mayhem?

In :untrace we should be removing our closef/__close wrapper.

[mbalmer]

Because f.close() could call untrace.

I am now thinking that is whole issue it not worth the time. Just don't close file handles that you pass to :trace(). It's a corner case anyways.

[daurnimator]

Because f.close() could call untrace.

Why/how?

I am now thinking that is whole issue it not worth the time. Just don't close file handles that you pass to :trace(). It's a corner case anyways.

The explicit :close() mayble. But the segfault when the handle is collected really does need to be addressed.

[mbalmer]

Am 02.03.15 um 20:28 schrieb daurnimator:

Because f.close() could call untrace.

Why/how?

I am now thinking that is whole issue it not worth the time. Just
don't close file handles that you pass to :trace(). It's a corner
case anyways.

The explicit |:close()| mayble. But the segfault when the handle is collected really does need to be addressed.

I can not think of an easy way to accomplish this. So for the moment avoid calling explicit :close() and avoid code that lets a file handle to be collected before :untrace() had been called.

That is not satisfactory but I guess there is not much we can do. If you find an easy and elegant solution that does not bloat the code, I am all open for it. I tried quite some ideas, non of them were really good.

[daurnimator]

I can not think of an easy way to accomplish this. So for the moment avoid calling explicit :close() and avoid code that lets a file handle to be collected before :untrace() had been called.

That is not satisfactory but I guess there is not much we can do. If you find an easy and elegant solution that does not bloat the code, I am all open for it. I tried quite some ideas, non of them were really good.

Please review the pull request I sent over. it does all the things I mentioned :)