greenplum-db / plcontainer Goto Github PK

e.g. plcontainer_populate.out

I did not check carefully, but I assume they are useless now. We should double-check them and remove them accordingly.

it's a test please ignore it.

We do not seem to need to have this code now. After removing this we could close(listen_fd) although this is trivial.

see commit 94aceed317730953476bec490ce0148b2af3c383 upstream

Test if this issue will be synced to tracker.

Dave Cramer has an inital patch
https://github.com/greenplum-db/plcontainer/pull/2/files/7c5b15bc188accc7b719f1dd8eeb3ed107621fb7
Since the code changes a lot since then, more work need to be done. I'm creating the issue here so that later the support could be added later, hopefully not long.

Our code does not seem to align with upstream plpython according to the code although I do not understand that part of code that much. See plpy_spi.c

+       } else {
+           /* FIXME: Wrong ? */
+           args[j].data.isnull = 1;
+       }

Now postgresql extension is in gpdb now, we need to add support for this in plcontainer since extension has advantage over language.

Currently, we send metadata, such as udf source code from qe to client every call.
It's better to send metadata only once.

i.e. tests under stress/

Maybe move them to tests/

I've seen a lot of code similar like this.

send_int32(conn, call->hasChanged);

hasChanged is defined as int.

It would better to define them as the fixed-width variable, e.g.

s/int hasChanged/int32 hasChanged/

This could avoid potential bug.

currently we send source code from qe to client for every tuple. it not neccessary.

Currently we use log_min_messages to control log level at client side. we'd better use a separate GUC. Since plcontainer is dynamic loaded, set custom guc may not work on segment. Perhaps, after gpdb supporting create extension, we can revisit this problem.

To align with upstream (greenplum and postgres).

We saw some times during stress testing, libcurl hangs. It looks like the remote side does not reply. Anyway, we should have a timeout setting in our libcurl code.

Note the options below.
https://curl.haxx.se/libcurl/c/CURLOPT_CONNECTTIMEOUT.html
https://curl.haxx.se/libcurl/c/CURLOPT_TIMEOUT.html
CURLOPT_NOSIGNAL

It should not block there at accept() for ever until the container is deleted.

Detailed error message.
--- /home/gpadmin/plcontainer_src/tests/expected/faultinject_python.out 2017-12-27 23:26:52.654279211 +0000
+++ /home/gpadmin/plcontainer_src/tests/results/faultinject_python.out 2017-12-27 23:26:52.657279189 +0000
@@ -72,7 +72,7 @@
GP_IGNORE:
GP_IGNORE:-- end_ignore
! ssh psql -d ${PL_TESTDB} -c 'select address from gp_segment_configuration where dbid=2' -t -A docker ps -a </dev/null | wc -l
-1
+2

We really do not know much about the code coverage of our tests. We should have framework to test it.

We have both curl and non-curl docker api code. However the non-curl code has some bugs or limitation. e.g. it does not handle tcp partial read well; it does not have timeout mechanism; it does not support chunked encoding (so inspect api code actually is working around this). We might better remove this code and leave this to more professional package, i.e. libcurl.

We requires libcurl >=7.40 to use the curl code. I assume that is because the unix domain socket support in libcurl starts from 7.40. We could document this requirement on README and Makefile. In the long run, we might change to use tcp thus lower version libcurl is allowed.

1. Check whether settings are legal (keys/values are legal).
2. Dedup entries.
3. Check mandatory entries are missing or not. Also for each mandatory entry, check the legality (e.g. string requirement; do not exceed the allowed number; etc).

See below.

postgres=# DO LANGUAGE plpythonu $$
# container: plc_python_shared
print 1;
$$;
DO
postgres=#

postgres=# DO LANGUAGE plcontainer $$
# container: plc_python_shared
print 1;
$$;
ERROR:  language "plcontainer" does not support inline code execution

Currently clients do not have a solution to filter various levels of log.

We should allow to set them. Typically solution includes: Set level in client argument or set via guc ( environment variable and/or message).

