pgspider / influxdb_fdw

InfluxDB Foreign Data Wrapper for PostgreSQL.
Other
58 stars 14 forks source link

create server with option host 'localhost' crash the database #38

Closed cobolbaby closed 9 months ago

cobolbaby commented 1 year ago

It seems that the option host should be prefixed with http:// or https://. Without the protocol header, the select statement on foreign_table will crash the postgresql 12.x , and you would get the error as below:

terminate called after throwing an instance of 'influxdb::InfluxDBException'
  what():  influx-cxx [GetTransport]: Ill-formed URI

I would like to ask if it is possible to verify the option when creating the server to avoid illegal format. Even if the format is illegal, it is better to give an error message when executing the query rather than crashing the database.

mkgrgis commented 11 months ago

Thanks, @cobolbaby ! Really http:// or https:// usage is important and undocumented. I am seeing all testing examples with this address format

https://github.com/pgspider/influxdb_fdw/blob/c7cc9f2138b87b6761476fed11381104022bb0de/sql/parameters_cxx_v1.conf#L2 https://github.com/pgspider/influxdb_fdw/blob/c7cc9f2138b87b6761476fed11381104022bb0de/sql/parameters_cxx_v2.conf#L2 https://github.com/pgspider/influxdb_fdw/blob/c7cc9f2138b87b6761476fed11381104022bb0de/sql/parameters_go.conf#L2

mkgrgis commented 11 months ago

Even if the format is illegal, it is better to give an error message when executing the query rather than crashing the database.

This is partially to influx-cxx https://github.com/pgspider/influxdb-cxx , see https://github.com/pgspider/influxdb-cxx/blob/5f2440d19ae457c53c0bbdb9337b6c10bd10e2cc/src/InfluxDBFactory.cxx#L65

As we know, InfluxDB metadata usage isn't very good implemented in this FDW. Can you try to improve this in C and C++? I can help with testing environment getting.

cobolbaby commented 11 months ago

As we know, InfluxDB metadata usage isn't very good implemented in this FDW. Can you try to improve this in C and C++? I can help with testing environment getting.

If I wanna do more complete exception handling in influxdb_fdw, which piece of code needs to be modified?

mkgrgis commented 11 months ago

If I wanna do more complete exception handling in influxdb_fdw, which piece of code needs to be modified?

In my opinion option value validating https://github.com/pgspider/influxdb_fdw/blob/c7cc9f2138b87b6761476fed11381104022bb0de/option.c#L106-L107 will good place for this. Please refer journal_mode operations in option.c https://github.com/pgspider/sqlite_fdw/pull/67/files#diff-2241a1312d5b2aced309a893bb6e00ce87796229a454eccbd65fbdb6b4ac6912 and something about partial string comparing in C. You can check if some initial characters is http:// or https:// and invalidate option otherwise before any FDW operations. Hence no need care about influx-cxx input values. Also please don't forget to write a test for invalid value near tests of valid values and follow existed PostgreSQL indent style.

mkgrgis commented 11 months ago

@cobolbaby here is one of prefix compare functions, you can ensure it works as expected and use.

bool check_prefix(const char *str, const char *prefix)
{
    return strncmp(prefix, str, strlen(prefix)) == 0;
}
mkgrgis commented 9 months ago

Fixed in https://github.com/pgspider/influxdb_fdw/commit/054821ed3fd6db18ee2040468a639fda036207a4#diff-2241a1312d5b2aced309a893bb6e00ce87796229a454eccbd65fbdb6b4ac6912 by @jopoly. Please verify @cobolbaby and close this issue if all is ok with hostname option value.

cobolbaby commented 9 months ago

@mkgrgis It works.

ALTER SERVER influxdb_svr
    OPTIONS (SET host 'infra-influx.itc.inventec.net');

ERROR:  influxdb_fdw: Host address must start with either http:// or https://
SQL state: XX000