Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
cobolbaby opened this issue Oct 17, 2023 · 7 comments
Closed

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

cobolbaby opened this issue Oct 17, 2023 · 7 comments

Comments

@cobolbaby
Copy link

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
Copy link
Contributor

mkgrgis commented Dec 9, 2023

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

\set SERVER 'host \'http://localhost\', port \'18086\', version \'1\''

\set SERVER 'host \'http://localhost\', port \'38086\', version \'2\', retention_policy \'\''

\set SERVER 'host \'http://localhost\', port \'8086\''

@mkgrgis
Copy link
Contributor

mkgrgis commented Dec 9, 2023

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
Copy link
Author

cobolbaby commented Dec 10, 2023

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
Copy link
Contributor

mkgrgis commented Dec 11, 2023

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

influxdb_fdw/option.c

Lines 106 to 107 in c7cc9f2

Datum
influxdb_fdw_validator(PG_FUNCTION_ARGS)
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
Copy link
Contributor

mkgrgis commented Dec 15, 2023

@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
Copy link
Contributor

mkgrgis commented Jan 31, 2024

Fixed in 054821e#diff-2241a1312d5b2aced309a893bb6e00ce87796229a454eccbd65fbdb6b4ac6912 by @jopoly. Please verify @cobolbaby and close this issue if all is ok with hostname option value.

@cobolbaby
Copy link
Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants