s3ext: fix error messages when failed to parse config file

When config file does not exist or is failed to parse, we write
log to pg_log by default. In previous, related logs are lost.
Besides, we initialize segid and segnum before parsing config
file to print correct message to users.
Signed-off-by: NHaozhou Wang <hawang@pivotal.io>

gpAux/extensions/gps3ext/include/s3utils.h

@@ -2,6 +2,7 @@
 #define __S3_UTILS_H__
 
 #include "s3common_headers.h"
+#include "s3log.h"
 
 #define MD5_DIGEST_STRING_LENGTH 17
 #define SHA_DIGEST_STRING_LENGTH 41
@@ -44,7 +45,6 @@ class MD5Calc {
 class Config {
    public:
     Config(const string& filename);
-    Config(const char* filename);
     ~Config();
     bool SectionExist(const string& sec);
     string Get(const string& sec, const string& key, const string& defaultvalue);

gpAux/extensions/gps3ext/regress/output/2_02_invalid_region.source

 CREATE READABLE EXTERNAL TABLE s3regress_invalid_region (date text, time text, open float, high float,
 	low float, volume int) LOCATION('s3://neverland.amazonaws.com/wherever/whatever/ config=@config_file@') format 'csv';
 SELECT count(*) FROM s3regress_invalid_region;
-ERROR:  Failed to init S3 extension, segid = 5, segnum = 6, please check your configurations and net connection: Failed to list bucket for URL: s3://neverland.amazonaws.com/wherever/whatever/, Function: open, File: src/s3bucket_reader.cpp(55). (gps3ext.cpp:88)  (seg5 slice1 ip-172-31-2-195.us-west-2.compute.internal:40001 pid=2338) (cdbdisp.c:1326)
+ERROR:  Failed to init S3 extension (segid = 5, segnum = 6), please check your configurations and net connection: Failed to list bucket for URL: s3://neverland.amazonaws.com/wherever/whatever/, Function: open, File: src/s3bucket_reader.cpp(55). (gps3ext.cpp:88)  (seg5 slice1 ip-172-31-2-195.us-west-2.compute.internal:40001 pid=2338) (cdbdisp.c:1326)
 DETAIL:  External table s3regress_invalid_region, file s3://neverland.amazonaws.com/wherever/whatever/ config=@config_file@
 DROP EXTERNAL TABLE s3regress_invalid_region;

gpAux/extensions/gps3ext/regress/output/2_03_invalid_config.source

 CREATE READABLE EXTERNAL TABLE s3regress_invalid_config (date text, time text, open float, high float,
 	low float, volume int) LOCATION('s3://s3-us-west-2.amazonaws.com/@read_prefix@/normal/') format 'csv';
 SELECT count(*) FROM s3regress_invalid_config;
-ERROR:  Failed to init S3 extension, segid = -1, segnum = -1, please check your configurations and network connection: reader_init caught a S3RuntimeError exception: Unexpected error: Failed to parse config file "s3/s3.conf", or it doesn't exist, Function: InitConfig, File: src/s3conf.cpp(28). (gps3ext.cpp:95)  (seg1 slice1 88daf7ed-1555-4d4c-5fca-62b98eea93ce:40001 pid=13622) (cdbdisp.c:1326)
+ERROR:  Failed to init S3 extension (segid = -1, segnum = -1), please check your configurations and network connection: reader_init caught a S3RuntimeError exception: Unexpected error: Failed to parse config file "s3/s3.conf", or it doesn't exist, Function: InitConfig, File: src/s3conf.cpp(28). (gps3ext.cpp:95)  (seg1 slice1 88daf7ed-1555-4d4c-5fca-62b98eea93ce:40001 pid=13622) (cdbdisp.c:1326)
 DETAIL:  External table s3regress_invalid_config, file s3://s3-us-west-2.amazonaws.com/@read_prefix@/normal/
 DROP EXTERNAL TABLE s3regress_invalid_config;

gpAux/extensions/gps3ext/src/gps3ext.cpp

@@ -102,8 +102,8 @@ Datum s3_import(PG_FUNCTION_ARGS) {
 
         gpreader = reader_init(url_with_options);
         if (!gpreader) {
-            ereport(ERROR, (0, errmsg("Failed to init S3 extension, segid = %d, "
-                                      "segnum = %d, please check your "
+            ereport(ERROR, (0, errmsg("Failed to init S3 extension (segid = %d, "
+                                      "segnum = %d), please check your "
                                       "configurations and network connection: %s",
                                       s3ext_segid, s3ext_segnum, s3extErrorMessage.c_str())));
         }
@@ -156,8 +156,8 @@ Datum s3_export(PG_FUNCTION_ARGS) {
 
         gpwriter = writer_init(url_with_options, format);
         if (!gpwriter) {
-            ereport(ERROR, (0, errmsg("Failed to init S3 extension, segid = %d, "
-                                      "segnum = %d, please check your "
+            ereport(ERROR, (0, errmsg("Failed to init S3 extension (segid = %d, "
+                                      "segnum = %d), please check your "
                                       "configurations and network connection: %s",
                                       s3ext_segid, s3ext_segnum, s3extErrorMessage.c_str())));
         }

gpAux/extensions/gps3ext/src/s3conf.cpp

@@ -13,23 +13,33 @@ int32_t s3ext_segid = -1;
 int32_t s3ext_segnum = -1;
 
 string s3ext_logserverhost;
-int32_t s3ext_loglevel = -1;
-int32_t s3ext_logtype = -1;
+int32_t s3ext_loglevel = EXT_WARNING;
+int32_t s3ext_logtype = INTERNAL_LOG;
 int32_t s3ext_logserverport = -1;
 int32_t s3ext_logsock_udp = -1;
 struct sockaddr_in s3ext_logserveraddr;
 
 bool InitConfig(S3Params& params, const string& conf_path, const string& section) {
+// initialize segment related info before loading config file, otherwise, if
+// it throws during parsing, segid and segnum values will be undefined.
+#ifdef S3_STANDALONE
+    s3ext_segid = 0;
+    s3ext_segnum = 1;
+#else
+    s3ext_segid = GpIdentity.segindex;
+    s3ext_segnum = GpIdentity.numsegments;
+#endif
+
     S3_CHECK_OR_DIE_MSG(!conf_path.empty(), S3RuntimeError, "Config file is not specified");
 
     Config s3cfg(conf_path);
 
     S3_CHECK_OR_DIE_MSG(s3cfg.Handle(), S3RuntimeError,
-                        "Failed to parse config file \"" + conf_path + "\", or it doesn't exist");
+                        "Failed to parse config file '" + conf_path + "', or it doesn't exist");
 
     S3_CHECK_OR_DIE_MSG(
         s3cfg.SectionExist(section), S3ConfigError,
-        "Selected section \"" + section + "\" does not exist, please check your configuration file",
+        "Selected section '" + section + "' does not exist, please check your configuration file",
         section);
 
     string content = s3cfg.Get(section.c_str(), "loglevel", "WARNING");
@@ -104,21 +114,13 @@ bool InitConfig(S3Params& params, const string& conf_path, const string& section
 
     params.setAutoCompress(to_bool(s3cfg.Get(section.c_str(), "autocompress", "true")));
 
-#ifdef S3_STANDALONE
-    s3ext_segid = 0;
-    s3ext_segnum = 1;
-#else
-    s3ext_segid = GpIdentity.segindex;
-    s3ext_segnum = GpIdentity.numsegments;
-#endif
-
     return true;
 }
 
 bool InitConfig(S3Params& params, const string& urlWithOptions) {
     string configPath = getOptS3(urlWithOptions, "config");
     if (configPath.empty()) {
-        S3ERROR("The 'config' parameter is not provided, use default value 's3/s3.conf'.");
+        S3WARN("The 'config' parameter is not provided, use default value 's3/s3.conf'.");
         configPath = "s3/s3.conf";
     }
 

gpAux/extensions/gps3ext/src/s3utils.cpp

@@ -165,16 +165,9 @@ Config::Config(const string &filename) : _conf(NULL) {
     if (!filename.empty()) this->_conf = ini_load(filename.c_str());
     if (this->_conf == NULL) {
 #ifndef S3_STANDALONE
-        write_log("Failed to load config file\n");
-#endif
-    }
-}
-
-Config::Config(const char *filename) : _conf(NULL) {
-    if (filename != NULL) this->_conf = ini_load(filename);
-    if (this->_conf == NULL) {
-#ifndef S3_STANDALONE
-        write_log("Failed to load configuration file\n");
+        write_log("Failed to load config file:'%s'\n", filename.c_str());
+#else
+        S3ERROR("Failed to load config file:'%s'", filename.c_str());
 #endif
     }
 }

gpAux/extensions/gps3ext/test/s3utils_test.cpp

@@ -53,17 +53,15 @@ TEST(Utils, sha256hmac) {
     EXPECT_STREQ("f7bc83f430538424b13298e6aa6fb143ef4d59a14946175997479dbc2d1a3cd8", hash_str);
 }
 
-TEST(Utils, ConfigNull) {
-    Config c1(NULL);
+TEST(Utils, ConfigEmpty) {
     uint64_t value = 0;
-    EXPECT_FALSE(c1.Scan("configtest", "config7", "%" PRIu64, &value));
 
-    Config c2("");
-    EXPECT_FALSE(c2.Scan("configtest", "config7", "%" PRIu64, &value));
+    Config c1("");
+    EXPECT_FALSE(c1.Scan("configtest", "config7", "%" PRIu64, &value));
 
     string str;
-    Config c3(str);
-    EXPECT_FALSE(c3.Scan("configtest", "config7", "%" PRIu64, &value));
+    Config c2(str);
+    EXPECT_FALSE(c2.Scan("configtest", "config7", "%" PRIu64, &value));
 }
 
 TEST(Utils, Config) {