Before spi execute with plan, it should double check the plan pointer which comes from the client code. A typical solution is to save previous spi-planned plans and have a check. See FIXME.

               /* FIXME: Sanity-check is needed!
+                * Maybe hash-store plan pointers for quick search?
+                * Or use array since we need to free all plans when backend quits.
+                * Or both?
+                */
+               plc_plan = (plcPlan *) ((char *) msg->pplan - offsetof(plcPlan, plan));
+               if (plc_plan->nargs != msg->nargs) {
+                   elog(ERROR, "argument number wrong for execute with plan: "
+                       "Saved number (%d) vs transferred number (%d)",
+                       plc_plan->nargs, msg->nargs);

For plan free, it is also needed.

In theory we have multiple kinds of backends (docker, separate process), we should try to rename "container" to "backend".

See FIXME in plpy_spi.c

 } else {
+       /* FIXME: For illegal type & error branch code above, do mem cleanup.
+        * It seems that receive_from_frontend() has this issue also.
+        */

That is to say, for the illegal type plcontainer_channel_receive() responds or the error response case, client program should do cleanup to avoid memory leak.

I run the client program after entering the container, and see this.
[root@16bb7a989b05 share]# ./pyclient
plcontainer log: pythonclient, gpadmin, postgres, 11275, LOG: Client has started execution at Fri Feb 9 02:11:44 2018
plcontainer log: pythonclient, gpadmin, postgres, 11275, ERROR: Socket timeout - no client connected within 20 seconds

Typically we could use a file to easily implement this.

e.g. Add plcontainer meta information in log. Remove or replace debug_print(). Maybe add a guc to control plcontainer log level.

At least

1. Do xmlFree() before elog(ERROR, ...) to avoid mem leak.
2. Address similar concerns as #226

This applies to both r and python.

While clients do not have memory context so we should be careful that there is memory leak in the code. Better have S/W infrastructure to easily test it.

e.g. In code below, (int16)out = .......

static int plc_pyobject_as_int2(PyObject *input, char **output, plcPyType *type UNUSED) {
int res = 0;
char out = (char)malloc(2);
*output = out;
if (PyInt_Check(input))
((short)out) = (short)PyInt_AsLong(input);

I am testing GitBot. Please ignore this issue and corresponding story created in the project's tracker (GPDB Procedural Languages).

Currently the readme points to github.com/greenplum-db/trusted-languages-poc but should point to github.com/greenplum-db/plcontainer for consistency's sake.

At least:

1. Sanity check if needed (argument number must be >= 0, size be >=0, etc);
2. Should have rx timeout if sender transmits bytes less than expected;

To help debugging.

Provide code coverage rate and report for specific release. Maye no need to run for each checkin though.

Recently there are a lot of change in plcontainer so that document is much out of date. So need to update README.md. At least:

1. Docker API version and OS version (We could run on both centos/rhel 6 and 7).
2. No "plcontainer configure" command.
3. Whether needs vagrant? Or at least should be friendly to users who have had a Linux environment.
4. Detailed step-by-step setup/build/test/run doc.

when run

plcontainer install -n plc_r_shared -i /usr/local/greenplum-db-devel/share/postgresql/plcontainer/plcontainer-r-images.tar.gz -c pivotaldata/plcontainer_r_shared:devel;

The host path is set to $GPHOME/bin/pyclient which should be $GPHOME/bin/rclient

Currently we created a shared path for unix domain socket when creating container. This path is writable so in theory client code could write under the path and this introduces a bit security concern. Potential solutions include: setuid to a less-privileged user after client initialization code runs and thus client code does not have permission to write under the path; set quota/limit for the path/directory in a feasible and simple way?

We also have log files in new shared path set in default configuration but we seem to be able to easily resolve this since it seems that there is sophisticated solution for container logging.

Currently R spi execute does not have limit. Upstream does not seem to have the functionality, but why not we provide these given this just needs very small effort since we have the support in shared code (for both python/r).

Currently we dynamically generate in pipeline. This is not friendly for test development in own dev environment.

We should remove those code in pipeline and move the default configuration into existing file plcontainer_configuration_test.xml.

Compile, and add icon on README.md if ok.

We might want to check coding style also.

We've seen case that docker fails to start during "medium-concurrency" testing (200-300 docker create/start). From users' respectively, we better have retry, instead of simply telling users a failure.

It needs to add some test cases to make sure the PL/Container returns correct results.
We can run PL/Container UDF, PL/R and PL/Python UDF in the same time with the same code to check the results in regression tests.

For insert/delete/update, currently we just support the "returning" versions of them although currently gpdb does not seem to support them. We should support the "non-returning" version in this function.

          switch (retval) {
            case SPI_OK_SELECT:
            case SPI_OK_INSERT_RETURNING:
            case SPI_OK_DELETE_RETURNING:
            case SPI_OK_UPDATE_RETURNING:
                /* some data was returned back */
                result = (plcMessage*)create_sql_result();
                break;
            default:
                elog(ERROR, "Cannot handle sql ('%s') with fn_readonly (%d) "
                     "and limit (%lld). Returns %d", msg->statement,
                     pinfo->fn_readonly, msg->limit, retval);
                break;
            }