Re: [HACKERS] Extensions, this time with a patch
Robert Haas writes: > I have committed the cfparser patch to which the above comments refer. Excellent, thank you! On to merging that into the extension's main branch, will still wait until after pg_execute_sql_file() is in to produce extension.v15.patch, unless advised otherwise. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Thu, Nov 25, 2010 at 4:06 PM, Dimitri Fontaine wrote: > Itagaki Takahiro writes: >> Thanks. I'll move the patch to Ready for Committer. > > Thanks! I have committed the cfparser patch to which the above comments refer. One patch per thread makes things easier! I adopted most of Itagaki Takahiro's suggestions, which were very helpful. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Itagaki Takahiro writes: > Thanks. I'll move the patch to Ready for Committer. Thanks! -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Thu, Nov 25, 2010 at 05:02, Dimitri Fontaine wrote: > Something like the attached, version 5 of the patch? I've been using the > function name ParseConfigFp because the internal parameter was called fp > in the previous function body. I suppose that could easily be changed at > commit time if necessary. Thanks. I'll move the patch to Ready for Committer. Here is a list of items for committers, including only cosmetic changes. - Comments in recovery.conf.sample needs to be adjusted. # (The quotes around the value are NOT optional, but the "=" is.) It seems to be the only description about quotes are not omissible before. - We might not need to check the result of ParseConfigFp() because it always raises FATAL on errors. - We could remove the unused argument "calling_file" in ParseConfigFp(). - I feel "struct name_value_pair" and "ConfigNameValuePair" a bit redundant names. I'd prefer something like ConfigVariable. - "fp" might be a better name than FILE *fd. There are 4 usages in xlog.c. > Extensions will need the support for custom_variable_classes as it is > done now, and as you say, the recovery will just error out. You have to > clean your recovery.conf file then try again (I just tried and confirm). > > I personally don't see any harm to have the features in all currently > known uses-cases, and I don't see any point in walking an extra mile > here to remove a feature in some cases. Sure. We will leave them. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Itagaki Takahiro writes: > RECOVERY_COMMAND_FILE is opened twice in the patch. The first time > is for checking the existence, and the second time is for parsing. > Instead of the repeat, how about adding FILE* version of parser? > It will be also called from ParseConfigFile() as a sub routine. > > bool ParseConfigFd(FILE *fd, const char *config_file, int depth, ...) Something like the attached, version 5 of the patch? I've been using the function name ParseConfigFp because the internal parameter was called fp in the previous function body. I suppose that could easily be changed at commit time if necessary. > BTW, the parser supports "include" and "custom_variable_classes" > not only for postgresql.conf but also for all files. Is it an > intended behavior? I think they are harmless, so we don't have > to change the codes; "include" might be useful even in recovery.conf, > and "custom_variable_classes" will be "unrecognized recovery > parameter" error after all. Extensions will need the support for custom_variable_classes as it is done now, and as you say, the recovery will just error out. You have to clean your recovery.conf file then try again (I just tried and confirm). I personally don't see any harm to have the features in all currently known uses-cases, and I don't see any point in walking an extra mile here to remove a feature in some cases. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/src/backend/access/transam/xlog.c --- b/src/backend/access/transam/xlog.c *** *** 5024,5200 str_time(pg_time_t tnow) } /* - * Parse one line from recovery.conf. 'cmdline' is the raw line from the - * file. If the line is parsed successfully, returns true, false indicates - * syntax error. On success, *key_p and *value_p are set to the parameter - * name and value on the line, respectively. If the line is an empty line, - * consisting entirely of whitespace and comments, function returns true - * and *keyp_p and *value_p are set to NULL. - * - * The pointers returned in *key_p and *value_p point to an internal buffer - * that is valid only until the next call of parseRecoveryCommandFile(). - */ - static bool - parseRecoveryCommandFileLine(char *cmdline, char **key_p, char **value_p) - { - char *ptr; - char *bufp; - char *key; - char *value; - static char *buf = NULL; - - *key_p = *value_p = NULL; - - /* - * Allocate the buffer on first use. It's used to hold both the parameter - * name and value. - */ - if (buf == NULL) - buf = malloc(MAXPGPATH + 1); - bufp = buf; - - /* Skip any whitespace at the beginning of line */ - for (ptr = cmdline; *ptr; ptr++) - { - if (!isspace((unsigned char) *ptr)) - break; - } - /* Ignore empty lines */ - if (*ptr == '\0' || *ptr == '#') - return true; - - /* Read the parameter name */ - key = bufp; - while (*ptr && !isspace((unsigned char) *ptr) && - *ptr != '=' && *ptr != '\'') - *(bufp++) = *(ptr++); - *(bufp++) = '\0'; - - /* Skip to the beginning quote of the parameter value */ - ptr = strchr(ptr, '\''); - if (!ptr) - return false; - ptr++; - - /* Read the parameter value to *bufp. Collapse any '' escapes as we go. */ - value = bufp; - for (;;) - { - if (*ptr == '\'') - { - ptr++; - if (*ptr == '\'') - *(bufp++) = '\''; - else - { - /* end of parameter */ - *bufp = '\0'; - break; - } - } - else if (*ptr == '\0') - return false; /* unterminated quoted string */ - else - *(bufp++) = *ptr; - - ptr++; - } - *(bufp++) = '\0'; - - /* Check that there's no garbage after the value */ - while (*ptr) - { - if (*ptr == '#') - break; - if (!isspace((unsigned char) *ptr)) - return false; - ptr++; - } - - /* Success! */ - *key_p = key; - *value_p = value; - return true; - } - - /* * See if there is a recovery command file (recovery.conf), and if so * read in parameters for archive recovery and XLOG streaming. * ! * XXX longer term intention is to expand this to ! * cater for additional parameters and controls ! * possibly use a flex lexer similar to the GUC one */ static void readRecoveryCommandFile(void) { FILE *fd; - char cmdline[MAXPGPATH]; TimeLineID rtli = 0; bool rtliGiven = false; ! bool syntaxError = false; fd = AllocateFile(RECOVERY_COMMAND_FILE, "r"); if (fd == NULL) { if (errno == ENOENT) ! return;/* not there, so no archive recovery */ ereport(FATAL, (errcode_for_file_access(), errmsg("could not open recovery command file \"%s\": %m", RECOVERY_COMMAND_FILE))); } ! /* ! * Parse the file... ! */ ! while (fgets(cmdline, sizeof(cmdline), fd) != NULL) ! { ! char *tok1; ! char *tok2; ! if (!parseRecoveryCommandFileLine(cmdline, &tok1, &tok2)) ! { ! syntaxError = true; ! break; ! } ! if (tok1 == NULL) ! continue; ! ! i
Re: [HACKERS] Extensions, this time with a patch
On Tue, Nov 23, 2010 at 18:19, Dimitri Fontaine wrote: > Please find that done in attached v4 of the cfparser patch. RECOVERY_COMMAND_FILE is opened twice in the patch. The first time is for checking the existence, and the second time is for parsing. Instead of the repeat, how about adding FILE* version of parser? It will be also called from ParseConfigFile() as a sub routine. bool ParseConfigFd(FILE *fd, const char *config_file, int depth, ...) BTW, the parser supports "include" and "custom_variable_classes" not only for postgresql.conf but also for all files. Is it an intended behavior? I think they are harmless, so we don't have to change the codes; "include" might be useful even in recovery.conf, and "custom_variable_classes" will be "unrecognized recovery parameter" error after all. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On 22.11.2010 03:35, Robert Haas wrote: On Sun, Nov 21, 2010 at 8:10 PM, Itagaki Takahiro wrote: On Wed, Oct 20, 2010 at 01:36, Dimitri Fontaine wrote: Ah yes, thinking it's an easy patch is not helping. Please find attached a revised version of it. I checked cfparser.v2.patch. It exports the static parseRecoveryCommandFileLine() in xlog.c as the global cfParseOneLine() in cfparser.c without modification. It generates one warning, but it can be easily fixed. cfparser.c:34: warning: no previous prototype for 'cfParseOneLine' Some discussions about the patch: * Is "cf" the best name for the prefix? Less abbreviated forms might be less confusable. Personally, I prefer "conf". * Can we export ParseConfigFile() in guc-file.l rather than parseRecoveryCommandFileLine()? It can solve the issue that unquoted parameter values in recovery.conf are not recognized. Even if we won't merge them, just allowing unquoted values would be useful. I'd really like to see postgresql.conf and recovery.conf parsing merged, and I suspect, as Itagaki-san says, that postgresql.conf parsing is the better model for any new code. +1. There was unanimous agreement in the synchronous replication threads that recovery.conf should be parsed with the GUC parser. The current recovery.conf parser doesn't support escaping, for example. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: > the handling of relative vs absolute paths is bogus here. I think it'd > make more sense to have a bool "are we including"; and if that's false and > the path is not absolute, then the file is relative to CWD; or maybe we > make it absolute by prepending PGDATA; maybe something else? (need to > think of something that makes sense for both recovery.conf and extension > control files) Current coding in extensions prepend any control or script file with sharepath, so that we're only dealing with absolute filename here. The idea is that it's no business for any other part of the code to have to know where we decide to install control and script files. My feeling is that when !is_absolute_path(config_file) and calling_file is NULL we should make the config_file absolute by prepending PGDATA. Please find that done in attached v4 of the cfparser patch. >> If that looks ok, do we want to add some documentation about the new >> lexer capabilities? > > beyond extra code comments? probably not. Great. >> Also, for what good reason would we want to prevent >> people from using the include facility? > > Not sure about this Ok, nothing special here. -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/src/backend/access/transam/xlog.c --- b/src/backend/access/transam/xlog.c *** *** 5024,5200 str_time(pg_time_t tnow) } /* - * Parse one line from recovery.conf. 'cmdline' is the raw line from the - * file. If the line is parsed successfully, returns true, false indicates - * syntax error. On success, *key_p and *value_p are set to the parameter - * name and value on the line, respectively. If the line is an empty line, - * consisting entirely of whitespace and comments, function returns true - * and *keyp_p and *value_p are set to NULL. - * - * The pointers returned in *key_p and *value_p point to an internal buffer - * that is valid only until the next call of parseRecoveryCommandFile(). - */ - static bool - parseRecoveryCommandFileLine(char *cmdline, char **key_p, char **value_p) - { - char *ptr; - char *bufp; - char *key; - char *value; - static char *buf = NULL; - - *key_p = *value_p = NULL; - - /* - * Allocate the buffer on first use. It's used to hold both the parameter - * name and value. - */ - if (buf == NULL) - buf = malloc(MAXPGPATH + 1); - bufp = buf; - - /* Skip any whitespace at the beginning of line */ - for (ptr = cmdline; *ptr; ptr++) - { - if (!isspace((unsigned char) *ptr)) - break; - } - /* Ignore empty lines */ - if (*ptr == '\0' || *ptr == '#') - return true; - - /* Read the parameter name */ - key = bufp; - while (*ptr && !isspace((unsigned char) *ptr) && - *ptr != '=' && *ptr != '\'') - *(bufp++) = *(ptr++); - *(bufp++) = '\0'; - - /* Skip to the beginning quote of the parameter value */ - ptr = strchr(ptr, '\''); - if (!ptr) - return false; - ptr++; - - /* Read the parameter value to *bufp. Collapse any '' escapes as we go. */ - value = bufp; - for (;;) - { - if (*ptr == '\'') - { - ptr++; - if (*ptr == '\'') - *(bufp++) = '\''; - else - { - /* end of parameter */ - *bufp = '\0'; - break; - } - } - else if (*ptr == '\0') - return false; /* unterminated quoted string */ - else - *(bufp++) = *ptr; - - ptr++; - } - *(bufp++) = '\0'; - - /* Check that there's no garbage after the value */ - while (*ptr) - { - if (*ptr == '#') - break; - if (!isspace((unsigned char) *ptr)) - return false; - ptr++; - } - - /* Success! */ - *key_p = key; - *value_p = value; - return true; - } - - /* * See if there is a recovery command file (recovery.conf), and if so * read in parameters for archive recovery and XLOG streaming. * ! * XXX longer term intention is to expand this to ! * cater for additional parameters and controls ! * possibly use a flex lexer similar to the GUC one */ static void readRecoveryCommandFile(void) { FILE *fd; - char cmdline[MAXPGPATH]; TimeLineID rtli = 0; bool rtliGiven = false; ! bool syntaxError = false; fd = AllocateFile(RECOVERY_COMMAND_FILE, "r"); if (fd == NULL) { if (errno == ENOENT) ! return;/* not there, so no archive recovery */ ereport(FATAL, (errcode_for_file_access(), errmsg("could not open recovery command file \"%s\": %m", RECOVERY_COMMAND_FILE))); } ! /* ! * Parse the file... ! */ ! while (fgets(cmdline, sizeof(cmdline), fd) != NULL) ! { ! char *tok1; ! char *tok2; ! ! if (!parseRecoveryCommandFileLine(cmdline, &tok1, &tok2)) ! { ! syntaxError = true; ! break; ! } ! if (tok1 == NULL) ! continue; ! if (strcmp(tok1, "restore_command") == 0) { ! recoveryRestoreCommand = pstrdup(tok2); ereport(DEBUG2, (errmsg("restore_command = '%s'", recoveryRestoreCommand))); } !
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: > Hmm, the first thought that comes to mind is that the GucContext param > to ParseConfigFile is unused and can be removed. This is probably an > oversight from when include files were introduced in 2006. Thanks for caring about that part. > I don't like the fact that this code handles custom_variable_classes > internally. I think this would be exposed to the parsing of extension > control files, which is obviously wrong. Well, in fact, not that much. The extension code has a special error case when dealing with custom variables if the class hasn't been already parsed, and what ParseConfigFile() is doing is pushing the custom_variable_classes setting in front of the list. guc-file.l says: /* * This variable must be processed first as it controls * the validity of other variables; so it goes at the head * of the result list. If we already found a value for it, * replace with this one. */ extension.c says: ereport(ERROR, (errmsg("Unsupported parameter '%s' in file: %s", tok1, filename), errhint("Be sure to have 'custom_variable_classes' set " "in a line before any custom variable."))); So if we don't change the code in ParseConfigFile() that will push custom_variable_classes in front of the list, all I have to change in the extension.c file is the error message. I fail to see a future usage of custom_variable_classes where it wouldn't help to have that in the list before any user setting that depends on it. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of lun nov 22 18:12:39 -0300 2010: > Itagaki Takahiro writes: > > No. My suggestion was just to use the internal parser. > > What about something like the attached, cfparser.v3.patch? the handling of relative vs absolute paths is bogus here. I think it'd make more sense to have a bool "are we including"; and if that's false and the path is not absolute, then the file is relative to CWD; or maybe we make it absolute by prepending PGDATA; maybe something else? (need to think of something that makes sense for both recovery.conf and extension control files) > If that looks ok, do we want to add some documentation about the new > lexer capabilities? beyond extra code comments? probably not. > Also, for what good reason would we want to prevent > people from using the include facility? Not sure about this -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Alvaro Herrera's message of lun nov 22 18:59:52 -0300 2010: > Hmm, the first thought that comes to mind is that the GucContext param > to ParseConfigFile is unused and can be removed. This is probably an > oversight from when include files were introduced in 2006. Committed and pushed. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of lun nov 22 18:12:39 -0300 2010: > Itagaki Takahiro writes: > > No. My suggestion was just to use the internal parser. > > What about something like the attached, cfparser.v3.patch? > > If that looks ok, do we want to add some documentation about the new > lexer capabilities? Also, for what good reason would we want to prevent > people from using the include facility? Hmm, the first thought that comes to mind is that the GucContext param to ParseConfigFile is unused and can be removed. This is probably an oversight from when include files were introduced in 2006. I don't like the fact that this code handles custom_variable_classes internally. I think this would be exposed to the parsing of extension control files, which is obviously wrong. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Itagaki Takahiro writes: > No. My suggestion was just to use the internal parser. What about something like the attached, cfparser.v3.patch? If that looks ok, do we want to add some documentation about the new lexer capabilities? Also, for what good reason would we want to prevent people from using the include facility? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/src/backend/access/transam/xlog.c --- b/src/backend/access/transam/xlog.c *** *** 5024,5200 str_time(pg_time_t tnow) } /* - * Parse one line from recovery.conf. 'cmdline' is the raw line from the - * file. If the line is parsed successfully, returns true, false indicates - * syntax error. On success, *key_p and *value_p are set to the parameter - * name and value on the line, respectively. If the line is an empty line, - * consisting entirely of whitespace and comments, function returns true - * and *keyp_p and *value_p are set to NULL. - * - * The pointers returned in *key_p and *value_p point to an internal buffer - * that is valid only until the next call of parseRecoveryCommandFile(). - */ - static bool - parseRecoveryCommandFileLine(char *cmdline, char **key_p, char **value_p) - { - char *ptr; - char *bufp; - char *key; - char *value; - static char *buf = NULL; - - *key_p = *value_p = NULL; - - /* - * Allocate the buffer on first use. It's used to hold both the parameter - * name and value. - */ - if (buf == NULL) - buf = malloc(MAXPGPATH + 1); - bufp = buf; - - /* Skip any whitespace at the beginning of line */ - for (ptr = cmdline; *ptr; ptr++) - { - if (!isspace((unsigned char) *ptr)) - break; - } - /* Ignore empty lines */ - if (*ptr == '\0' || *ptr == '#') - return true; - - /* Read the parameter name */ - key = bufp; - while (*ptr && !isspace((unsigned char) *ptr) && - *ptr != '=' && *ptr != '\'') - *(bufp++) = *(ptr++); - *(bufp++) = '\0'; - - /* Skip to the beginning quote of the parameter value */ - ptr = strchr(ptr, '\''); - if (!ptr) - return false; - ptr++; - - /* Read the parameter value to *bufp. Collapse any '' escapes as we go. */ - value = bufp; - for (;;) - { - if (*ptr == '\'') - { - ptr++; - if (*ptr == '\'') - *(bufp++) = '\''; - else - { - /* end of parameter */ - *bufp = '\0'; - break; - } - } - else if (*ptr == '\0') - return false; /* unterminated quoted string */ - else - *(bufp++) = *ptr; - - ptr++; - } - *(bufp++) = '\0'; - - /* Check that there's no garbage after the value */ - while (*ptr) - { - if (*ptr == '#') - break; - if (!isspace((unsigned char) *ptr)) - return false; - ptr++; - } - - /* Success! */ - *key_p = key; - *value_p = value; - return true; - } - - /* * See if there is a recovery command file (recovery.conf), and if so * read in parameters for archive recovery and XLOG streaming. * ! * XXX longer term intention is to expand this to ! * cater for additional parameters and controls ! * possibly use a flex lexer similar to the GUC one */ static void readRecoveryCommandFile(void) { FILE *fd; - char cmdline[MAXPGPATH]; TimeLineID rtli = 0; bool rtliGiven = false; ! bool syntaxError = false; fd = AllocateFile(RECOVERY_COMMAND_FILE, "r"); ! if (fd == NULL) ! { ! if (errno == ENOENT) ! return;/* not there, so no archive recovery */ ! ereport(FATAL, ! (errcode_for_file_access(), ! errmsg("could not open recovery command file \"%s\": %m", ! RECOVERY_COMMAND_FILE))); ! } /* ! * Parse the file... */ ! while (fgets(cmdline, sizeof(cmdline), fd) != NULL) ! { ! char *tok1; ! char *tok2; ! ! if (!parseRecoveryCommandFileLine(cmdline, &tok1, &tok2)) ! { ! syntaxError = true; ! break; ! } ! if (tok1 == NULL) ! continue; ! if (strcmp(tok1, "restore_command") == 0) { ! recoveryRestoreCommand = pstrdup(tok2); ereport(DEBUG2, (errmsg("restore_command = '%s'", recoveryRestoreCommand))); } ! else if (strcmp(tok1, "recovery_end_command") == 0) { ! recoveryEndCommand = pstrdup(tok2); ereport(DEBUG2, (errmsg("recovery_end_command = '%s'", recoveryEndCommand))); } ! else if (strcmp(tok1, "archive_cleanup_command") == 0) { ! archiveCleanupCommand = pstrdup(tok2); ereport(DEBUG2, (errmsg("archive_cleanup_command = '%s'", archiveCleanupCommand))); } ! else if (strcmp(tok1, "recovery_target_timeline") == 0) { rtliGiven = true; ! if (strcmp(tok2, "latest") == 0) rtli = 0; else { errno = 0; ! rtli = (TimeLineID) strtoul(tok2, NULL, 0); if (errno == EINVAL || errno == ERANGE) ereport(FATAL, (errmsg("recovery_target_timeline is not a valid number: \"%s\"", ! tok2))); } if (rtli) ereport(DEB
Re: [HACKERS] Extensions, this time with a patch
On Mon, Nov 22, 2010 at 18:36, Dimitri Fontaine wrote: >> * Can we export ParseConfigFile() in guc-file.l rather than >> parseRecoveryCommandFileLine()? > > Should we then consider recovery.conf entries as ordinary GUCs? That > would allow to connect to a standby and issue 'show primary_conninfo' > there. This has been asked before, already. No. My suggestion was just to use the internal parser. ParseConfigFile() returns parsed parameters as head_p and tail_p. So, we can use it independently from GUC variables. static bool ParseConfigFile(const char *config_file, const char *calling_file, int depth, GucContext context, int elevel, struct name_value_pair **head_p, struct name_value_pair **tail_p) Special codes for "include" and "custom_variable_classes" in it might not be needed to parse recovery.conf and extensions. We would disable them when we parse non-guc configuration files. Or, we could allow "include" in recovery.conf if we think it is useful in some cases. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Itagaki Takahiro writes: > I checked cfparser.v2.patch. Thanks for reviewing it! > It exports the static parseRecoveryCommandFileLine() in xlog.c > as the global cfParseOneLine() in cfparser.c without modification. > > It generates one warning, but it can be easily fixed. > cfparser.c:34: warning: no previous prototype for 'cfParseOneLine' Mmmm, that must have been a cherry-picking error on my side, it seems I forgot to include this patch in the cfparser branch. Sorry about that. http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=e6b4c670a0189ee8a799521af06c1ee63f9e530e > Some discussions about the patch: > > * Is "cf" the best name for the prefix? Less abbreviated forms might > be less confusable. Personally, I prefer "conf". Will change accordingly if that's the choice. > * Can we export ParseConfigFile() in guc-file.l rather than > parseRecoveryCommandFileLine()? It can solve the issue that unquoted > parameter values in recovery.conf are not recognized. Even if we > won't merge them, just allowing unquoted values would be useful. Should we then consider recovery.conf entries as ordinary GUCs? That would allow to connect to a standby and issue 'show primary_conninfo' there. This has been asked before, already. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Sun, Nov 21, 2010 at 8:10 PM, Itagaki Takahiro wrote: > On Wed, Oct 20, 2010 at 01:36, Dimitri Fontaine > wrote: >> Ah yes, thinking it's an easy patch is not helping. Please find attached >> a revised version of it. > > I checked cfparser.v2.patch. > > It exports the static parseRecoveryCommandFileLine() in xlog.c > as the global cfParseOneLine() in cfparser.c without modification. > > It generates one warning, but it can be easily fixed. > cfparser.c:34: warning: no previous prototype for 'cfParseOneLine' > > Some discussions about the patch: > > * Is "cf" the best name for the prefix? Less abbreviated forms might > be less confusable. Personally, I prefer "conf". > > * Can we export ParseConfigFile() in guc-file.l rather than > parseRecoveryCommandFileLine()? It can solve the issue that unquoted > parameter values in recovery.conf are not recognized. Even if we > won't merge them, just allowing unquoted values would be useful. I'd really like to see postgresql.conf and recovery.conf parsing merged, and I suspect, as Itagaki-san says, that postgresql.conf parsing is the better model for any new code. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Wed, Oct 20, 2010 at 01:36, Dimitri Fontaine wrote: > Ah yes, thinking it's an easy patch is not helping. Please find attached > a revised version of it. I checked cfparser.v2.patch. It exports the static parseRecoveryCommandFileLine() in xlog.c as the global cfParseOneLine() in cfparser.c without modification. It generates one warning, but it can be easily fixed. cfparser.c:34: warning: no previous prototype for 'cfParseOneLine' Some discussions about the patch: * Is "cf" the best name for the prefix? Less abbreviated forms might be less confusable. Personally, I prefer "conf". * Can we export ParseConfigFile() in guc-file.l rather than parseRecoveryCommandFileLine()? It can solve the issue that unquoted parameter values in recovery.conf are not recognized. Even if we won't merge them, just allowing unquoted values would be useful. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Le 25 oct. 2010 à 17:26, Alvaro Herrera a écrit : > Ah, some reading of the patch reveals that the "script" defaults to the > control file name, but it can be overridden. Yes, that's new in v10. In v11 I've kept that and removed the name property in the control file, so that we have: cat contrib/intarray/intarray.control.in # intarray comment = 'one-dimensional arrays of integers: functions, operators, index support' version = 'EXTVERSION' script = '_int.sql' > I noticed that you're using ExclusiveLock when creating an extension, > citing the handling of the global variable create_extension for this. > There are two problems here: one is that you're releasing the lock way > too early: if you wanted this to be effective, you'd need to hold on to > the lock until after you've registered the extension. > > The other is that there is no need for this at all, because this backend > cannot be concurrently running another CREATE EXTENSION comand, and > this is a backend-local variable. So there's no point. I wanted to protect from another backend trying to create the same extension at the same time. So the critical path is the inserting into the catalog. I now see I failed to include the duplicate object check into the critical path, when I added that later. Do you confirm protecting the insertion in the catalog is not worthy of special locking? To get proper locking requires some more thinking than I did put in, but if you say I'd better remove it... > Why palloc create_extension every time? Isn't it better to initialize > it properly and have a boolean value telling whether it's to be used? > Also, if an extension fails partway through creation, the var will be > left set. I think you need a PG_TRY block to reset it. Good catches. I'm still uneasy with which memory context what allocation belongs too, hence the palloc()ing here. > (I find the repeated coding pattern that tests create_extension for > NULL-ness before calling recordDependencyOn a bit awkward; maybe hide it > in a function or macro? But then maybe that's just me. Also, why > palloc it? Seems better to have it static. Notice your new calls to > recordDependencyOn are the only ones with operands not using the & > operator.) In fact the goal of the test is to check if we're in the code path for CREATE EXTENSION, rather than pointer validity per-say. I'll go have it static, too, with a bool to determine the code path. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support PS: I hope to be able to send this email, but uploading the git repo will be uneasy from the wifi here at best. Will send patches if email is ok. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Le 25 oct. 2010 à 17:26, Alvaro Herrera a écrit : > Ah, some reading of the patch reveals that the "script" defaults to the > control file name, but it can be overridden. Yes, that's new in v10. In v11 I've kept that and removed the name property in the control file, so that we have: cat contrib/intarray/intarray.control.in # intarray comment = 'one-dimensional arrays of integers: functions, operators, index support' version = 'EXTVERSION' script = '_int.sql' > I noticed that you're using ExclusiveLock when creating an extension, > citing the handling of the global variable create_extension for this. > There are two problems here: one is that you're releasing the lock way > too early: if you wanted this to be effective, you'd need to hold on to > the lock until after you've registered the extension. > > The other is that there is no need for this at all, because this backend > cannot be concurrently running another CREATE EXTENSION comand, and > this is a backend-local variable. So there's no point. I wanted to protect from another backend trying to create the same extension at the same time. So the critical path is the inserting into the catalog. I now see I failed to include the duplicate object check into the critical path, when I added that later. Do you confirm protecting the insertion in the catalog is not worthy of special locking? To get proper locking requires some more thinking than I did put in, but if you say I'd better remove it... > Why palloc create_extension every time? Isn't it better to initialize > it properly and have a boolean value telling whether it's to be used? > Also, if an extension fails partway through creation, the var will be > left set. I think you need a PG_TRY block to reset it. Good catches. I'm still uneasy with which memory context what allocation belongs too, hence the palloc()ing here. > (I find the repeated coding pattern that tests create_extension for > NULL-ness before calling recordDependencyOn a bit awkward; maybe hide it > in a function or macro? But then maybe that's just me. Also, why > palloc it? Seems better to have it static. Notice your new calls to > recordDependencyOn are the only ones with operands not using the & > operator.) In fact the goal of the test is to check if we're in the code path for CREATE EXTENSION, rather than pointer validity per-say. I'll go have it static, too, with a bool to determine the code path. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support PS: I hope to be able to send this email, but uploading the git repo will be uneasy from the wifi here at best. Will send patches if email is ok. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of vie oct 22 16:43:56 -0300 2010: > Dimitri Fontaine writes: > > For information, when we talk about performance problem, please note > > that on my workstation with a default setup (not that it's important > > here) we're talking about 86,420 ms for a loop of 100 > > perform * from pg_extensions; BTW it strikes me that it would be easier on the code that there were just a couple of simple functions, one returning the list of installed extensions and another one returning the list of installable extensions. The rest of SRF functions needn't be implemented in C, you could implement them in SQL instead by joining to pg_depend and whatnot. Also, PFA a couple of minor fixes. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support 0001-A-bunch-of-minor-fixes.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Alvaro Herrera's message of lun oct 25 10:37:22 -0300 2010: > Excerpts from Alvaro Herrera's message of vie oct 22 17:02:22 -0300 2010: > > > > I'll go rework the patch sometime later to drop the name from the > > > control file, but that also means fixing several contrib modules by > > > changing their file names, operation for which the project has no policy > > > yet as far as I understand (we used CVS before). > > > > Change what file names? You lost me there. I thought the extension > > name was going to be equal to the control file name, and said control > > file doesn't exist yet, so you don't need to rename any existant file. > > Am I confusing something? > > Hmm, after reading the latest blog post, it seems that the patch > requires that the control file is equal to the .sql install script. Is > this the case? I don't see a reason for this requirement; why not > simply have a line for the install script in the control file? That > way, you don't need to rename the .sql files. Ah, some reading of the patch reveals that the "script" defaults to the control file name, but it can be overridden. I noticed that you're using ExclusiveLock when creating an extension, citing the handling of the global variable create_extension for this. There are two problems here: one is that you're releasing the lock way too early: if you wanted this to be effective, you'd need to hold on to the lock until after you've registered the extension. The other is that there is no need for this at all, because this backend cannot be concurrently running another CREATE EXTENSION comand, and this is a backend-local variable. So there's no point. Why palloc create_extension every time? Isn't it better to initialize it properly and have a boolean value telling whether it's to be used? Also, if an extension fails partway through creation, the var will be left set. I think you need a PG_TRY block to reset it. (I find the repeated coding pattern that tests create_extension for NULL-ness before calling recordDependencyOn a bit awkward; maybe hide it in a function or macro? But then maybe that's just me. Also, why palloc it? Seems better to have it static. Notice your new calls to recordDependencyOn are the only ones with operands not using the & operator.) -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Alvaro Herrera's message of vie oct 22 17:02:22 -0300 2010: > Excerpts from Dimitri Fontaine's message of vie oct 22 16:21:14 -0300 2010: > > I'll go rework the patch sometime later to drop the name from the > > control file, but that also means fixing several contrib modules by > > changing their file names, operation for which the project has no policy > > yet as far as I understand (we used CVS before). > > Change what file names? You lost me there. I thought the extension > name was going to be equal to the control file name, and said control > file doesn't exist yet, so you don't need to rename any existant file. > Am I confusing something? Hmm, after reading the latest blog post, it seems that the patch requires that the control file is equal to the .sql install script. Is this the case? I don't see a reason for this requirement; why not simply have a line for the install script in the control file? That way, you don't need to rename the .sql files. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Itagaki Takahiro writes: > On Sat, Oct 23, 2010 at 5:26 AM, Josh Berkus wrote: >>> Yeah - what is the feasibility of cleaning up the things where there >>> are naming inconsistencies right now? >> >> Easy. Â Heck, the only reason we didn't do it 2 years ago was that we >> were waiting for extensions before bothering. > We could rename the module name, directory, and documentation path, > but could not rename .so files because pg_restore would fail to restore > functions written in C because they contains previous name of .so files. > The issue will be solved by the EXTENSION patch, but the feature cannot > be used to upgrade to 9.1 from older versions, no? Hmm, there seems to be some confusion here as to whether we are talking about "move the source code around" or "change user-visible module names". I'm distinctly not for the latter, but I'm not sure if that's what Josh meant. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Sat, Oct 23, 2010 at 5:26 AM, Josh Berkus wrote: >> Yeah - what is the feasibility of cleaning up the things where there >> are naming inconsistencies right now? > > Easy. Heck, the only reason we didn't do it 2 years ago was that we > were waiting for extensions before bothering. We could rename the module name, directory, and documentation path, but could not rename .so files because pg_restore would fail to restore functions written in C because they contains previous name of .so files. The issue will be solved by the EXTENSION patch, but the feature cannot be used to upgrade to 9.1 from older versions, no? -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: > Anyway, my point is that by default there's no directory scanning: the > lookup is first directed towards ${extension-name}.control, of > course. Only when this file does not exists or its name property is > different from the extension name do we get to scan the directory, and > we stop as soon as we find the .control file with the right name in it. This seems (a) way overcomplicated, and (b) sorely in need of a unique-index mechanism. Please just use the filenames and have done. This is lily-gilding at its finest. People who want to use non-ascii filenames are welcome to do so, and deal with any ensuing fallout themselves. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
> Yeah - what is the feasibility of cleaning up the things where there > are naming inconsistencies right now? Easy. Heck, the only reason we didn't do it 2 years ago was that we were waiting for extensions before bothering. Go Dimitri! Yaaay. -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Robert Haas's message of vie oct 22 17:07:10 -0300 2010: > On Fri, Oct 22, 2010 at 4:02 PM, Alvaro Herrera > wrote: > > That said, I don't think there's any reason now to stop a committer from > > renaming files (as long as this has been previously discussed and agreed > > to, just like any other commit). > > Yeah - what is the feasibility of cleaning up the things where there > are naming inconsistencies right now? We're in Git-land now. Why should we worry unnecessarily about old restrictions? It wasn't a policy, just a tool limitation. Do you have concrete proposals? If so please start a new thread :-) -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Fri, Oct 22, 2010 at 4:02 PM, Alvaro Herrera wrote: > That said, I don't think there's any reason now to stop a committer from > renaming files (as long as this has been previously discussed and agreed > to, just like any other commit). Yeah - what is the feasibility of cleaning up the things where there are naming inconsistencies right now? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of vie oct 22 16:21:14 -0300 2010: > So I think it's a good compromise, and as it's superuser-only I would > think it's acceptable as-is. Apparently it's not, which ain't the end of > the world but an unexpected surprise for me. And when I don't > understand, I tend to insist until I do or until I resign, whichever > comes first, but you would know that by now :) Yeah, well, this is just my personal opinion. Others are welcome to chime in. > I'll go rework the patch sometime later to drop the name from the > control file, but that also means fixing several contrib modules by > changing their file names, operation for which the project has no policy > yet as far as I understand (we used CVS before). Change what file names? You lost me there. I thought the extension name was going to be equal to the control file name, and said control file doesn't exist yet, so you don't need to rename any existant file. Am I confusing something? That said, I don't think there's any reason now to stop a committer from renaming files (as long as this has been previously discussed and agreed to, just like any other commit). > Superuser only. To manage extensions, nothing to do with live load or > user queries. But if you say performance is important here and 3ms to > display all available extensions sucks, so be it. To be honest I'm not all that concerned with raw performance here. I just think that scanning a directory is a strange way for the search to behave. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: > For information, when we talk about performance problem, please note > that on my workstation with a default setup (not that it's important > here) we're talking about 86,420 ms for a loop of 100 > perform * from pg_extensions; That's right, but > That displays 36 extensions and needs to parse their files twice and for > some of them need to scan the directory and parse other extension > control files before to get to the right one. Average less than 1ms to > do all that on my workstation, and typically less than 3ms if you > include psql side of things. That's not what happens, the pg_extensions() SRF will scan the directory once and parse each control file once, of course. I'm tired enough to mix the behaviour of finding the control file given *one* extension name at CREATE EXTENSION time with listing all available extensions. Sorry for the noise. In my mind though, the baseline remains the same. Now I will have a sleep and prepare for holidays, in some meaningful order… Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: > Well, things like pgfoundry project names are also restricted AFAICT and > I don't think anyone has a problem with that. For things you publish, sure. But extensions will also handle in-house developments of functions and other in-database API-like stuff, in fact any SQL script you have that you don't want to maintain in the database dumps is an extension. So it's broader than just public Open Source projects. Anyway, my point is that by default there's no directory scanning: the lookup is first directed towards ${extension-name}.control, of course. Only when this file does not exists or its name property is different from the extension name do we get to scan the directory, and we stop as soon as we find the .control file with the right name in it. So I think it's a good compromise, and as it's superuser-only I would think it's acceptable as-is. Apparently it's not, which ain't the end of the world but an unexpected surprise for me. And when I don't understand, I tend to insist until I do or until I resign, whichever comes first, but you would know that by now :) I'll go rework the patch sometime later to drop the name from the control file, but that also means fixing several contrib modules by changing their file names, operation for which the project has no policy yet as far as I understand (we used CVS before). For information, when we talk about performance problem, please note that on my workstation with a default setup (not that it's important here) we're talking about 86,420 ms for a loop of 100 perform * from pg_extensions; That displays 36 extensions and needs to parse their files twice and for some of them need to scan the directory and parse other extension control files before to get to the right one. Average less than 1ms to do all that on my workstation, and typically less than 3ms if you include psql side of things. Superuser only. To manage extensions, nothing to do with live load or user queries. But if you say performance is important here and 3ms to display all available extensions sucks, so be it. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of vie oct 22 13:30:22 -0300 2010: > So extension names are forced into English? I would live with that, I > just don't find the answer friendly to the users. Well, things like pgfoundry project names are also restricted AFAICT and I don't think anyone has a problem with that. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Andrew Dunstan writes: > ASCII is not the same thing as English. You can create the names in Pig > Latin or Redneck if you want. It's the charset that's being restricted here, > and we restrict many more things than this to ASCII. I'd like to read Itagaki's opinion here, will wait :) -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On 10/22/2010 12:30 PM, Dimitri Fontaine wrote: Alvaro Herrera writes: Excerpts from Dimitri Fontaine's message of vie oct 22 12:25:07 -0300 2010: Now, if we want to do it the other way round and force extension name to be the filename, we will have to live with all the restrictions that filename imposes and that are not the same depending on the OS and the filesystem, I think, and with systems where we have no way to know what is the filesystem encoding. Am I wring in thinking that this might be a problem? I don't see a problem limiting extension names to use only ASCII chars, and a subset of those, at that. They're just names. If you want to get fancy you can use the description. So extension names are forced into English? I would live with that, I just don't find the answer friendly to the users. ASCII is not the same thing as English. You can create the names in Pig Latin or Redneck if you want. It's the charset that's being restricted here, and we restrict many more things than this to ASCII. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: > Excerpts from Dimitri Fontaine's message of vie oct 22 12:25:07 -0300 2010: > >> Now, if we want to do it the other way round and force extension name to >> be the filename, we will have to live with all the restrictions that >> filename imposes and that are not the same depending on the OS and the >> filesystem, I think, and with systems where we have no way to know what >> is the filesystem encoding. Am I wring in thinking that this might be a >> problem? > > I don't see a problem limiting extension names to use only ASCII chars, > and a subset of those, at that. They're just names. If you want to get > fancy you can use the description. So extension names are forced into English? I would live with that, I just don't find the answer friendly to the users. I'd think we can live with some directory scanning that only happens when installing extensions, or checking available extensions. All of which being superuser only. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Alvaro Herrera's message of vie oct 22 13:17:41 -0300 2010: > > Given that the control file now supports an encoding parameter, I don't > > see what this is good for, but I might be missing something obvious for > > people working with different encodings etc. Japan seems to be quite > > special as far as encodings are concerned. > > Seems we're both arging the say way, but neither of us is Japanese ;-) Argh. "arguing the same way". Blame it on typing on a high-latency link :-( -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of vie oct 22 12:25:07 -0300 2010: > Now, if we want to do it the other way round and force extension name to > be the filename, we will have to live with all the restrictions that > filename imposes and that are not the same depending on the OS and the > filesystem, I think, and with systems where we have no way to know what > is the filesystem encoding. Am I wring in thinking that this might be a > problem? I don't see a problem limiting extension names to use only ASCII chars, and a subset of those, at that. They're just names. If you want to get fancy you can use the description. > If that's not a problem and we want to have the extension name into the > control file name, then I'll go remove the name property there. > > > It seems more sensible to allow the variation to occur in the Makefile, > > i.e. _int/Makefile should contain > > EXTENSION=intarray > > > > Was this discussed previously? If so, why was this idea rejected? > > It's already possible to do this. But the contrib uses _int.sql.in and > MODULE_big = _int and I didn't change that. I had the control file named > the same as the script file, but with the current implementation we can > change that (there's a script property in the control file). I think it makes more sense to change the script name in the control file. That way you never need to scan anything. As for this being seldom used, consider the case where the user mistypes the extension name. You will be scanning the directory without good reason. > >> > * pg_execute_from_file() and encoding > >> > It expects the file is in server encoding, but it is not always true > >> > because we support multiple encodings in the same installation. > >> > How about adding encoding parameter to the function? > >> > => pg_execute_from_file( file, encoding ) > > > > This seems sensible ... at least as sensible as it is to allow COPY to > > specify the encoding. > > Well why not, for convenience, but you can surround the call to this > function with SET client_encoding and get the same behaviour. Will add > the argument if required, though. I don't think it is. In fact, it seems better to leave it out. CREATE EXTENSION will have its own mechanism for specifying the encoding (which it'll obtain from the control file), so I don't see the need. > >> > CREATE EXTENSION could have optional ENCODING option. > >> > => CREATE EXTENSION name [ ENCODING 'which' ] > > > > This seems like putting the burden on the user on getting the command > > right. That seems prone to failure. > > Given that the control file now supports an encoding parameter, I don't > see what this is good for, but I might be missing something obvious for > people working with different encodings etc. Japan seems to be quite > special as far as encodings are concerned. Seems we're both arging the say way, but neither of us is Japanese ;-) -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > Yes. It's horrid for performance, and it's worse for understandability, > and there's no obvious benefit to set against those. Please let's make > the rule that the control file name equals the extension name. Performance… well if you say that CREATE EXTENSION performance prevails on its flexibility, let's say that I didn't expect it. See also my previous mail for details (directory scanning only happens for those extensions having chosen not to name their control file sensibly) and the filesystem/OS problems (are they real?). > But then you have to figure out what encoding it is, and you have to > know that before you start reading the file. I think a better idea > would be to do the same thing we did for text search config files: > mandate that these files have to be in utf8. Agreed, if as I understand it you're talking about the control file itself, which supports an 'encoding' parameter that applies to the SQL script. >> I think it is OK to have the control files be pure ASCII (this doesn't >> mean they are in SQL_ASCII though). The problems will start when we >> decide to allow translations for extension descriptions. But we can >> leave that for another day. > > Well, we can see the issue now, and anyway who's to say an extension > might not want to load up some non-ASCII data? The case is covered with the 'script' and 'encoding' parameters present in v10 I think. If we force the control file itself to be UTF8 then we can allow translations embedded in the file, it seems. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: >> Fixed by having the CREATE EXTENSION command consider it's being given >> the name of the extension as found in the control file, rather than the >> control file base name. > > Hang on. Did I miss something? Why doesn't the control file name equal > the extension name? I think the idea that you have to scan the whole > share directory and parse all control files to find the one you need is > a bit strange. The default behavior is to have the control file name the same as the extension's file name, so there's no directory scanning in this case. It so happen that the existing "extensions" are not currently following the rule, even when we limit ourselves to contrib, so I adjusted the code to be able to be more friendly. Performance wise, it only concerns the command CREATE EXTENSION and the queries to the pg_extensions system view, that's used in \dx. Does not seem that critical. Now, if we want to do it the other way round and force extension name to be the filename, we will have to live with all the restrictions that filename imposes and that are not the same depending on the OS and the filesystem, I think, and with systems where we have no way to know what is the filesystem encoding. Am I wring in thinking that this might be a problem? If that's not a problem and we want to have the extension name into the control file name, then I'll go remove the name property there. > It seems more sensible to allow the variation to occur in the Makefile, > i.e. _int/Makefile should contain > EXTENSION=intarray > > Was this discussed previously? If so, why was this idea rejected? It's already possible to do this. But the contrib uses _int.sql.in and MODULE_big = _int and I didn't change that. I had the control file named the same as the script file, but with the current implementation we can change that (there's a script property in the control file). >> > * pg_execute_from_file() and encoding >> > It expects the file is in server encoding, but it is not always true >> > because we support multiple encodings in the same installation. >> > How about adding encoding parameter to the function? >> > => pg_execute_from_file( file, encoding ) > > This seems sensible ... at least as sensible as it is to allow COPY to > specify the encoding. Well why not, for convenience, but you can surround the call to this function with SET client_encoding and get the same behaviour. Will add the argument if required, though. >> > CREATE EXTENSION could have optional ENCODING option. >> > => CREATE EXTENSION name [ ENCODING 'which' ] > > This seems like putting the burden on the user on getting the command > right. That seems prone to failure. Given that the control file now supports an encoding parameter, I don't see what this is good for, but I might be missing something obvious for people working with different encodings etc. Japan seems to be quite special as far as encodings are concerned. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: > Hang on. Did I miss something? Why doesn't the control file name equal > the extension name? I think the idea that you have to scan the whole > share directory and parse all control files to find the one you need is > a bit strange. Yes. It's horrid for performance, and it's worse for understandability, and there's no obvious benefit to set against those. Please let's make the rule that the control file name equals the extension name. >> It expects the file is in server encoding, but it is not always true >> because we support multiple encodings in the same installation. >> How about adding encoding parameter to the function? >> => pg_execute_from_file( file, encoding ) > This seems sensible ... at least as sensible as it is to allow COPY to > specify the encoding. But then you have to figure out what encoding it is, and you have to know that before you start reading the file. I think a better idea would be to do the same thing we did for text search config files: mandate that these files have to be in utf8. > I think it is OK to have the control files be pure ASCII (this doesn't > mean they are in SQL_ASCII though). The problems will start when we > decide to allow translations for extension descriptions. But we can > leave that for another day. Well, we can see the issue now, and anyway who's to say an extension might not want to load up some non-ASCII data? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of vie oct 22 10:43:58 -0300 2010: > Itagaki Takahiro writes: > > * There are some inconsistency between extension names in \dx+ view > > and actual name used by CREATE EXTENSION. > > - auto_username => insert_username > > - intarray => _int > > - xml2 => pgxml > > We might need to rename them, or add 'installer'/'uninstaller' entries > > into control files to support different extension names from .so name. > > Fixed by having the CREATE EXTENSION command consider it's being given > the name of the extension as found in the control file, rather than the > control file base name. Hang on. Did I miss something? Why doesn't the control file name equal the extension name? I think the idea that you have to scan the whole share directory and parse all control files to find the one you need is a bit strange. It seems more sensible to allow the variation to occur in the Makefile, i.e. _int/Makefile should contain EXTENSION=intarray Was this discussed previously? If so, why was this idea rejected? > > * pg_execute_from_file() and encoding > > It expects the file is in server encoding, but it is not always true > > because we support multiple encodings in the same installation. > > How about adding encoding parameter to the function? > > => pg_execute_from_file( file, encoding ) This seems sensible ... at least as sensible as it is to allow COPY to specify the encoding. > > CREATE EXTENSION could have optional ENCODING option. > > => CREATE EXTENSION name [ ENCODING 'which' ] This seems like putting the burden on the user on getting the command right. That seems prone to failure. > So after adding support for this option in the command, I realized it > might be useless. What I've done is implemented an 'encoding' parameter > in the control file, so that the extension's author / maintainer are > able to set that from there. That makes more sense. > As I began by implementing the syntax, I didn't remove it yet, and maybe > there's a use case for it. Some strange encoding setting might require > the DBA to override what the author thinks the script encoding is? I doubt this is necessary. If the DBA needs to override it (why?), it's possible to edit the file. > I didn't (yet) specified any encoding for the contrib control files, as > I'm not sure lots of thoughts have been given to them. Should I set > SQL_ASCII, following what file(1) tells me, or do we aim for another > encoding here, or is it useless (as I think) to set SQL_ASCII when the > default is the database encoding? I think it is OK to have the control files be pure ASCII (this doesn't mean they are in SQL_ASCII though). The problems will start when we decide to allow translations for extension descriptions. But we can leave that for another day. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Itagaki Takahiro writes: > BTW, did you register your patch to the next commitfest? > It would be better to do so for tracking the activities. > https://commitfest.postgresql.org/action/commitfest_view?id=8 https://commitfest.postgresql.org/action/patch_view?id=404 -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: > That's done now and for those paying attention, of course those examples > won't need to add a script property in their control files, as soon as > we both scan the SHAREDIR to find the proper control file and that the > default script is ${name}.sql, which is what's used everywhere in our > contribs. Ok, that's confusing. The ${name} above is the name sans extension of the control file (`basename foo.control .control`), not the name of the extension. That's why it works without having to override the script property in the contribs control files: _int.control and _int.sql are the files for the extension named 'intarray'. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: > I lean toward adding support for a script variable into the control file > which defaults to script = '${name}.sql' and will have to be edited only > in those 3 cases you're reporting about. I'm going to work on that this > morning, it looks simple enough to get reworked if necessary. > > (yes it means we have to scan all control files to find the one where > the name property is the one given in the CREATE EXTENSION command, > but the code already exists --- it still has to be refactored) That's done now and for those paying attention, of course those examples won't need to add a script property in their control files, as soon as we both scan the SHAREDIR to find the proper control file and that the default script is ${name}.sql, which is what's used everywhere in our contribs. New patch to follow later, including the other modifications on the table (error message, script file encoding, etc). Note that the control files being parsed to check their name property against the extension name that's been asked by the user, I think that means they have to be in a fixed known encoding. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Fri, Oct 22, 2010 at 1:31 AM, Dimitri Fontaine wrote: > Of course, you what that means? Yes, another version of the patch, that > will build the control file out of the control.in at build time rather > than install time, and that's back to using EXTVERSION both in the > Makefile and in the .control.in file. Here are detailed report for v9 patch. * extension.v9.patch.gz seems to contain other changes in the repo. Did you use old master to get the diff? * Typo in doc/xfunc.sgml. They are to be "replaceable" probably. openjade:xfunc.sgml:2510:32:E: element "REPLACABLE" undefined openjade:xfunc.sgml:2523:46:E: element "REPLACABLE" undefined * There are some inconsistency between extension names in \dx+ view and actual name used by CREATE EXTENSION. - auto_username => insert_username - intarray => _int - xml2 => pgxml We might need to rename them, or add 'installer'/'uninstaller' entries into control files to support different extension names from .so name. * pg_execute_from_file() and encoding It expects the file is in server encoding, but it is not always true because we support multiple encodings in the same installation. How about adding encoding parameter to the function? => pg_execute_from_file( file, encoding ) CREATE EXTENSION could have optional ENCODING option. => CREATE EXTENSION name [ ENCODING 'which' ] I strongly hope the multi-encoding support for my Japanese textsearch extension. Comments in the extension is written in UTF-8, but both UTF-8 and EUC_JP are equally used for database encodings. * Error messages in pg_execute_from_file() - "must be superuser to get file information" would be "must be superuser to execute file" . - "File '%s' could not be executed" would be "could not execute file: '%s'". Our message style guide is here: http://www.postgresql.org/docs/9.0/static/error-style-guide.html Many messages in extension.c are also to be adjusted. commands/extension.c needs to be cleaned up a bit more: * fsize in read_extension_control_file() is not used. * ferror() test just after AllocateFile() is not needed; NULL checking is enough. * malloc() in add_extension_custom_variable_classes(). I think the README says nothing about malloc() except assign_hook cases; palloc would be better here. BTW, did you register your patch to the next commitfest? It would be better to do so for tracking the activities. https://commitfest.postgresql.org/action/commitfest_view?id=8 -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Thu, Oct 21, 2010 at 11:12 AM, Dimitri Fontaine wrote: > "David E. Wheeler" writes: >> Sure. The reason to do it, though, is so that extension authors can create >> just one metadata file, instead of two (or three, if one must also put such >> data into the Makefile). > > That's a good idea, but my guess is that the implementation cost of > supporting the control format in your perl infrastructure is at least an > order of magnitude lower than the cost for me to support your current > JSON file format, so I lean towards you having an automated way to fill > in the json file from the control one... Ah, truth to power. :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of jue oct 21 12:53:18 -0300 2010: > This part of the problem didn't receive much thoughts yet, and it shows > up. About the rest of the patch have been in my head for months, I > expect less problems there... Keep on it. You're doing a terrific job. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
"David E. Wheeler" writes: > Be aware that PGXS sets a $(VERSION) variable already, so you'll need > to use another name, I think, to guard from conflicts. This is in > Makefile.global: Of course that's not a problem for contribs, and I used EXTVERSION in a previous version of the patch. I guess I will get back to use $(EXTVERSION) in the Makefile next time I have to produce a patch. This part of the problem didn't receive much thoughts yet, and it shows up. About the rest of the patch have been in my head for months, I expect less problems there... Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Oct 21, 2010, at 8:12 AM, Dimitri Fontaine wrote: > That's a good idea, but my guess is that the implementation cost of > supporting the control format in your perl infrastructure is at least an > order of magnitude lower than the cost for me to support your current > JSON file format, so I lean towards you having an automated way to fill > in the json file from the control one... Well, it *will* be easier. Eventually. Right now, the file has to be edited by hand. Which I can tell you from experience is rather…error-prone. Anyway, I wouldn't push for a JSON file format until a parser was just there for you to use without too much trouble. > The Makefile supports $(VERSION) because chances are it's already there > (think packaging or tarball release targets). Having yet another place > where to manually maintain a version number ain't appealing. Be aware that PGXS sets a $(VERSION) variable already, so you'll need to use another name, I think, to guard from conflicts. This is in Makefile.global: VERSION = 9.0.1 MAJORVERSION = 9.0 Maybe use EXTVERSION? You don't want to overwrite the core version because a makefile author could use it to change the build (pgTAP does this, for example). > In the latest patch, though, the only other thing you find in the > Makefile about the extension is its basename, which must be the one of > both the .control and the .sql files. And it's possible for $(EXTENSION) > to be a list of them, too, because of contrib/spi. Right, that makes sense. Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
"David E. Wheeler" writes: > Sure. The reason to do it, though, is so that extension authors can create > just one metadata file, instead of two (or three, if one must also put such > data into the Makefile). That's a good idea, but my guess is that the implementation cost of supporting the control format in your perl infrastructure is at least an order of magnitude lower than the cost for me to support your current JSON file format, so I lean towards you having an automated way to fill in the json file from the control one... The Makefile supports $(VERSION) because chances are it's already there (think packaging or tarball release targets). Having yet another place where to manually maintain a version number ain't appealing. In the latest patch, though, the only other thing you find in the Makefile about the extension is its basename, which must be the one of both the .control and the .sql files. And it's possible for $(EXTENSION) to be a list of them, too, because of contrib/spi. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Oct 21, 2010, at 12:33 AM, Dimitri Fontaine wrote: > I don't see what it buys us in this very context. The main thing here to > realize is that I wrote about no code to parse the control file. I don't > think the extension patch should depend on the JSON-in-core patch. > > Now, once we have JSON and before the release, I guess given a good > reason to have much more complex configuration files that don't look at > all like postgresql.conf, we could revisit. Sure. The reason to do it, though, is so that extension authors can create just one metadata file, instead of two (or three, if one must also put such data into the Makefile). Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > ... well, that's just a bug in hstore. *All* the contrib modules > should be using PGXS, unless they have a damn good reason not to; > which is not apparent for hstore. Here's a patch. -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/contrib/hstore/Makefile --- b/contrib/hstore/Makefile *** *** 1,9 # contrib/hstore/Makefile - subdir = contrib/hstore - top_builddir = ../.. - include $(top_builddir)/src/Makefile.global - MODULE_big = hstore OBJS = hstore_io.o hstore_op.o hstore_gist.o hstore_gin.o hstore_compat.o \ crc32.o --- 1,5 *** *** 12,15 DATA_built = hstore.sql --- 8,21 DATA = uninstall_hstore.sql REGRESS = hstore + ifdef USE_PGXS + PG_CONFIG = pg_config + PGXS := $(shell $(PG_CONFIG) --pgxs) + include $(PGXS) + else + subdir = contrib/hstore + top_builddir = ../.. + include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk + endif + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: > Itagaki Takahiro writes: >> Why does only hstore have version '9.1'? Any other modules have >> '9.1devel'. > It's the only contrib that's not using PGXS but instead directly > includes $(top_builddir)/src/Makefile.global, ... well, that's just a bug in hstore. *All* the contrib modules should be using PGXS, unless they have a damn good reason not to; which is not apparent for hstore. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
"David E. Wheeler" writes: > Might I suggest instead a META.json file like PGXN requires? Here's a > simple example: I don't see what it buys us in this very context. The main thing here to realize is that I wrote about no code to parse the control file. I don't think the extension patch should depend on the JSON-in-core patch. Now, once we have JSON and before the release, I guess given a good reason to have much more complex configuration files that don't look at all like postgresql.conf, we could revisit. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Oct 20, 2010, at 9:58 PM, Alvaro Herrera wrote: > What's wrong with sticking to Makefile syntax? Are we intending to > build a JSON parser in GNU make perchance? That metadata isn't *for* make, is it? D -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Itagaki Takahiro's message of jue oct 21 00:01:59 -0300 2010: > On Thu, Oct 21, 2010 at 8:14 AM, David E. Wheeler > wrote: > > Might I suggest instead a META.json file like PGXN requires? > > I think JSON is also reasonable, but one of the problem to use JSON format is > we cannot apply the extension patch until JSON patch has been applied ;-) What's wrong with sticking to Makefile syntax? Are we intending to build a JSON parser in GNU make perchance? -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Thu, Oct 21, 2010 at 8:14 AM, David E. Wheeler wrote: > Might I suggest instead a META.json file like PGXN requires? I think JSON is also reasonable, but one of the problem to use JSON format is we cannot apply the extension patch until JSON patch has been applied ;-) BTW, does anyone needs JSON formatted configuration files for other purposes? There might be some discussions in "Standby registration" or "Configuring synchronous replication" threads. Module control files are so simple that they don't always require JSON format, such as nested variable. But configuration files for replication might be more complex. If needed, it would be reasonable to introduce a JSON reader. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Thu, Oct 21, 2010 at 7:12 AM, Dimitri Fontaine wrote: > This control file contains at minimum a single line for the name of the > extension, but it's better already with a comment for users. I've been > filling them for our extensions, pasting from the documentation: > > name | version | > +--+ > fuzzystrmatch | 9.1devel | > hstore | 9.1 | Why does only hstore have version '9.1'? Any other modules have '9.1devel'. > If you provide a $(VERSION) variable, then a line in the control file is > automatically added at make install (version = '$(VERSION)'), in order > to make life easier for extension authors. In v7, a line of "version = '...'" is added at "make install", and removed at "make clean". Also, if we runs "make install" multiple times, version lines are added repeatedly. I don't think they are good ideas; we should not modify source codes stored in git repo when we build them. How about having *.control.in and replace magic keywords in them at "make"? "make install" won't modify files at all, and "make clean" just removes *.control. It is the way we're using for *.sql.in and MODULE_PATHNAME. > Some extensions are missing here because they fail to compile on my > workstation where all the libs aren't installed --- ossp, xml2, etc I found xml2/pgxml.control should have 'pgxml" for the name. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Oct 20, 2010, at 3:12 PM, Dimitri Fontaine wrote: > So, the idea is that $(EXTENSION) is a list of extensions you're > providing from the Makefile (most often, a list of one extension, but > contrib/spi is an exception here). Each extension in the list must have > a corresponding $EXTENSION.control file. > > This control file contains at minimum a single line for the name of the > extension, but it's better already with a comment for users. I've been > filling them for our extensions, pasting from the documentation: Might I suggest instead a META.json file like PGXN requires? Here's a simple example: { "name": "pair", "abstract": "A key/value pair data type", "version": "0.1.0", "maintainer": "David E. Wheeler ", "license": "postgresql", } They can have a lot more information, too. Her's the one I actually shipped with pair: http://github.com/theory/kv-pair/blob/master/META.json The meta spec is here: http://github.com/theory/pgxn/wiki/PGXN-Meta-Spec Anyway, the point is that it might be useful for us to sync on this format. I went with JSON for a few reasons: * CPAN is switching to it (from YAML) * It's extremely widespread * It's useful for ac-hoc REST-style requests * The format will likely be in 9.1. Thoughts? BTW, really excited that you're finally getting EXTENSION done, Dim. This is going to be *great* for PostgreSQL developers. I'll have to work it into my talk at West. https://www.postgresqlconference.org/content/building-and-distributing-postgresql-extensions-without-learning-c Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > That is simply a horrid idea. Just make it specify EXTENSION. And VERSION too, finally. So any extension > >> and guessing >> the CONTROL file name from the EXTENSION name only occurs when CONTROL >> has not been provided. > > Here, on the other hand, I'm wondering why have two variables at all. > Is there any sane use-case for the control file to not be named the same > as the extension? It seems like that would accomplish little except to > sow confusion. > > regards, tom lane -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > That is simply a horrid idea. Just make it specify EXTENSION. Black magic it is, will remove in v7. > Is there any sane use-case for the control file to not be named the same > as the extension? It seems like that would accomplish little except to > sow confusion. The goal of the 3 variables EXTENSION, EXTVERSION, EXTCOMMENT is to prepare the control file with 3 lines formatted variable = 'value'. If you specify CONTROL instead, that should be the file name you're providing directly. It then grew up into being a directive to produce the right file set for spi, but the simplest thing would be to hand prepare the files there. If you agree with that, that's what I'll do in v7. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: > Tom Lane writes: >> I don't think that "no changes to the makefiles" is a requirement, >> or even a wish-list item, for this. I think it's perfectly reasonable >> for the makefile to have to specify the module name; far better that >> than that we get the name by some "magic" or other. > It seemed easy to get a reasonable approach requiring very few edits in > contribs so I favoured that. Now, it's still entirely possible to hand > adjust. Determining the extension name automatically from DATA_built or > DATA is only done where EXTENSION has not been provided, That is simply a horrid idea. Just make it specify EXTENSION. > and guessing > the CONTROL file name from the EXTENSION name only occurs when CONTROL > has not been provided. Here, on the other hand, I'm wondering why have two variables at all. Is there any sane use-case for the control file to not be named the same as the extension? It seems like that would accomplish little except to sow confusion. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: >> and use the equivalent of SET LOCAL in the CREATE EXTENSION code? > > I had assumed that that was how he was doing it ... I'm currently doing: SetConfigOption("client_min_messages", "warning", PGC_SUSET, PGC_S_SESSION); And then manually reverting to what was there before the command: SetConfigOption("client_min_messages", old_cmsgs, PGC_SUSET, PGC_S_SESSION); The thing is that CREATE EXTENSION can be part of a transaction, so even SET LOCAL ain't going to work here, we need to reset before continuing the transaction. I don't know that SET LOCAL is RESET after a savepoint, so we would still need to care about that "by hand", right? -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > I don't think that "no changes to the makefiles" is a requirement, > or even a wish-list item, for this. I think it's perfectly reasonable > for the makefile to have to specify the module name; far better that > than that we get the name by some "magic" or other. It seemed easy to get a reasonable approach requiring very few edits in contribs so I favoured that. Now, it's still entirely possible to hand adjust. Determining the extension name automatically from DATA_built or DATA is only done where EXTENSION has not been provided, and guessing the CONTROL file name from the EXTENSION name only occurs when CONTROL has not been provided. Of course if those changes (inlined there after) are seen as a bad idea, then I will change all contrib Makefiles to add EXTENSION, EXTVERSION (which always is MAJORVERSION here) and CONTROL (which almost always is EXTENSION.control). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support # create extension support ifndef CONTROL ifndef EXTENSION ifdef DATA_built EXTENSION = $(basename $(notdir $(firstword $(DATA_built else ifdef DATA EXTENSION = $(basename $(notdir $(firstword $(DATA endif # DATA_built endif # EXTENSION ifndef EXTVERSION EXTVERSION = $(MAJORVERSION) endif ifdef EXTENSION CONTROL = $(EXTENSION).control endif # EXTENSION endif # CONTROL control: # create .control to keep track that we created the control file(s) @for file in $(CONTROL); do \ test -f `basename $$file .control`.sql -a ! -f $$file && touch .control || true ; \ if [ -f .control ]; then \ if [ -n "$(EXTENSION)" ]; then \ (echo "name = '$(EXTENSION)'"; echo "version = '$(EXTVERSION)'") > $$file ; \ else \ (echo "name = '`basename $$file .control`'"; echo "version = '$(EXTVERSION)'") > $$file ; \ fi ; \ if [ -n "$(EXTCOMMENT)" ]; then echo "comment = '$(EXTCOMMENT)'" >> $$file ; fi ; \ fi ; \ done install: all installdirs control ifneq (,$(DATA)$(DATA_built)$(CONTROL)) @for file in $(addprefix $(srcdir)/, $(DATA)) $(DATA_built) $(CONTROL); do \ echo "$(INSTALL_DATA) $$file '$(DESTDIR)$(datadir)/$(datamoduledir)'"; \ $(INSTALL_DATA) $$file '$(DESTDIR)$(datadir)/$(datamoduledir)'; \ done endif # DATA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: > Excerpts from Dimitri Fontaine's message of mié oct 20 07:22:53 -0300 2010: >> Using SPI to execute the extension's script already means that it can >> not contain explicit BEGIN and COMMIT commands. Now, is it possible to >> force a Reset of all GUCs after script's execution? > Would it work to force a new transaction internally in CREATE EXTENSION, No, but maybe a savepoint? > and use the equivalent of SET LOCAL in the CREATE EXTENSION code? I had assumed that that was how he was doing it ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: > Tom Lane writes: >> If the extensions manager is dependent on the assumption that a module's >> name matches the name of the directory it's built in > It is not. There's some magic for simple cases so that contrib mostly > "works" with no editing, but of course, that's only mostly. > The version Itakagi-San worked with had not a single change to the > contrib sources, I don't think that "no changes to the makefiles" is a requirement, or even a wish-list item, for this. I think it's perfectly reasonable for the makefile to have to specify the module name; far better that than that we get the name by some "magic" or other. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of mié oct 20 07:22:53 -0300 2010: > Itagaki Takahiro writes: > > CREATE EXTENSION command > > * Environment could be modified by the installer script. > > =# SHOW search_path; => "$user",public > > =# CREATE EXTENSION dblink; > > =# SHOW search_path; => public > > because almost all of the modules have SET search_path in the scripts: > > -- Adjust this setting to control where the objects get created. > > SET search_path = public; > > > > Is is an intended behavior? Personally, I want the installer to run in > > sandbox. > > One idea is to rewrite module scripts to use BEGIN - SET LOCAL - COMMIT, > > but we cannot execute CREATE EXTENSION in transaction if do so. > > Using SPI to execute the extension's script already means that it can > not contain explicit BEGIN and COMMIT commands. Now, is it possible to > force a Reset of all GUCs after script's execution? Would it work to force a new transaction internally in CREATE EXTENSION, and use the equivalent of SET LOCAL in the CREATE EXTENSION code? -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Robert Haas writes: > On Wed, Oct 20, 2010 at 6:22 AM, Dimitri Fontaine > wrote: >> In v6 patch, should client_min_messages or log_min_messages be lower >> than WARNING, they get set to WARNING for the script install context. We >> still dump the extension's script at each WARNING, but you can set your >> client_min_messages (and log_min_messages) to ERROR before hand. > I would vote for overriding client_min_messages but not log_min_messages. Why? The problem with unreasonably bulky messages is just as objectionable for the log. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > If the extensions manager is dependent on the assumption that a module's > name matches the name of the directory it's built in It is not. There's some magic for simple cases so that contrib mostly "works" with no editing, but of course, that's only mostly. The version Itakagi-San worked with had not a single change to the contrib sources, I've only begun to change things there (in v6) with the spi case, that now produces 5 extensions control files out of a single Makefile, thanks to this single new line: CONTROL = $(addsuffix .control, $(MODULES)) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Itagaki Takahiro writes: > On Wed, Oct 20, 2010 at 12:58 PM, Alvaro Herrera > wrote: >> Lets rename the directory. > Hmmm, but we call it 'xml2' in the doc. There is no 'pgxml' at all in it. > http://developer.postgresql.org/pgdocs/postgres/xml2.html > However, I don't think we can change the module name because pg_upgrade > will fail if the module (.so) name was changed. So, it might be the > point of compromise to keep two names until we deprecate it completely. If the extensions manager is dependent on the assumption that a module's name matches the name of the directory it's built in, that assumption needs to be removed anyway. There are too many use-cases where that wouldn't hold, even if we try to force the standard contrib modules to follow such a rule. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Wed, Oct 20, 2010 at 9:33 AM, Dimitri Fontaine wrote: > Robert Haas writes: >> I would vote for overriding client_min_messages but not log_min_messages. > > Well it defaults to WARNING so I see your point. Then again, we're > talking about hundreds of lines (3197 lines of isn, 531 lines for > hstore) of output per message, containing a script that you're not > maintaining. Do we really want that amount of extra logging? Well, my thought was that it makes sense to override the user's logging preferences because, after all, if they wanted the extra logging, they could run the script by hand. But what gets logged to the system log is server policy, not user preference, and I'm reluctant to think we should second-guess whatever the admin has configured. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Robert Haas writes: > I would vote for overriding client_min_messages but not log_min_messages. Well it defaults to WARNING so I see your point. Then again, we're talking about hundreds of lines (3197 lines of isn, 531 lines for hstore) of output per message, containing a script that you're not maintaining. Do we really want that amount of extra logging? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Wed, Oct 20, 2010 at 6:22 AM, Dimitri Fontaine wrote: > In v6 patch, should client_min_messages or log_min_messages be lower > than WARNING, they get set to WARNING for the script install context. We > still dump the extension's script at each WARNING, but you can set your > client_min_messages (and log_min_messages) to ERROR before hand. I would vote for overriding client_min_messages but not log_min_messages. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > Robert Haas writes: >> It seems good to do this in the normal case, but (1) if >> client_min_messages was already set higher than WARNING, we probably >> should not lower it and (2) we might want to have a way to lower it >> for troubleshooting purposes. > > I think the standard way of troubleshooting would be to run the > extension's script by hand, no? So while I agree with (1), > I'm not sure we need to sweat about (2). Will go and fix (1), but like Tom I think (2) is handled by the extension's author running pg_execute_from_file() rather than CREATE EXTENSION for debugging purposes: then the settings are not changed. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Sorry. I missed v5 patch and other new ones. Some of the issues might have been already fixed in the new version. On Wed, Oct 20, 2010 at 12:19 PM, Itagaki Takahiro wrote: > On Tue, Oct 19, 2010 at 9:33 PM, Dimitri Fontaine >> Fixed in v4, attached. > > Source code > * It still has a warning. > * It has duplicated oids. > > Tests for each contrib module > * Some modules dumps noisy NOTICE or WARNING messages. > * Modules that should be added to shared_preload_libraries. > * Modules that only have *.sql, but not *.sql.in. > * Hyphen (-) in module name > * xml2 module has a different extension name from the directory name. > * spi module has multiple *.so, but only one *.control. > > CREATE EXTENSION command > * Environment could be modified by the installer script. > * Non-ASCII characters in script -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Wed, Oct 20, 2010 at 12:58 PM, Alvaro Herrera wrote: >> * xml2 module has a different extension name from the directory name. >> The directory name is 'xml2', but the extension name is 'pgxml'. > > Lets rename the directory. Hmmm, but we call it 'xml2' in the doc. There is no 'pgxml' at all in it. http://developer.postgresql.org/pgdocs/postgres/xml2.html However, I don't think we can change the module name because pg_upgrade will fail if the module (.so) name was changed. So, it might be the point of compromise to keep two names until we deprecate it completely. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Itagaki Takahiro's message of mié oct 20 00:19:44 -0300 2010: > * xml2 module has a different extension name from the directory name. > The directory name is 'xml2', but the extension name is 'pgxml'. > I'm not sure whether we should change the names. Or, merging xml2 module > into core and deprecating xml2 might be the best solution. Lets rename the directory. We used to have a problem with that in CVS, in Git it's no longer an issue. Merging xml2 into core would be nice, but it's been attempted many times and it's obvious that it's going to require a lot of work. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Tue, Oct 19, 2010 at 9:33 PM, Dimitri Fontaine wrote: > Fixed in v4, attached. It works as expected in basic functionality, so I pick up edge case issues. Some of them might be a problem for the patch; They might come from our non-standardized module design. Source code * It still has a warning. backend/utils/init/postinit.c needs #include "commands/extension.h". * It has duplicated oids. $ ./duplicate_oids 3627 3695 3696 3697 3698 3699 Tests for each contrib module * Some modules dumps noisy NOTICE or WARNING messages. We need to fix btree_gist, chkpass, cube, hstore, isn, ltree, pg_trgm, and seg. CREATE EXTENSION commands for them dumps the contents of installer script as CONTEXT. We can add SET client_min_messages in each script, but there is an issue to use SET command in scripts, discussed in below. * Modules that should be added to shared_preload_libraries. auto_explain, dummy_seclabel, passwordcheck, and pg_stat_statements are designed to be preloaded with shared_preload_libraries. If we support modifying GUC variables vis SQL, we could add some hooks to update conf files. * Modules that only have *.sql, but not *.sql.in. There are no *.control files for intagg and intarray, maybe because they don't have *.sql.in. But we should support them, too. * Hyphen (-) in module name 'uuid-ossp' has a hyphen in the name. Do we need double-quotes to install such extensions? (CREATE EXTENSION "uuid-ossp" works.) Also, custom_variable_classes doesn't support hyphens; only [A-Za-z0-9_] are accepted for now, but will we also support hyphens? * xml2 module has a different extension name from the directory name. The directory name is 'xml2', but the extension name is 'pgxml'. I'm not sure whether we should change the names. Or, merging xml2 module into core and deprecating xml2 might be the best solution. * spi module has multiple *.so, but only one *.control. 'spi' module generates multiple libraries: 'autoinc', 'insert_username', 'moddatetime', 'refint', and 'timetravel'. But it has only autoinc.control. We might need control files for each library. CREATE EXTENSION command * Environment could be modified by the installer script. =# SHOW search_path; => "$user",public =# CREATE EXTENSION dblink; =# SHOW search_path; => public because almost all of the modules have SET search_path in the scripts: -- Adjust this setting to control where the objects get created. SET search_path = public; Is is an intended behavior? Personally, I want the installer to run in sandbox. One idea is to rewrite module scripts to use BEGIN - SET LOCAL - COMMIT, but we cannot execute CREATE EXTENSION in transaction if do so. * Non-ASCII characters in script User-defined module could have non-ascii characters in their install script. They often have "SET client_encoding" at the beginning, and it works as expected if executed by psql. However, it raises encoding errors if executed by CREATE EXTENSION because they are executed as one multi-command. Are there any solution to solve the issue? I think it's a bit difficult because encodings in database, script, and client might be different. If encodings in script and client are different, the server might need to handle two different client encodings in the same time. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of mar oct 19 13:09:47 -0300 2010: > Tom Lane writes: > > You could argue that either way I guess. The script knows what it > > needs, but OTOH just about every extension there is will probably > > be generating useless NOTICEs unless something is done, so maybe > > the extension management code should take care of it for them. > > Either way is the key here too, so please find attached a revised (v5) > patch which will force log_min_messages and client_min_messages to > WARNING while the script is run. I think you should pstrdup the return value from GetConfigOption. (As a matter of style, I'd name the local vars differently, so that they don't show up when you search the code for the global vars; perhaps "old_cmsgs" and "old_lmsgs" would do, for example.) -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Robert Haas writes: > On Tue, Oct 19, 2010 at 12:09 PM, Dimitri Fontaine > wrote: >> Either way is the key here too, so please find attached a revised (v5) >> patch which will force log_min_messages and client_min_messages to >> WARNING while the script is run. > It seems good to do this in the normal case, but (1) if > client_min_messages was already set higher than WARNING, we probably > should not lower it and (2) we might want to have a way to lower it > for troubleshooting purposes. I think the standard way of troubleshooting would be to run the extension's script by hand, no? So while I agree with (1), I'm not sure we need to sweat about (2). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Tue, Oct 19, 2010 at 12:09 PM, Dimitri Fontaine wrote: > Tom Lane writes: >> You could argue that either way I guess. The script knows what it >> needs, but OTOH just about every extension there is will probably >> be generating useless NOTICEs unless something is done, so maybe >> the extension management code should take care of it for them. > > Either way is the key here too, so please find attached a revised (v5) > patch which will force log_min_messages and client_min_messages to > WARNING while the script is run. It seems good to do this in the normal case, but (1) if client_min_messages was already set higher than WARNING, we probably should not lower it and (2) we might want to have a way to lower it for troubleshooting purposes. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: > Hmm, this needs some cleanup; the comments still talk about the old > function name; and about just the recovery.conf file. Ah yes, thinking it's an easy patch is not helping. Please find attached a revised version of it. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/src/backend/access/transam/xlog.c --- b/src/backend/access/transam/xlog.c *** *** 55,60 --- 55,61 #include "utils/guc.h" #include "utils/ps_status.h" #include "utils/relmapper.h" + #include "utils/cfparser.h" #include "pg_trace.h" *** *** 5018,5117 str_time(pg_time_t tnow) } /* - * Parse one line from recovery.conf. 'cmdline' is the raw line from the - * file. If the line is parsed successfully, returns true, false indicates - * syntax error. On success, *key_p and *value_p are set to the parameter - * name and value on the line, respectively. If the line is an empty line, - * consisting entirely of whitespace and comments, function returns true - * and *keyp_p and *value_p are set to NULL. - * - * The pointers returned in *key_p and *value_p point to an internal buffer - * that is valid only until the next call of parseRecoveryCommandFile(). - */ - static bool - parseRecoveryCommandFileLine(char *cmdline, char **key_p, char **value_p) - { - char *ptr; - char *bufp; - char *key; - char *value; - static char *buf = NULL; - - *key_p = *value_p = NULL; - - /* - * Allocate the buffer on first use. It's used to hold both the parameter - * name and value. - */ - if (buf == NULL) - buf = malloc(MAXPGPATH + 1); - bufp = buf; - - /* Skip any whitespace at the beginning of line */ - for (ptr = cmdline; *ptr; ptr++) - { - if (!isspace((unsigned char) *ptr)) - break; - } - /* Ignore empty lines */ - if (*ptr == '\0' || *ptr == '#') - return true; - - /* Read the parameter name */ - key = bufp; - while (*ptr && !isspace((unsigned char) *ptr) && - *ptr != '=' && *ptr != '\'') - *(bufp++) = *(ptr++); - *(bufp++) = '\0'; - - /* Skip to the beginning quote of the parameter value */ - ptr = strchr(ptr, '\''); - if (!ptr) - return false; - ptr++; - - /* Read the parameter value to *bufp. Collapse any '' escapes as we go. */ - value = bufp; - for (;;) - { - if (*ptr == '\'') - { - ptr++; - if (*ptr == '\'') - *(bufp++) = '\''; - else - { - /* end of parameter */ - *bufp = '\0'; - break; - } - } - else if (*ptr == '\0') - return false; /* unterminated quoted string */ - else - *(bufp++) = *ptr; - - ptr++; - } - *(bufp++) = '\0'; - - /* Check that there's no garbage after the value */ - while (*ptr) - { - if (*ptr == '#') - break; - if (!isspace((unsigned char) *ptr)) - return false; - ptr++; - } - - /* Success! */ - *key_p = key; - *value_p = value; - return true; - } - - /* * See if there is a recovery command file (recovery.conf), and if so * read in parameters for archive recovery and XLOG streaming. * --- 5019,5024 *** *** 5147,5153 readRecoveryCommandFile(void) char *tok1; char *tok2; ! if (!parseRecoveryCommandFileLine(cmdline, &tok1, &tok2)) { syntaxError = true; break; --- 5054,5060 char *tok1; char *tok2; ! if (!cfParseOneLine(cmdline, &tok1, &tok2)) { syntaxError = true; break; *** a/src/backend/utils/misc/Makefile --- b/src/backend/utils/misc/Makefile *** *** 15,21 include $(top_builddir)/src/Makefile.global override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS) OBJS = guc.o help_config.o pg_rusage.o ps_status.o superuser.o tzparser.o \ !rbtree.o # This location might depend on the installation directories. Therefore # we can't subsitute it into pg_config.h. --- 15,21 override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS) OBJS = guc.o help_config.o pg_rusage.o ps_status.o superuser.o tzparser.o \ !rbtree.o cfparser.o # This location might depend on the installation directories. Therefore # we can't subsitute it into pg_config.h. *** /dev/null --- b/src/backend/utils/misc/cfparser.c *** *** 0 --- 1,114 + /*- + * + * cfparser.c + * Function for parsing RecoveryCommandFile and Extension Control files + * + * This very simple file format (line oriented, variable = 'value') is used + * as the recovery.conf and the extension control file format. + * + * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * IDENTIFICATION + * src/backend/utils/misc/cfparser.c + * + *- + */ + + #include "postgres.h" + + /* + * Parse one line from
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: > The error message wording needs some work; maybe > errmsg("file \"%s\" could not be executed", filename) [...] > I happened to notice that there are several pieces of code that are > calling SPI_connect and SPI_finish without checking the return value, > notably the FTS code and xml.c. Please find attached pg_execute_from_file.v4.patch with fixes. I've used the usual error messages for SPI_connect() and finish this time. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/doc/src/sgml/func.sgml --- b/doc/src/sgml/func.sgml *** *** 13897,13902 postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup()); --- 13897,13909 record Return information about a file + + + pg_execute_from_file(filename text) + +void +Executes the SQL commands contained in a file + *** *** 13935,13940 SELECT (pg_stat_file('filename')).modification; --- 13942,13956 + + pg_execute_from_file + + + pg_execute_from_file makes the server + execute SQL commands to be found in a file. This function is + reserved to superusers. + + The functions shown in manage advisory locks. For details about proper use of these functions, see *** *** 13957,13962 SELECT (pg_stat_file('filename')).modification; --- 13973,13979 void Obtain exclusive advisory lock + pg_advisory_lock(key1 int, key2 int) *** a/src/backend/utils/adt/genfile.c --- b/src/backend/utils/adt/genfile.c *** *** 7,12 --- 7,13 * Copyright (c) 2004-2010, PostgreSQL Global Development Group * * Author: Andreas Pflug + * Dimitri Fontaine * * IDENTIFICATION * src/backend/utils/adt/genfile.c *** *** 21,26 --- 22,28 #include #include "catalog/pg_type.h" + #include "executor/spi.h" #include "funcapi.h" #include "mb/pg_wchar.h" #include "miscadmin.h" *** *** 264,266 pg_ls_dir(PG_FUNCTION_ARGS) --- 266,340 SRF_RETURN_DONE(funcctx); } + + /* + * Read a file then execute the SQL commands it contains. + */ + Datum + pg_execute_from_file(PG_FUNCTION_ARGS) + { + text *filename_t = PG_GETARG_TEXT_P(0); + char *filename; + FILE *file; + int64 fsize = -1, nbytes; + struct stat fst; + char *query_string = NULL; + + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + (errmsg("must be superuser to get file information"; + + /* + * Only superuser can call pg_execute_from_file, and CREATE EXTENSION + * uses that too. Don't double check the PATH. Also note that + * extension's install files are not in $PGDATA but `pg_config + * --sharedir`. + */ + filename = text_to_cstring(filename_t); + + if (stat(filename, &fst) < 0) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not stat file \"%s\": %m", filename))); + + fsize = Int64GetDatum((int64) fst.st_size); + + if ((file = AllocateFile(filename, PG_BINARY_R)) == NULL) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not open file \"%s\" for reading: %m", + filename))); + + if (ferror(file)) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not read file \"%s\": %m", filename))); + + query_string = (char *)palloc((fsize+1)*sizeof(char)); + memset(query_string, 0, fsize+1); + nbytes = fread(query_string, 1, (size_t) fsize, file); + pg_verifymbstr(query_string, nbytes, false); + FreeFile(file); + + /* + * We abuse some internal knowledge from spi.h here. As we don't know + * which queries are going to get executed, we don't know what to expect + * as an OK return code from SPI_execute(). We assume that + * SPI_OK_CONNECT, SPI_OK_FINISH and SPI_OK_FETCH are quite improbable, + * though, and the errors are negatives. So a valid return code is + * considered to be SPI_OK_UTILITY or anything from there. + */ + if (SPI_connect() != SPI_OK_CONNECT) + elog(ERROR, "SPI_connect failed"); + + if (SPI_execute(query_string, false, 0) < SPI_OK_UTILITY) + ereport(ERROR, + (errcode(ERRCODE_DATA_EXCEPTION), + errmsg("File '%s' could not be executed", filename))); + + if (SPI_finish() != SPI_OK_FINISH) + elog(ERROR, "SPI_finish failed"); + + PG_RETURN_VOID(); + } *** a/src/include/catalog/pg_proc.h --- b/src/include/catalog/pg_proc.h *** *** 3386,3399 DESCR("reload configuration files"); DATA(insert OID = 2622 ( pg_rotate_logfile PGNSP PGUID 12 1 0 0 f f f t f v 0 0 16 "" _null_ _null_ _null_ _null_ pg_rotate_logfile _null_ _null_ _null_ )); DESCR("rotate log file"); ! DATA(insert OID = 2623 ( pg_stat_file PGNSP PGUID 12 1 0 0 f f f t f v 1
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of vie oct 15 16:15:23 -0300 2010: > The cfparser patch didn't change, the current version is still v1. Hmm, this needs some cleanup; the comments still talk about the old function name; and about just the recovery.conf file. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Dimitri Fontaine's message of dom oct 17 16:30:47 -0300 2010: > The bulk of it is now short enough to be inlined in the mail, and if you > have more comments I guess they'll be directed at this portion of the > patch, so let's make it easy: > > /* > * We abuse some internal knowledge from spi.h here. As we don't know > * which queries are going to get executed, we don't know what to expect > * as an OK return code from SPI_execute(). We assume that > * SPI_OK_CONNECT, SPI_OK_FINISH and SPI_OK_FETCH are quite improbable, > * though, and the errors are negatives. So a valid return code is > * considered to be SPI_OK_UTILITY or anything from there. > */ > SPI_connect(); > if (SPI_execute(query_string, false, 0) < SPI_OK_UTILITY) > ereport(ERROR, > (errcode(ERRCODE_DATA_EXCEPTION), > errmsg("File '%s' contains invalid query", filename))); > SPI_finish(); SPI_OK_FETCH is indeed improbable -- it's not used anywhere in the SPI routines, and hasn't been for ages. SPI_OK_CONNECT and SPI_OK_FINNISH are only issued by SPI_connect and SPI_finish, so presumably they can't be returned by a script. The error message wording needs some work; maybe errmsg("file \"%s\" could not be executed", filename) SPI_execute sets the query as errcontext, which is good; but apparently there is no error position, which is bad. I'm not sure how feasible is to fix this. (Not this patch's responsibility anyway.) I happened to notice that there are several pieces of code that are calling SPI_connect and SPI_finish without checking the return value, notably the FTS code and xml.c. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > You could argue that either way I guess. The script knows what it > needs, but OTOH just about every extension there is will probably > be generating useless NOTICEs unless something is done, so maybe > the extension management code should take care of it for them. Either way is the key here too, so please find attached a revised (v5) patch which will force log_min_messages and client_min_messages to WARNING while the script is run. v5 also contains the \dx bug fix about repalloc. Please note that I didn't touch any contrib yet, so that hstore will still dump its full script here: dim=# create extension isn; NOTICE: Installing extension 'isn' from '/Users/dim/pgsql/exts/share/contrib/isn.sql', with user data CREATE EXTENSION dim=# create extension hstore; NOTICE: Installing extension 'hstore' from '/Users/dim/pgsql/exts/share/contrib/hstore.sql', with user data WARNING: => is deprecated as an operator name DETAIL: This name may be disallowed altogether in future versions of PostgreSQL. CONTEXT: SQL statement "/* contrib/hstore/hstore.sql.in */ The script follows here. Maybe 9.1 is when to deprecate => as an operator name in the hstore official extension? :) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support Of course the git repo is still maintained for those prefering it: http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension extension.v5.patch.gz Description: pg_dump support for PostgreSQL extensions -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: > Tom Lane writes: >> Only if the script is intentionally noisy. The right fix here is >> probably to bump up the min message level while running the script. > You mean doing that from the SQL script itself (using SET) or in the > pg_execute_from_file() code? My guess is the former, but... You could argue that either way I guess. The script knows what it needs, but OTOH just about every extension there is will probably be generating useless NOTICEs unless something is done, so maybe the extension management code should take care of it for them. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > Only if the script is intentionally noisy. The right fix here is > probably to bump up the min message level while running the script. You mean doing that from the SQL script itself (using SET) or in the pg_execute_from_file() code? My guess is the former, but... Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: > Robert Haas writes: >> I don't see why. I think the real action item here is to remove => >> from hstore. > As input, consider that lots of extensions will create types that are > only a shell at the moment of the CREATE TYPE, and for each of those > types you will see the (potentially > 1000 lines long) whole SQL script > dumped on the screen. Only if the script is intentionally noisy. The right fix here is probably to bump up the min message level while running the script. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Tue, Oct 19, 2010 at 9:07 AM, Dimitri Fontaine wrote: > Robert Haas writes: >> I don't see why. I think the real action item here is to remove => >> from hstore. > > As input, consider that lots of extensions will create types that are > only a shell at the moment of the CREATE TYPE, and for each of those > types you will see the (potentially > 1000 lines long) whole SQL script > dumped on the screen. > > In the following script, I've filtered out the scripts, but it's written > out for each NOTICE message that you see: > > dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension citext;" > 2>&1 | egrep 'NOTICE|ERROR' > NOTICE: Installing extension 'citext' from > '/Users/dim/pgsql/exts/share/contrib/citext.sql', with user data > NOTICE: return type citext is only a shell > NOTICE: argument type citext is only a shell > NOTICE: return type citext is only a shell > NOTICE: argument type citext is only a shell Well, perhaps it's reasonable to suppress NOTICE messages then. That particular message could perhaps be quieted, but we probably don't want to do that with every NOTICE that might occur (e.g. those that you might get on table creation for sequences, indices, etc.). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Robert Haas writes: > I don't see why. I think the real action item here is to remove => > from hstore. As input, consider that lots of extensions will create types that are only a shell at the moment of the CREATE TYPE, and for each of those types you will see the (potentially > 1000 lines long) whole SQL script dumped on the screen. In the following script, I've filtered out the scripts, but it's written out for each NOTICE message that you see: dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension citext;" 2>&1 | egrep 'NOTICE|ERROR' NOTICE: Installing extension 'citext' from '/Users/dim/pgsql/exts/share/contrib/citext.sql', with user data NOTICE: return type citext is only a shell NOTICE: argument type citext is only a shell NOTICE: return type citext is only a shell NOTICE: argument type citext is only a shell dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension cube;" 2>&1 | egrep 'NOTICE|ERROR' NOTICE: Installing extension 'cube' from '/Users/dim/pgsql/exts/share/contrib/cube.sql', with user data NOTICE: type "cube" is not yet defined NOTICE: return type cube is only a shell NOTICE: return type cube is only a shell NOTICE: argument type cube is only a shell dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension earthdistance;" 2>&1 | egrep 'NOTICE|ERROR' NOTICE: Installing extension 'earthdistance' from '/Users/dim/pgsql/exts/share/contrib/earthdistance.sql', with user data dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension fuzzystrmatch;" 2>&1 | egrep 'NOTICE|ERROR' NOTICE: Installing extension 'fuzzystrmatch' from '/Users/dim/pgsql/exts/share/contrib/fuzzystrmatch.sql', with user data dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension hstore;" 2>&1 | egrep 'NOTICE|ERROR' NOTICE: Installing extension 'hstore' from '/Users/dim/pgsql/exts/share/contrib/hstore.sql', with user data NOTICE: return type hstore is only a shell NOTICE: argument type hstore is only a shell NOTICE: return type hstore is only a shell NOTICE: argument type hstore is only a shell NOTICE: return type ghstore is only a shell NOTICE: argument type ghstore is only a shell dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension isn;" 2>&1 | egrep 'NOTICE|ERROR' NOTICE: Installing extension 'isn' from '/Users/dim/pgsql/exts/share/contrib/isn.sql', with user data NOTICE: type "ean13" is not yet defined NOTICE: argument type ean13 is only a shell NOTICE: type "isbn13" is not yet defined NOTICE: argument type isbn13 is only a shell NOTICE: type "ismn13" is not yet defined NOTICE: argument type ismn13 is only a shell NOTICE: type "issn13" is not yet defined NOTICE: argument type issn13 is only a shell NOTICE: type "isbn" is not yet defined NOTICE: argument type isbn is only a shell NOTICE: type "ismn" is not yet defined NOTICE: argument type ismn is only a shell NOTICE: type "issn" is not yet defined NOTICE: argument type issn is only a shell NOTICE: type "upc" is not yet defined NOTICE: argument type upc is only a shell dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension ltree;" 2>&1 | egrep 'NOTICE|ERROR' NOTICE: Installing extension 'ltree' from '/Users/dim/pgsql/exts/share/contrib/ltree.sql', with user data NOTICE: type "ltree" is not yet defined NOTICE: argument type ltree is only a shell NOTICE: type "lquery" is not yet defined NOTICE: argument type lquery is only a shell NOTICE: type "ltxtquery" is not yet defined NOTICE: argument type ltxtquery is only a shell NOTICE: type "ltree_gist" is not yet defined NOTICE: argument type ltree_gist is only a shell Just saying, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support PS: Oh, a repalloc() bug now. Will fix later in the afternoon, \dx or select * from pg_extensions(); crashes with more than 10 extensions installed in the v4 patch. That's what I get for doing that on a Saturday evening. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Oct 19, 2010, at 8:33 AM, Dimitri Fontaine wrote: > I didn't realise that using SPI would mean dumping the all script in > case of even NOTICEs. May be we want to protect against that in the > CREATE EXTENSION case, but I didn't have a look at how to do it. Do we > want CREATE EXTENSION to be more quiet than SPI usually is? I don't see why. I think the real action item here is to remove => from hstore. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Itagaki Takahiro writes: > * There are some compiler warnings. You might be missing something in > copyfuncs and equalfuncs. Fixed in v4, attached. > * There might be some bugs in pg_dump: > pg_dump: schema with OID 2200 does not exist, but is needed for object > 16411 Fixed in v4, attached. The problem was with the query used in pg_dump to filter out relations that are part of an extension, in getTables(). A composite type will create an underlying relation of relkind 'c', but there was no direct dependency to the extension, thus the filter failed to bypass it. It's fixed by adding a direct internal dependency between the relation and the extension, as that's so much easier than doing a recursive scan of pg_depend in pg_dump SQL queries. I will try to find out if other cases are forgotten like this one. Note that as a result you need to drop extension dblink then create it again so that you benefit from the fix. Also note that drop extension is recursively following the dependencies so is not concerned by this bug. > * The example in the doc "CREATE EXTENSION hstore" dumps surprising > warning messages, > We would be better to avoid such messages, though it's not an issue > for EXTENSION. > > WARNING: => is deprecated as an operator name > DETAIL: This name may be disallowed altogether in future versions of > PostgreSQL. > CONTEXT: SQL statement "/* contrib/hstore/hstore.sql.in */ > (followed by dumped script) > I didn't realise that using SPI would mean dumping the all script in case of even NOTICEs. May be we want to protect against that in the CREATE EXTENSION case, but I didn't have a look at how to do it. Do we want CREATE EXTENSION to be more quiet than SPI usually is? > * Docs sql-createextension.html has two odd links: > > See Also > DROP EXTENSION, Table 9-61, Appendix F > Fixed in v4, attached. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support extension.v4.patch.gz Description: pg_dump support for PostgreSQL extensions -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Itagaki Takahiro writes: > CREATE EXTENSION is interesting feature! > I just compiled it and tested it via SQL commands. Here is a quick > report. Thanks for you time and interest! > * There are some compiler warnings. You might be missing something in > copyfuncs and equalfuncs. > > copyfuncs.c:3119: warning: ‘_copyCreateExtensionStmt’ defined but not used > copyfuncs.c:3130: warning: ‘_copyDropExtensionStmt’ defined but not used > equalfuncs.c:1593: warning: ‘_equalCreateExtensionStmt’ defined but not used > equalfuncs.c:1602: warning: ‘_equalDropExtensionStmt’ defined but not used > postinit.c: In function ‘CheckMyDatabase’: > postinit.c:341: warning: implicit declaration of function ‘ExtensionSetCVC’ > Ouch, sorry about that, I didn't spot them. Will fix and post a v4 patch soon. > * There might be some bugs in pg_dump: > > postgres=# CREATE EXTENSION dblink; > NOTICE: Installing extension 'dblink' from > '$PGHOME/share/contrib/dblink.sql', with user data > CREATE EXTENSION > postgres=# \q > $ pg_dump > pg_dump: schema with OID 2200 does not exist, but is needed for object 16411 > I've hit that sometime but though that were tied to the dependency bug fixed in the v3 patch. I can reproduce here, will fix too. > * The example in the doc "CREATE EXTENSION hstore" dumps surprising > warning messages, > We would be better to avoid such messages, though it's not an issue > for EXTENSION. > > WARNING: => is deprecated as an operator name > DETAIL: This name may be disallowed altogether in future versions of > PostgreSQL. > CONTEXT: SQL statement "/* contrib/hstore/hstore.sql.in */ > (followed by dumped script) > I don't have a dumped script here, that maybe depends on verbosity options? > * Docs sql-createextension.html has two odd links: > > See Also > DROP EXTENSION, Table 9-61, Appendix F > I didn't know if using xref would do, but if you find that odd, I will replace with linkend and a custom label there. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
On Mon, Oct 18, 2010 at 8:37 PM, Dimitri Fontaine wrote: > Here's another version of the patch, v3. Changes: CREATE EXTENSION is interesting feature! I just compiled it and tested it via SQL commands. Here is a quick report. * There are some compiler warnings. You might be missing something in copyfuncs and equalfuncs. copyfuncs.c:3119: warning: ‘_copyCreateExtensionStmt’ defined but not used copyfuncs.c:3130: warning: ‘_copyDropExtensionStmt’ defined but not used equalfuncs.c:1593: warning: ‘_equalCreateExtensionStmt’ defined but not used equalfuncs.c:1602: warning: ‘_equalDropExtensionStmt’ defined but not used postinit.c: In function ‘CheckMyDatabase’: postinit.c:341: warning: implicit declaration of function ‘ExtensionSetCVC’ * There might be some bugs in pg_dump: postgres=# CREATE EXTENSION dblink; NOTICE: Installing extension 'dblink' from '$PGHOME/share/contrib/dblink.sql', with user data CREATE EXTENSION postgres=# \q $ pg_dump pg_dump: schema with OID 2200 does not exist, but is needed for object 16411 * The example in the doc "CREATE EXTENSION hstore" dumps surprising warning messages, We would be better to avoid such messages, though it's not an issue for EXTENSION. WARNING: => is deprecated as an operator name DETAIL: This name may be disallowed altogether in future versions of PostgreSQL. CONTEXT: SQL statement "/* contrib/hstore/hstore.sql.in */ (followed by dumped script) * Docs sql-createextension.html has two odd links: See Also DROP EXTENSION, Table 9-61, Appendix F -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Dimitri Fontaine writes: >> - Bugs to fix >> >>There's at least one where drop extension leaves things behind, >>although it uses performDeletion(). The dependencies are fine enough >>so that the leftover objects are not part of the dump done before to >>drop, though. > > That well seems to me to be an existing bug that I'm just putting in the > light. The reading of comments from findDependentObjects() makes me > think that following nested DEPENDENCY_INTERNAL levels is broken, but I > didn't have time to figure out how exactly, nor to try to fix. > > > http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension Here's another version of the patch, v3. Changes: - mentioned bug is fixed, that indeed was a shortcoming in following dependencies, due to a new way of using DEPENDENCY_INTERNAL in the extension code. - system view pg_extensions, using a function of the same name, lists all available and installed extensions. That should make the life of pgAdmin developers easier, among other users :) - pg_dump now issues CREATE EXTENSION foo WITH NO DATA; variant, and extension install script can use pg_extension_with_user_data() which returns true only when they have to create user data objects (often, fact table with pre-loaded data that the user is free to change) - pgxs.mk now will use given $(EXTENSION), $(EXTVERSION) and $(EXTCOMMENT) variables to produce the control file, or use an existing one. When $(EXTENSION) is not given, it will try to guess it from $(DATA) and $(DATA_built). - more documentation, I think it's all covered now - merges of the cfparser and pg_execute_from_file latest versions, so that the patch is self-contained and you can test it should you want to. I intend to merge from the official branch (pgmaster here) then produce another version of the patch without that code once it's separately committed, as that's the outlined plan so far. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support extension.v3.patch.gz Description: pg_dump support for PostgreSQL extensions -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Tom Lane writes: > Alvaro Herrera writes: >> Eh, I realize now that the right way to go about this is to use SPI. > > Yeah, that would be one way to go about it. But IMO postgres.c should > be solely concerned with interactions with the client. I didn't notice it's "possible" to use SPI from within the backend core code, and now see precedent in xml.c where the user can give a query string. I've used SPI_execute() in the new (attached) version of the patch, that's not touching postgres.c at all anymore. The bulk of it is now short enough to be inlined in the mail, and if you have more comments I guess they'll be directed at this portion of the patch, so let's make it easy: /* * We abuse some internal knowledge from spi.h here. As we don't know * which queries are going to get executed, we don't know what to expect * as an OK return code from SPI_execute(). We assume that * SPI_OK_CONNECT, SPI_OK_FINISH and SPI_OK_FETCH are quite improbable, * though, and the errors are negatives. So a valid return code is * considered to be SPI_OK_UTILITY or anything from there. */ SPI_connect(); if (SPI_execute(query_string, false, 0) < SPI_OK_UTILITY) ereport(ERROR, (errcode(ERRCODE_DATA_EXCEPTION), errmsg("File '%s' contains invalid query", filename))); SPI_finish(); Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/doc/src/sgml/func.sgml --- b/doc/src/sgml/func.sgml *** *** 13895,13900 postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup()); --- 13895,13907 record Return information about a file + + + pg_execute_from_file(filename text) + +void +Executes the SQL commands contained in a file + *** *** 13933,13938 SELECT (pg_stat_file('filename')).modification; --- 13940,13954 + + pg_execute_from_file + + + pg_execute_from_file makes the server + execute SQL commands to be found in a file. This function is + reserved to superusers. + + The functions shown in manage advisory locks. For details about proper use of these functions, see *** *** 13955,13960 SELECT (pg_stat_file('filename')).modification; --- 13971,13977 void Obtain exclusive advisory lock + pg_advisory_lock(key1 int, key2 int) *** a/src/backend/utils/adt/genfile.c --- b/src/backend/utils/adt/genfile.c *** *** 7,12 --- 7,13 * Copyright (c) 2004-2010, PostgreSQL Global Development Group * * Author: Andreas Pflug + * Dimitri Fontaine * * IDENTIFICATION * src/backend/utils/adt/genfile.c *** *** 21,26 --- 22,28 #include #include "catalog/pg_type.h" + #include "executor/spi.h" #include "funcapi.h" #include "mb/pg_wchar.h" #include "miscadmin.h" *** *** 264,266 pg_ls_dir(PG_FUNCTION_ARGS) --- 266,336 SRF_RETURN_DONE(funcctx); } + + /* + * Read a file then execute the SQL commands it contains. + */ + Datum + pg_execute_from_file(PG_FUNCTION_ARGS) + { + text *filename_t = PG_GETARG_TEXT_P(0); + char *filename; + FILE *file; + int64 fsize = -1, nbytes; + struct stat fst; + char *query_string = NULL; + + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + (errmsg("must be superuser to get file information"; + + /* + * Only superuser can call pg_execute_from_file, and CREATE EXTENSION + * uses that too. Don't double check the PATH. Also note that + * extension's install files are not in $PGDATA but `pg_config + * --sharedir`. + */ + filename = text_to_cstring(filename_t); + + if (stat(filename, &fst) < 0) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not stat file \"%s\": %m", filename))); + + fsize = Int64GetDatum((int64) fst.st_size); + + if ((file = AllocateFile(filename, PG_BINARY_R)) == NULL) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not open file \"%s\" for reading: %m", + filename))); + + if (ferror(file)) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not read file \"%s\": %m", filename))); + + query_string = (char *)palloc((fsize+1)*sizeof(char)); + memset(query_string, 0, fsize+1); + nbytes = fread(query_string, 1, (size_t) fsize, file); + pg_verifymbstr(query_string, nbytes, false); + FreeFile(file); + + /* + * We abuse some internal knowledge from spi.h here. As we don't know + * which queries are going to get executed, we don't know what to expect + * as an OK return code from SPI_execute(). We assume t
Re: [HACKERS] Extensions, this time with a patch
On Sun, Oct 17, 2010 at 12:09:51AM -0300, Alvaro Herrera wrote: > Excerpts from Tom Lane's message of sáb oct 16 23:32:49 -0300 2010: > > Alvaro Herrera writes: > > > The intent here is to execute some code from the file directly inside > > > the server. > > > > > Eh, I realize now that the right way to go about this is to use SPI. > > > > Yeah, that would be one way to go about it. But IMO postgres.c should > > be solely concerned with interactions with the client. > > Duly noted. Should this be noted in a README? Source code comments? I'm thinking if Alvaro didn't know it, it's not clear enough from context. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Excerpts from Tom Lane's message of sáb oct 16 23:32:49 -0300 2010: > Alvaro Herrera writes: > > The intent here is to execute some code from the file directly inside > > the server. > > > Eh, I realize now that the right way to go about this is to use SPI. > > Yeah, that would be one way to go about it. But IMO postgres.c should > be solely concerned with interactions with the client. Duly noted. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extensions, this time with a patch
Alvaro Herrera writes: > The intent here is to execute some code from the file directly inside > the server. > Eh, I realize now that the right way to go about this is to use SPI. Yeah, that would be one way to go about it. But IMO postgres.c should be solely concerned with interactions with the client. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers