subject:"\[HACKERS\] Extensions, this time with a patch"

Re: [HACKERS] Extensions, this time with a patch

2010-12-03 Thread Robert Haas

On Thu, Nov 25, 2010 at 4:06 PM, Dimitri Fontaine
dimi...@2ndquadrant.fr wrote:
 Itagaki Takahiro itagaki.takah...@gmail.com 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

2010-12-03 Thread Dimitri Fontaine

Robert Haas robertmh...@gmail.com 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

2010-11-25 Thread Dimitri Fontaine

Itagaki Takahiro itagaki.takah...@gmail.com 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

2010-11-24 Thread Itagaki Takahiro

On Tue, Nov 23, 2010 at 18:19, Dimitri Fontaine dimi...@2ndquadrant.fr 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

2010-11-24 Thread Dimitri Fontaine

Itagaki Takahiro itagaki.takah...@gmail.com 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;
! 
! 		if

Re: [HACKERS] Extensions, this time with a patch

2010-11-24 Thread Itagaki Takahiro

On Thu, Nov 25, 2010 at 05:02, Dimitri Fontaine dimi...@2ndquadrant.fr 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

2010-11-23 Thread Dimitri Fontaine

Alvaro Herrera alvhe...@commandprompt.com 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

2010-11-23 Thread Dimitri Fontaine

Alvaro Herrera alvhe...@commandprompt.com 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)));
  		}
! 		else if

Re: [HACKERS] Extensions, this time with a patch

2010-11-23 Thread Heikki Linnakangas


On 22.11.2010 03:35, Robert Haas wrote:

On Sun, Nov 21, 2010 at 8:10 PM, Itagaki Takahiro
itagaki.takah...@gmail.com  wrote:

On Wed, Oct 20, 2010 at 01:36, Dimitri Fontainedimi...@2ndquadrant.fr  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

2010-11-22 Thread Dimitri Fontaine

Itagaki Takahiro itagaki.takah...@gmail.com 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

1 2 >

1 - 100 of 120 matches

Mail list logo