Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sat, 4 Mar 2017 14:26:54 +0530
Robert Haas <robertmh...@gmail.com> wrote:

> On Fri, Mar 3, 2017 at 7:21 PM, Karl O. Pinc <k...@meme.com> wrote:
> > But if the code does not exit the loop then
> > before calling elog(ERROR), shouldn't it
> > also call FreeFile(fd)?  
> 
> Hmm.  Normally error recovery cleans up opened files, but since
> logfile_open() directly calls fopen(), that's not going to work here.
> So that does seem to be a problem.

Should something different be done to open the file or is it
enough to call FreeFile(fd) to clean up properly?

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Fri, 3 Mar 2017 15:24:53 +0900
Michael Paquier  wrote:
> 
> +   /*
> +* No space found, file content is corrupted.  Return
> NULL to the
> +* caller and inform him on the situation.
> +*/
> +   elog(ERROR,
> +"missing space character in \"%s\"",
> LOG_METAINFO_DATAFILE);
> +   break;
> There is no need to issue a break after a elog(ERROR).

There's 2 cases of issuing a break after a elog(ERROR),
both in the same loop.

And the comment could then be amended as well to just:

  No space found, file content is corrupted

Because (I presume) there's no returning of any value
going on.

But if the code does not exit the loop then
before calling elog(ERROR), shouldn't it
also call FreeFile(fd)?

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 15 Feb 2017 15:23:00 -0500
Robert Haas  wrote:

> +ereport(WARNING,
> +(errcode(ERRCODE_INTERNAL_ERROR),
> + errmsg("corrupted data found in \"%s\"",
> +LOG_METAINFO_DATAFILE)));
> 
> elog seems fine here.  There's no need for this to be translatable.
> Also, I'd change the level to ERROR.
> 
> + errhint("The supported log formats are
> \"stderr\""
> +" and \"csvlog\".")));
> 
> I think our preferred style is not to break strings across lines like
> this.
> 
> +log_filepath[strcspn(log_filepath, "\n")] = '\0';
> 
> We have no existing dependency on strcspn().  It might be better not
> to add one just for this feature; I suggest using strchr() instead,
> which is widely used in our existing code.


Attached is a v29 patch which fixes the above problems.

The Syslogger hunk remains to be fixed.  I have no plans
to do so at this time.

Note that since I have to write an "if" anyway, I'm going ahead
and raising an error condition when there's no newline in the
current_logfiles file.   The strcspn() ignored the missing newline.
The new code could do so as well by negating the if condition
should that be preferable.


On a different topic, I pulled from master and now
I see that git finds the following untracked:

src/bin/pg_basebackup/pg_receivexlog
src/bin/pg_resetxlog/
src/bin/pg_xlogdump/

I'd appreciate knowing if I'm doing something dumb
on my end to make this happen.  Thanks.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 963e70c..ae1fa0b 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -906,6 +906,7 @@ pg_current_logfile(PG_FUNCTION_ARGS)
 	char*logfmt;
 	char*log_filepath;
 	char*log_format = lbuffer;
+	char*nlpos;
 
 	/* The log format parameter is optional */
 	if (PG_NARGS() == 0 || PG_ARGISNULL(0))
@@ -918,8 +919,7 @@ pg_current_logfile(PG_FUNCTION_ARGS)
 			ereport(ERROR,
 	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 	 errmsg("log format \"%s\" is not supported", logfmt),
-	 errhint("The supported log formats are \"stderr\""
-	" and \"csvlog\".")));
+	 errhint("The supported log formats are \"stderr\" and \"csvlog\".")));
 	}
 
 	fd = AllocateFile(LOG_METAINFO_DATAFILE, "r");
@@ -947,19 +947,28 @@ pg_current_logfile(PG_FUNCTION_ARGS)
 		if (log_filepath == NULL)
 		{
 			/*
-			 * Corrupted data found, return NULL to the caller and
+			 * Corrupted data found, no space.  Return NULL to the caller and
 			 * inform him on the situation.
 			 */
-			ereport(WARNING,
-	(errcode(ERRCODE_INTERNAL_ERROR),
-	 errmsg("corrupted data found in \"%s\"",
-			LOG_METAINFO_DATAFILE)));
+			elog(ERROR,
+ "missing space character in \"%s\"", LOG_METAINFO_DATAFILE);
 			break;
 		}
 
 		*log_filepath = '\0';
 		log_filepath++;
-		log_filepath[strcspn(log_filepath, "\n")] = '\0';
+		nlpos = strchr(log_filepath, '\n');
+		if (nlpos == NULL)
+		{
+			/*
+			 * Corrupted data found, no newline.  Return NULL to the caller
+			 * and inform him on the situation.
+			 */
+			elog(ERROR,
+ "missing newline character in \"%s\"", LOG_METAINFO_DATAFILE);
+			break;
+		}
+		*nlpos = '\0';
 
 		if (logfmt == NULL || strcmp(logfmt, log_format) == 0)
 		{

-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 15 Feb 2017 15:23:00 -0500
Robert Haas  wrote:

> + errhint("The supported log formats are
> \"stderr\""
> +" and \"csvlog\".")));
> 
> I think our preferred style is not to break strings across lines like
> this.

How do you do that and not exceed the 80 character line limit?
Just ignore the line length limit?


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 19 Jan 2017 12:12:18 -0300
Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:

> Karl O. Pinc wrote:
> > On Wed, 18 Jan 2017 19:27:40 -0300
> > Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:  
> 
> > > I thought this part was odd -- I mean, why is SysLogger_Start()
> > > being called if the collector is not enabled?  Turns out we do it
> > > and return early if not enabled.  But not in all cases -- there
> > > is one callsite in postmaster.c that avoids the call if the
> > > collector is disabled. That needs to be changed if we want this
> > > to work reliably.  
> > 
> > Is this an argument for having the current_logfiles always exist
> > and be empty when there is no in-filesystem logfile?  It always felt
> > to me that the code would be simpler that way.  
> 
> I don't know.  I am just saying that you need to patch postmaster.c
> line 1726 to remove the second arm of the &&.

Gilles,

I'm not available just now.  Can you do this or enlist Michael?

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 19:27:40 -0300
Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:

> Karl O. Pinc wrote:
> 
> > @@ -511,10 +519,16 @@ int
> >  SysLogger_Start(void)
> >  {
> > pid_t   sysloggerPid;
> > -   char   *filename;
> >  
> > +   /*
> > +* Logging collector is not enabled. We don't know where
> > messages are
> > +* logged.  Remove outdated file holding the current log
> > filenames.
> > +*/
> > if (!Logging_collector)
> > +   {
> > +   unlink(LOG_METAINFO_DATAFILE);
> > return 0;
> > +   }  
> 
> I thought this part was odd -- I mean, why is SysLogger_Start() being
> called if the collector is not enabled?  Turns out we do it and return
> early if not enabled.  But not in all cases -- there is one callsite
> in postmaster.c that avoids the call if the collector is disabled.
> That needs to be changed if we want this to work reliably.

Is this an argument for having the current_logfiles always exist
and be empty when there is no in-filesystem logfile?  It always felt
to me that the code would be simpler that way.

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 19 Jan 2017 16:59:10 +0900
Michael Paquier  wrote:

> On Thu, Jan 19, 2017 at 7:27 AM, Alvaro Herrera
>  wrote:

> > I don't think the "abstract names" stuff is an improvement (just
> > look at the quoting mess in ConfigureNamesString).  I think we
> > should do without that; at least as part of this patch.  If you
> > think there's code that can get better because of the idea, let's
> > see it.  

Fair enough.

FWIW, when I first wrote the "abstract names" stuff there were another
half dozen or so occurrences of the constants.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 15:08:09 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> I would like to see index entries for "current_logfiles"
> so this stuff is findable.

Attached is a v27 of the patch.

I polished some of the sentences in the documentation
and added index entries.

Also attached are a series of 2 patches to apply on top
of v27 which make symbols out of hardcoded constants.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c..427980d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4218,6 +4218,11 @@ SELECT * FROM parent WHERE key = 2400;
   where to log
  
 
+ 
+   current_logfiles
+   and the log_destination configuration parameter
+ 
+
  
 
  
@@ -4248,6 +4253,27 @@ SELECT * FROM parent WHERE key = 2400;
  must be enabled to generate
 CSV-format log output.

+   
+When either stderr or
+csvlog are included, the file
+current_logfiles is created to record the location
+of the log file(s) currently in use by the logging collector and the
+associated logging destination. This provides a convenient way to
+find the logs currently in use by the instance. Here is an example of
+this file's content:
+
+stderr pg_log/postgresql.log
+csvlog pg_log/postgresql.csv
+
+
+current_logfiles is recreated when a new log file
+is created as an effect of rotation, and
+when log_destination is reloaded.  It is removed when
+neither stderr
+nor csvlog are included
+in log_destination, and when the logging collector is
+disabled.
+   
 

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2504a46..5b0be26 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15449,6 +15449,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15667,6 +15674,45 @@ SET search_path TO schema , schema, ..

 

+pg_current_logfile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+ current_logfiles
+ and the pg_current_logfile function
+   
+
+   
+Logging
+current_logfiles file and the pg_current_logfile
+function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of the log file(s) currently in use by the logging collector.
+The path includes the  directory
+and the log file name.  Log collection must be enabled or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply, as text,
+either csvlog or stderr as the value of the
+optional parameter. The return value is NULL when the
+log format requested is not a configured
+.  The
+pg_current_logfiles reflects the contents of the
+current_logfiles file.
+   
+
+   
 pg_my_temp_schema

 
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..9865751 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -36,6 +36,10 @@ these required items, the cluster configuration files
 PGDATA, although it is possible to place them elsewhere.
 
 
+
+  current_logfiles
+
+
 
 Contents of PGDATA
 
@@ -61,6 +65,12 @@ Item
 
 
 
+ current_logfiles
+ File recording the log file(s) currently written to by the logging
+  collector
+
+
+
  global
  Subdirectory containing cluster-wide tables, such as
  pg_database
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 07f291b..cbeeab8 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1055,6 +1055,7 @@ REVOKE EXECUTE ON FUNCTION pg_xlog_replay_pause() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_xlog_replay_resume() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_rotate_logfile() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_reload_conf() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_current_logfile() FROM public;
 
 REVOKE EXECUTE ON FUNCTION pg_stat_reset() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_shared(text) FROM public;
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 13a0301..e6899c6 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 15:52:36 -0500
Robert Haas <robertmh...@gmail.com> wrote:

> On Wed, Jan 18, 2017 at 12:08 PM, Karl O. Pinc <k...@meme.com> wrote:
> > Seems to me that the file format should
> > be documented if there's any intention that the end user
> > look at or otherwise use the file's content.
> >
> > It's fine with me if the content of current_logfiles
> > is supposed to be internal to PG and not exposed
> > to the end user.  I'm writing to make sure that
> > this is a considered decision.  
> 
> On the whole, documenting it seems better than documenting it,
> provided there's a logical place to include it in the existing
> documentation.
> 
> But, anyway, Michael shouldn't remove it without some explanation or
> discussion.

He explained that where it was looked clunky, it being
inside a table that otherwise has rows that are not tall.

And, it looks like I'm wrong.  The format is documented
by way of an example in section 19.8.1. Where To Log
under log_destination (string).

Sorry for the bother.

I would like to see index entries for "current_logfiles"
so this stuff is findable.

Regards.


Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 11:08:23 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> Hi Micheal,
> 
> On Wed, 18 Jan 2017 10:26:43 -0600
> "Karl O. Pinc" <k...@meme.com> wrote:
> 
> > > v26 patch attached which fixes this.
> 
> I was glancing over the changes to the documentation
> you made between the v22 and v25

If it were me I'd have the documentation mention
that the pg_current_logfiles() result is
supplied on a "best effort" basis and under
rare conditions may be outdated.   The
sentence in the pg_current_logfles() docs
which reads "pg_current_logfiles reflects the contents 
of the file current_logfiles." now carries little
meaning because the current_logfiles docs no
longer mention that the file content may be outdated.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Micheal,

On Wed, 18 Jan 2017 10:26:43 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> > v26 patch attached which fixes this.  

I was glancing over the changes to the documentation
you made between the v22 and v25 and from looking at the diffs 
it seems the format of the current_logfiles file content is no longer
documented.  Seems to me that the file format should
be documented if there's any intention that the end user
look at or otherwise use the file's content.

It's fine with me if the content of current_logfiles
is supposed to be internal to PG and not exposed
to the end user.  I'm writing to make sure that
this is a considered decision.

I also see that all the index entries in the docs
to the current_logfiles file were removed.  (And
I think maybe some index entries to various places
where pg_current_logfiles() was mentioned in the docs.)
I see no reason why we shouldn't have more rather
than fewer index entries in the docs.

I haven't made any sort of though review of your
changes to the docs but this jumped out at me
and I wanted to comment before the patches got
passed on to the committers.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 10:11:20 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> You must write
>   errcode(ERRCODE_INTERNAL_ERROR)
> instead of just
>   ERRCODE_INTERNAL_ERROR
> 
> If you don't you get back 01000 for the error code.

> v26 patch attached which fixes this.

Attached are revised versions of the 2 (optional) patches which
make hardcoded constants into symbols to apply on
top of the v26 patch.


Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v26.diff.abstract_guc_part1
Description: Binary data


patch_pg_current_logfile-v26.diff.abstract_guc_part2
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 13:26:46 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:

> On Wed, Jan 18, 2017 at 11:36 AM, Karl O. Pinc <k...@meme.com> wrote:
> > On Wed, 18 Jan 2017 11:08:25 +0900
> > Michael Paquier <michael.paqu...@gmail.com> wrote:
> >  
> >> Our ideas rather map here, ERRCODE_INTERNAL_ERROR would be adapted
> >> for this situation. Do any of you want to give it a shot or should
> >> I?  

> What do you think about that?
> +   log_filepath = strchr(lbuffer, ' ');
> +   if (log_filepath == NULL)
> +   {
> +   /*
> +* Corrupted data found, return NULL to the caller and
> still
> +* inform him on the situation.
> +*/
> +   ereport(WARNING,
> +   (ERRCODE_INTERNAL_ERROR,
> +errmsg("corrupted data found in \"%s\"",
> +   LOG_METAINFO_DATAFILE)));
> +   break;
> +   }

You must write

  errcode(ERRCODE_INTERNAL_ERROR)

instead of just
  
  ERRCODE_INTERNAL_ERROR

If you don't you get back 01000 for the error code.
Test by using psql with '\set VERBOSITY verbose'.

v26 patch attached which fixes this.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c..3188496 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4248,6 +4248,25 @@ SELECT * FROM parent WHERE key = 2400;
  must be enabled to generate
 CSV-format log output.

+   
+When either stderr or
+csvlog are included, the file
+current_logfiles gets created and records the location
+of the log file(s) currently in use by the logging collector and the
+associated logging destination. This provides a convenient way to
+find the logs currently in use by the instance. Here is an example of
+contents of this file:
+
+stderr pg_log/postgresql.log
+csvlog pg_log/postgresql.csv
+
+Note that this file gets updated when a new log file is created
+as an effect of rotation or when log_destination is
+reloaded.  current_logfiles is removed when
+stderr and csvlog
+are not included in log_destination or when the
+logging collector is disabled.
+   
 

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index eb1b698..8524dff 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15659,6 +15666,34 @@ SET search_path TO schema , schema, ..

 

+pg_current_logfile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of the log file(s) currently in use by the logging collector.
+The path includes the  directory
+and the log file name.  Log collection must be enabled or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply, as text,
+either csvlog or stderr as the value of the
+optional parameter. The return value is NULL when the
+log format requested is not a configured
+.
+pg_current_logfiles reflects the contents of the
+file current_logfiles.
+   
+
+   
 pg_my_temp_schema

 
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..388ed34 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -61,6 +61,12 @@ Item
 
 
 
+ current_logfiles
+ File recording the log file(s) currently written to by the logging
+  collector
+
+
+
  global
  Subdirectory containing cluster-wide tables, such as
  pg_database
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 31aade1..1d7f68b 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1055,6 +1055,7 @@ REVOKE EXECUTE ON FUNCTION pg_xlog_replay_pause() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_xlog_replay_resume() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_rotate_logfile() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_reload_conf() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_cu

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 11:08:25 +0900
Michael Paquier  wrote:

> Our ideas rather map here, ERRCODE_INTERNAL_ERROR would be adapted for
> this situation. Do any of you want to give it a shot or should I?

You're welcome to it.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 17 Jan 2017 23:00:43 +0100
Gilles Darold <gilles.dar...@dalibo.com> wrote:

> Le 17/01/2017 à 19:58, Karl O. Pinc a écrit :
> > On Tue, 17 Jan 2017 19:06:22 +0100
> > Gilles Darold <gilles.dar...@dalibo.com> wrote:
> >  
> >> Le 17/01/2017 à 03:22, Michael Paquier a écrit :  
> >>> On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc <k...@meme.com>
> >>> wrote:
> >>>> On January 15, 2017 11:47:51 PM CST, Michael Paquier
> >>>> <michael.paqu...@gmail.com> wrote:
> >  
> >>>>> Also, I would rather see an ERROR returned to the user in case
> >>>>> of bad data in current_logfiles, I did not change that either as
> >>>>> that's the original intention of Gilles.
> >> I'm not against adding a warning or error message here even if it
> >> may never occurs, but we need a new error code as it seems to me
> >> that no actual error code can be used.
> > Seems to me the XX001 "data_corrupted" sub-category of
> > XX000 "internal_error" is appropriate.  
> 
> I don't think so, this file doesn't contain any data and we must not
> report such error in this case. Somethink like "bad/unknow file
> format" would be better.

Maybe.  It's not user-supplied data that's corrupted but it is
PG generated data which is generated for and supplied to the user.
I just looked at all uses of XX001 and it is true that they all
involve corruption of user-supplied data.

If you don't want to use XX001 use XX000.  It does not seem worth
making a new error code for just this one case.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 17 Jan 2017 19:06:22 +0100
Gilles Darold <gilles.dar...@dalibo.com> wrote:

> Le 17/01/2017 à 03:22, Michael Paquier a écrit :
> > On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc <k...@meme.com>
> > wrote:  
> >> On January 15, 2017 11:47:51 PM CST, Michael Paquier
> >> <michael.paqu...@gmail.com> wrote:  


> >>> Also, I would rather see an ERROR returned to the user in case of
> >>> bad data in current_logfiles, I did not change that either as
> >>> that's the original intention of Gilles.  

> I'm not against adding a warning or error message here even if it may
> never occurs, but we need a new error code as it seems to me that no
> actual error code can be used.  

Seems to me the XX001 "data_corrupted" sub-category of
XX000 "internal_error" is appropriate.

https://www.postgresql.org/docs/devel/static/errcodes-appendix.html

As a dbadmin/sysadm I'd defiantly like to see something in the log if
you're going to raise anything user-side.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 17 Jan 2017 07:58:43 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> On Tue, 17 Jan 2017 11:22:45 +0900
> Michael Paquier <michael.paqu...@gmail.com> wrote:
> 
> > On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc <k...@meme.com>
> > wrote:  
> 
> > >>Also, I would rather see an ERROR returned to the user in case of
> > >>bad data in current_logfiles,

> > 
> > Hm... OK. At the same time not throwing at least a WARNING is
> > confusing

What I find a little disconcerting is that there'd be nothing in the
logs.



Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 17 Jan 2017 11:22:45 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:

> On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc <k...@meme.com> wrote:

> >>Also, I would rather see an ERROR returned to the user in case of
> >>bad data in current_logfiles, I did not change that either as
> >>that's the original intention of Gilles.  
> >
> > I could be wrong but I seem to recall that Robert recommended
> > against an error message. If there is bad data then something is
> > really wrong, up to some sort of an attack on the back end. Because
> > this sort of thing simply shouldn't happen it's enough in my
> > opinion to guard against buffer overruns etc and just get on with
> > life. If something goes unexpectedly and badly wrong with internal
> > data structures in general there would have to be all kinds of
> > additional code to cover every possible problem and that doesn't
> > seem to make sense.  
> 
> Hm... OK. At the same time not throwing at least a WARNING is
> confusing, because a NULL result is returned as well even if the input
> method is incorrect and even if the method is correct but it is not
> present in current_logfiles. As the user is thought as a trusted user
> as it has access to this function, I would think that being verbose on
> the error handling, or at least warnings, would make things easier to
> analyze.

These are all valid points.

In the context of reliability it's worth noting that
pg_current_logfile() results are inherently unreliable.
If the OS returns ENFILE or EMFILE when opening the current_logfiles
file (but not previously) the content, and so pg_current_logfile()
results, will be outdated until the next logfile rotation.
You wouldn't expect this to happen, but it might.  Which
is similar to the situation where the content of the current_logfiles
is corrupted; very unexpected but possible.


Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On January 15, 2017 11:47:51 PM CST, Michael Paquier 
<michael.paqu...@gmail.com> wrote:
>On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc <k...@meme.com> wrote:
>
>
>> Attached also are 2 patches which abstract some hardcoded
>> constants into symbols.  Whether to apply them is a matter
>> of style and left to the maintainers, which is why these
>> are separate patches.
>
>Making the strings csvlog, stderr and eventlog variables? Why not
>because the patch presented here uses them in new places. Now it is
>not like it is a huge maintenance burden to keep them as strings, so I
>would personally not bother much.

To my mind it's a matter readability. It is useful to be able to search for or 
identify quickly when reading, e. g., all the places where the keyword stderr 
relates to output log destination and not some other more common use. The 
downside is it is more code...


>OK. I have done a round of hands-on review on this patch, finishing
>with the attached. In the things I have done:

Thank you.

>With all those issues fixed, I finish with the attached, that I am
>fine to pass down to a committer. I still think that this should be
>only one function using a SRF with rows shaped as (type, path) for
>simplicity, but as I am visibly outnumbered I won't insist further.

That also makes a certain amount of sense to me but I can't say I have thought 
deeply about it. Haven't paid any attention to this issue and am fine letting 
things move forward as is.

>Also, I would rather see an ERROR returned to the user in case of bad
>data in current_logfiles, I did not change that either as that's the
>original intention of Gilles.

I could be wrong but I seem to recall that Robert recommended against an error 
message. If there is bad data then something is really wrong, up to some sort 
of an attack on the back end. Because this sort of thing simply shouldn't 
happen it's enough in my opinion to guard against buffer overruns etc and just 
get on with life. If something goes unexpectedly and badly wrong with internal 
data structures in general there would have to be all kinds of additional code 
to cover every possible problem and that doesn't seem to make sense.

Sorry about the previous email with empty content. My email client got away 
from me.


Karl <k...@meme.com>
Free  Software:  "You don't pay back you pay forward."
-- Robert A. Heinlein



-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On January 15, 2017 11:47:51 PM CST, Michael Paquier 
<michael.paqu...@gmail.com> wrote:
>On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc <k...@meme.com> wrote:
>> I do have a question here regards code formatting.
>> The patch now contains:
>>
>> if (log_filepath == NULL)
>> {
>> /* Bad data. Avoid segfaults etc. and return NULL to caller.
>*/
>> break;
>> }
>>
>> I'm not sure how this fits in with PG coding style,
>> whether the {} should be removed or what.  I've looked
>> around and can't find an example of an if with a single
>> line then block and a comment.  Maybe this means that
>> I shouldn't be doing this, but if I put the comment above
>> the if then it will "run into" the comment block which
>> immediately precedes the above code which describes
>> a larger body of code.  So perhaps someone should look
>> at this and tell me how to improve it.
>
>Including brackets in this case makes a more readable code. That's the
>pattern respected the most in PG code. See for example
>XLogInsertRecord():
>else
>{
>/*
>  * This was an xlog-switch record, but the current insert location was
>  * already exactly at the beginning of a segment, so there was no need
> * to do anything.
> */
>}
>
>> Attached also are 2 patches which abstract some hardcoded
>> constants into symbols.  Whether to apply them is a matter
>> of style and left to the maintainers, which is why these
>> are separate patches.
>
>Making the strings csvlog, stderr and eventlog variables? Why not
>because the patch presented here uses them in new places. Now it is
>not like it is a huge maintenance burden to keep them as strings, so I
>would personally not bother much.
>
>> The first patch changes only code on the master
>> branch, the 2nd patch changes the new code.
>
>Thanks for keeping things separated.
>
>> I have not looked further at the patch or checked
>> to see that all changes previously recommended have been
>> made.  Michael, if you're confident that everything is good
>> go ahead and move the patchs forward to the maintainers.  Otherwise
>> please let me know and I'll see about finding time
>> for further review.
>
>OK. I have done a round of hands-on review on this patch, finishing
>with the attached. In the things I have done:
>- Elimination of useless noise diff
>- Fixes some typos (logile, log file log, etc.)
>- Adjusted docs.
>- Docs were overdoing in storage.sgml. Let's keep the description
>simple.
>- Having a paragraph at the beginning of "Error Reporting and Logging"
>in config.sgml does not look like a good idea to me. As the updates of
>current_logfiles is associated with log_destination I'd rather see
>this part added in the description of the GUC.
>- Missed a (void) in the definition of update_metainfo_datafile().
>- Moved update_metainfo_datafile() before the signal handler routines
>in syslogger.c for clarity.
>- The last "return" is useless in update_metainfo_datafile().
>- In pg_current_logfile(), it is useless to call PG_RETURN_NULL after
>emitting an ERROR message.
>- Two calls to FreeFile(fd) have been forgotten in
>pg_current_logfile(). In one case, errno needs to be saved beforehand
>to be sure that the correct error string is generated for the user.
>- A portion of 010_pg_basebackup.pl was not updated.
>- Incorrect header ordering in basebackup.c.
>- Decoding of CR and LF characters seem to work fine when
>log_file_name include some.
>
>With all those issues fixed, I finish with the attached, that I am
>fine to pass down to a committer. I still think that this should be
>only one function using a SRF with rows shaped as (type, path) for
>simplicity, but as I am visibly outnumbered I won't insist further.
>Also, I would rather see an ERROR returned to the user in case of bad
>data in current_logfiles, I did not change that either as that's the
>original intention of Gilles.
>-- 
>Michael


Karl <k...@meme.com>
Free  Software:  "You don't pay back you pay forward."
-- Robert A. Heinlein

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sun, 15 Jan 2017 07:20:40 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> On Sun, 15 Jan 2017 14:54:44 +0900
> Michael Paquier <michael.paqu...@gmail.com> wrote:
> 
> > I think that's all I have for this patch.  

> I'd like to submit with it an addendum patch that
> makes symbols out of some constants.  I'll see if I can
> get that done soon.

Attached is a version 23 of the patch.  The only change
is follow-through and cleanup of code posted in-line in emails.

  src/backend/utils/adt/misc.c | 13 ++---
  1 file changed, 6 insertions(+), 7 deletions(-)

This includes making comments into full sentences.


I do have a question here regards code formatting.
The patch now contains:

if (log_filepath == NULL)
{
/* Bad data. Avoid segfaults etc. and return NULL to caller. */
break;
}

I'm not sure how this fits in with PG coding style,
whether the {} should be removed or what.  I've looked
around and can't find an example of an if with a single
line then block and a comment.  Maybe this means that
I shouldn't be doing this, but if I put the comment above
the if then it will "run into" the comment block which
immediately precedes the above code which describes
a larger body of code.  So perhaps someone should look
at this and tell me how to improve it.


Attached also are 2 patches which abstract some hardcoded
constants into symbols.  Whether to apply them is a matter
of style and left to the maintainers, which is why these
are separate patches.  

The first patch changes only code on the master
branch, the 2nd patch changes the new code.


I have not looked further at the patch or checked
to see that all changes previously recommended have been
made.  Michael, if you're confident that everything is good
go ahead and move the patchs forward to the maintainers.  Otherwise
please let me know and I'll see about finding time
for further review.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c..c44984d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4211,6 +4211,17 @@ SELECT * FROM parent WHERE key = 2400;
  server log
 
 
+
+ When logs are written to the file system, the  file includes the type,
+ the location and the name of the logs currently in use. This provides a
+ convenient way to find the logs currently in used by the instance.
+
+$ cat $PGDATA/current_logfiles
+stderr pg_log/postgresql-2017-01-13_085812.log
+
+
+
 
  Where To Log
 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e3186..693669b 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15658,6 +15665,39 @@ SET search_path TO schema , schema, ..
 the time when the postmaster process re-read the configuration files.)

 
+
+pg_current_logile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of either the csv or stderr log file currently in use by the
+logging collector.  This is a path including the
+ directory and the log file name.
+Log collection must be active or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered
+list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply,
+as text, either csvlog
+or stderr as the value of the optional parameter. The
+return value is NULL when the log format requested
+is not a configured .
+
+pg_current_logfiles reflects the content of the
+ file.  All caveats
+regards current_logfiles content are applicable
+to pg_current_logfiles' return value.
+   
+

 pg_my_temp_schema

diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..3ce2a7e 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -60,6 +60,38 @@ Item
  Subdirectory containing per-database subdirectories
 
 
+
+ 
+  
+   current_logfiles
+  
+  
+   Logging
+   current_logfiles file
+  
+  current_logfiles
+ 
+ 
+  
+  A file recording the log file(s) currently written to by the syslogger
+  and the file's log formats, stderr
+  or csvlog.  Each line of the file is a space separated l

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sun, 15 Jan 2017 14:54:44 +0900
Michael Paquier  wrote:

> I think that's all I have for this patch.

I'd like to submit with it an addendum patch that
makes symbols out of some constants.  I'll see if I can
get that done soon.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 12 Jan 2017 13:14:28 +0100
Gilles Darold  wrote:

> My bad, I was thinking that Karl has planned an update of the patch in
> his last post, sorry for my misunderstanding.

I was, but have been swept along by events and not gotten to it.



Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

I don't plan to be able to get back to this patch until late
this week or early next week.  My plan is to then go though
the whole thing and fix everything I can find.  If we're both
working at the same time this could lead to wasted effort
so I will write as soon as I start work and if you are working
at the same time I'll send smaller hunks.

By the by, my last email contained some stupidity in it's
code suggestion because it is structured like:

while
  if (...)
do something;
  else
break;
  do more...;

Stupid to have an if/else construct.

while
  if (!...)
break;
  do something;
  do more...;

Is cleaner.  Apologies if my coding out loud is confusing things.
I'll fix this in the next go-round if you don't get to it first.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sat, 10 Dec 2016 19:41:21 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> On Fri, 9 Dec 2016 23:36:12 -0600
> "Karl O. Pinc" <k...@meme.com> wrote:
> 
> > Instead I propose (code I have not actually executed):
> > ...
> > charlbuffer[MAXPGPATH];
> > char*log_format = lbuffer;
> > ...
> > 
> > /* extract log format and log file path from the line */
> > log_filepath = strchr(lbuffer, ' ');  /* lbuffer == log_format
> > */ *log_filepath = '\0'; /* terminate log_format */
> > log_filepath++;   /* start of file path */
> > log_filepath[strcspn(log_filepath, "\n")] = '\0';  
> 
> Er, I guess I prefer the more paranoid, just because who knows
> what might have manged to somehow write the file that's read
> into lbuffer:
> 
> ...
> charlbuffer[MAXPGPATH];
> char*log_format = lbuffer;
> ...
> 
> /* extract log format and log file path from the line */
> if (log_filepath = strchr(lbuffer, ' ')) /* lbuffer == log_format
> */ *log_filepath = '\0';/* terminate log_format */
> log_filepath++;  /* start of file path */
> log_filepath[strcspn(log_filepath, "\n")] = '\0';

*sigh*


...
charlbuffer[MAXPGPATH];
char*log_format = lbuffer;
...

/* extract log format and log file path from the line */
/* lbuffer == log_format, they share storage */
if (log_filepath = strchr(lbuffer, ' '))
*log_filepath = '\0';   /* terminate log_format */
else
{
/* Unknown format, no space. Return NULL to caller. */
lbuffer[0] = '\0';
break;
}
log_filepath++;  /* start of file path   */
log_filepath[strcspn(log_filepath, "\n")] = '\0';


> The file read is, of course, normally written by postgres.  But
> possibly writing to unintended memory locations, even virtual address
> NULL, does not seem good.
> 
> Any feedback from more experienced PG developers as how to best handle
> this case would be welcome.
> 
> Regards,
> 
> Karl <k...@meme.com>
> Free Software:  "You don't pay back, you pay forward."
>  -- Robert A. Heinlein
> 




Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Fri, 9 Dec 2016 23:36:12 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> Instead I propose (code I have not actually executed):
> ...
> charlbuffer[MAXPGPATH];
> char*log_format = lbuffer;
> ...
> 
> /* extract log format and log file path from the line */
> log_filepath = strchr(lbuffer, ' ');  /* lbuffer == log_format */
> *log_filepath = '\0'; /* terminate log_format */
> log_filepath++;   /* start of file path */
> log_filepath[strcspn(log_filepath, "\n")] = '\0';

Er, I guess I prefer the more paranoid, just because who knows
what might have manged to somehow write the file that's read
into lbuffer:

...
charlbuffer[MAXPGPATH];
char*log_format = lbuffer;
...

/* extract log format and log file path from the line */
if (log_filepath = strchr(lbuffer, ' ')) /* lbuffer == log_format */
*log_filepath = '\0';/* terminate log_format */
log_filepath++;  /* start of file path */
log_filepath[strcspn(log_filepath, "\n")] = '\0';

The file read is, of course, normally written by postgres.  But possibly
writing to unintended memory locations, even virtual address NULL, does
not seem good.

Any feedback from more experienced PG developers as how to best handle
this case would be welcome.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Fri, 9 Dec 2016 23:41:25 +0100
Gilles Darold  wrote:

>   /* extract log format and log file path from the line */
>   log_filepath = strchr(lbuffer, ' ');
>   log_filepath++;
>   lbuffer[log_filepath-lbuffer-1] = '\0';
>   log_format = lbuffer;
>   *strchr(log_filepath, '\n') = '\0';

Instead I propose (code I have not actually executed):
...
charlbuffer[MAXPGPATH];
char*log_format = lbuffer;
...

/* extract log format and log file path from the line */
log_filepath = strchr(lbuffer, ' ');  /* lbuffer == log_format */
*log_filepath = '\0'; /* terminate log_format */
log_filepath++;   /* start of file path */
log_filepath[strcspn(log_filepath, "\n")] = '\0';


My first thought was to follow your example and begin with
log_format = lbuffer;
But upon reflection I think changing the declaration of log_format
to use an initializer better expresses how storage is always shared.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 8 Dec 2016 11:27:34 -0500
Robert Haas <robertmh...@gmail.com> wrote:

> On Wed, Dec 7, 2016 at 11:02 PM, Karl O. Pinc <k...@meme.com> wrote:
> > I read this and knew that I hadn't finished review, but didn't
> > immediately respond because I thought the patch had to be
> > marked "ready for committer" on commitfest.postgresql.org
> > before the committers would spend a lot of time on it.  
> 
> Generally that's true, but I was trying to be helpful and trying also
> to move this toward some kind of conclusion.

It has been very helpful, particularly your last reply.  And I think
there's now a clear path forward.

(I'm not looking for any replies to the below, although of course would
welcome whatever you've got to say.)

> It is, of course, not my job to decide who is at fault in whatever may
> or may not have gone wrong during the reviewing process.

Understood.  And I'm not looking for any sort of judgment or
attempting to pass judgment.  It has been frustrating though
and your email provided an opportunity to vent, and to
provide some feedback, of whatever quality, to the review
process.

It's been that much more frustrating because I don't really
care one way or another about the feature.  I was just trying
to build up credit reviewing somebody else's patch, and instead
probably did the opposite with all the thrashing.  :)

>  I do think that the blizzard of small patches that you've
> submitted in some of your emails may possibly be a bit overwhelming.
> We shouldn't really need a stack of a dozen or more patches to
> implement one small feature.  Declarative partitioning only had 7.
> Why does this need more than twice that number?

I'm trying to break up changes into patches which are as small
as possible to make them more easily understood by providing
a guide for the reader/reviewer.  So rather than
a single patch I'd make, say, 3.  One for documentation to describe
the change.  Another which does whatever refactoring is necessary
to the existing code, but which otherwise does not introduce any
functional changes.  And another which adds the new code which makes
use of the refactoring.  At each stage the code should continue
to work without bugs.  The other party can then decide to incorporate
the patchs into the main patch, which is itself another attached
patch.

Do this for several unrelated changes to the main patch and the patches
add up.

Mostly I wouldn't expect various patches to be reviewed by the
committers, they'd get incorporated into the main patch.

This is how I break down the work when I look at code changes.
What's it do?  What does it change/break in the existing code?
How does the new stuff work?  But this process has not worked
here so I guess I'll stop.

But I expect you will see at least 3 patches submitted for
committer review.  I see a number of hardcoded constants,
now that the main patch adds additional code, that can
be made into symbols to, IMO, improve code clarity.
Guiles points out that this is an issue of coding style
and might be considered unnecessary complication.
So they are not in the main patch.  They are attached
(applied to v14 of the main patch; really, the first applies
to the master PG branch and the 2nd to v14 of the
pg_current_logfiles patch) if you want to look
at them now and provide some guidance as to whether they
should be dropped or included in the patch.

> > The extreme case is the attached "cleanup_rotate" patch.
> > (Which applies to v14 of this patch.)  It's nothing but
> > a little bit of tiding up of the master branch,

> I took a look at that patch just now and I guess I don't really see
> the point.

The point would be to make the code easier to read.  Saving cycles
is hardly ever worthwhile in my opinion.  The whole point
of code is to be read by people and be understandable.
If it's not understood it's useless going forward.

Once I've gone to the effort to understand something
written, that I'm going to be responsible for maintaining, I like to
see it written as clearly as possible.  In my experience
if you don't do this then little confusions multiply and
eventually the whole thing turns to mud.

The trade-off, as you say, is the overhead involved
in running minor changes through the production
process.  I figure this can be minimized by bundling
such changes with related substantial changes.

Again, it's not working so I'll drop it.

> > FYI.  The point of the "retry_current_logfiles"
> > patch series is to handle the case
> > where logfile_writename gets a ENFILE or EMFILE.
> > When this happens the current_logfiles file can
> > "skip" and not contain the name(s) of the current
> > log file for an entire log rotation.  To miss, say,
> > a month of logs because the logs only rotate monthly
> > and your log processing is based on reading the
>

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hello Robert,

On Wed, 7 Dec 2016 18:52:24 -0500
Robert Haas  wrote:

> On Tue, Dec 6, 2016 at 6:11 PM, Gilles Darold
>  wrote:
> > It seems that all fixes have been included in this patch.  

I read this and knew that I hadn't finished review, but didn't
immediately respond because I thought the patch had to be
marked "ready for committer" on commitfest.postgresql.org
before the committers would spend a lot of time on it.

I don't have the time to fully respond, or I'd be able to
attach new code, but want to comment before anybody else
spends a lot of time on this patch.

> Yikes.  This patch has certainly had a lot of review, but it has lots
> of basic inconsistencies with PostgreSQL coding style, which one would
> think somebody would have noticed by now,

Yes and no.  I don't really know what the coding style is and
rather than have to make multiple corrections to code that might
get weeded out and discarded I've been focusing on getting the logic
right.  Really, I've not put a thought to it except for trying to write
what I write like what's there, and sticking to < 80 chars per line.

There has been lots of review.
The only truly effective way I've found to communicate regards
the pg_current_logfiles patch has been to write code and provide
detailed test cases.  I could be wrong, but unless I submit
code I don't feel like I've been understood.
I just haven't been interested in re-formatting
somebody else's code before I think the code really works.

>  I don't have any objection
> to the basic idea of this patch, but it's got to look and feel like
> the surrounding PostgreSQL code.  And not be unnecessarily
> complicated.

I've been introducing some complication, but I hope it's necessary.
To keep the review process simple my plan has been to submit
multiple patches.  One with the basics and more on top of that
that introduce complication that can be considered and accepted or
rejected.  So I send emails with multiple patches, some that I think
need to go into the core patch and others to be kept separate.
But, although I try, this does not seem to have been communicated
because I keep getting emails back that contain only a single patch.

Maybe something's wrong with my review process but I don't know
how to fix it.

If you think a single patch is the way to go I can open up
separate tickets at commitfest.postgresql.org.  But this seems
like overkill because all the patches touch the same code.

The extreme case is the attached "cleanup_rotate" patch.
(Which applies to v14 of this patch.)  It's nothing but
a little bit of tiding up of the master branch, but does
touch code that's already being modified so it seems
like the committers would want to look at it at the same
time as when reviewing the pg_current_logfile patch.  
Once you've looked at the pg_current_logfile patch
you've already looked at and modified code in the function
the "cleanup_rotate" patch touches.

But the "cleanup_rotate" patch got included
in the v15 version of the patch and is also in v16.
I'm expecting to submit it as a separate patch
along with the pg_current_logfile patch and tried
to be very very clear about this.  It
really has nothing to do with the pg_current_logfile
functionality.  (And is the only "separate" patch
which really has nothing to do with the pg_current_logfile
functionality.)

More examples of separate patches are below.  Any guidance
would be appreciated.

> 
> Detailed comments below:

> rm_log_metainfo() could be merged into logfile_writename(), since
> they're always called in conjunction and in the same pattern. 

This would be true, if it weren't for the attached
"retry_current_logfiles" patches that were applied in
v15 of the patch and removed from v16 due to some
mis-communication I don't understand.

(But the attached patches apply on top of the the v14 patch.
I don't have time to refactor them now.)

FYI.  The point of the "retry_current_logfiles"
patch series is to handle the case
where logfile_writename gets a ENFILE or EMFILE.
When this happens the current_logfiles file can
"skip" and not contain the name(s) of the current
log file for an entire log rotation.  To miss, say,
a month of logs because the logs only rotate monthly
and your log processing is based on reading the
current_logfiles file sounds like a problem.

What I was attempting to communicate in my email response
to the v15 patch is that the (attached, but applies to the
v14 patch again) "backoff" patch should be submitted
as a separate patch.  It seems over-complicated, but
exists in case a committer is worried about re-trying
writes on a system that's busy enough to generate ENFILE
or EMFILE errors.


> +if (errno == ENFILE || errno == EMFILE)
> +ereport(LOG,
> +(errmsg("system is too busy to write logfile meta
> info, %s will be updated on next rotation (or use SIGHUP to retry)",
> LOG_METAINFO_DATAFILE)));
> 
> This seems like a totally 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> I've attached the v15 of the patch 

> I've not applied patch patch_pg_current_logfile-v14.diff.backoff to
> prevent constant call of logfile_writename() on a busy system (errno =
> ENFILE | EMFILE).

I don't think it should be applied and included in the basic
functionality patch in any case. I think it needs to be submitted as a
separate patch along with the basic functionality patch.  Backing off
the retry of the current_logfiles write could be overly fancy and
simply not needed.

> I think this can be done quite simply by testing if
> log rotate is still enabled. This is possible because function
> logfile_rotate() is already testing if  errno = ENFILE | EMFILE and in
> this case rotation_disabled is set to true. So the following test
> should do the work:
> 
>if (log_metainfo_stale && !rotation_disabled)
>logfile_writename();
> 
> This is included in v15 patch.

I don't see this helping much, if at all.

First, it's not clear just when rotation_disabled can be false
when log_metainfo_stale is true.  The typical execution
path is for logfile_writename() to be called after rotate_logfile()
has already set rotataion_disabled to true.   logfile_writename()
is the only place setting log_metainfo_stale to true and
rotate_logfile() the only place settig rotation_disabled to false.

While it's possible
that log_metainfo_stale might be set to true when logfile_writename()
is called from within open_csvlogfile(), this does not help with the
stderr case.  IMO better to just test for log_metaifo_stale at the
code snippet above.

Second, the whole point of retrying the logfile_writename() call is
to be sure that the current_logfiles file is updated before the logs
rotate.  Waiting until logfile rotation is enabled defeats the purpose.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> I've attached the v15 of the patch that applies your changes/fixes ...

Per the following:

On Mon, 21 Nov 2016 13:17:17 -0500
Robert Haas  wrote:

> It would really be much better to submit anything that's not
> absolutely necessary for the new feature as a separate patch, rather
> than bundling things together.

The following patch should really be submitted (along with
your patch) as a separate and independent patch:

patch_pg_current_logfile-v14.diff.cleanup_rotate

It introduces no new functionality and only cleans up existing code.
The idea is to give the maintainers only one thing to consider
at a time.  It can be looked at along with your patch since it
touches the same code but, like the GUC symbol patch, it's
not a necessary part of your patch's functionality.

At present patch_pg_current_logfile-v14.diff.cleanup_rotate is bundled
in with the v15 patch.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> I've attached the v15 of the patch that applies your changes/fixes and
> remove the call to strtok().

I like the idea of replacing the strtok() call with sscanf()
but I see problems.

It won't work if log_filename begins with a space because
(the docs say) that a space in the sscanf() format represents
any amount of whitespace.

As written, there's a potential buffer overrun in log_format.
You could fix this by making log_format as large as lbuffer,
but this seems clunky.

Regards,


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> Again, patches patch_pg_current_logfile-v14.diff.doc_linux_default-v2
> have not been included because I don't see any reason to talk
> especially about systemd. If you talk about systemd you must talk
> about other stderr handler by all systems. IMO saying that
> current_logfile is present only if logging_collector is enabled and
> log_destination include stderr or/and csvlog is enough, no need to
> talk about systemd and behavior of Linux distributions.

Fair enough.  And I'd sooner not talk about systemd or other such
specifics too.

The concern I'm attempting to address is that the patch touts
the current_logfiles file in the section on logging without
reservation.  But anyone compiling from source or using most
pre-built Linux binaries won't have the file. (And the
pg_current_logfiles() function won't work either.)

Maybe the answer is to  change

When logs are written to the file-system ...

to

When the PostgreSQL backend
database server write logs to the file-system (which is often
not the default configuration) ...

?

Or something.  This also seems verbose, yet incomplete because
although it mentions that the default is to not have the backend
write logs it does not say anything about where to look to change
the default.

The thing is, on most default setups you do get logs written to the
filesystem, but they are not written by the PG backend.

I'll let you (or anyone else who might be concerned) move this forward
because I don't seem to have a good answer.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 23 Nov 2016 23:08:18 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> On Wed, 23 Nov 2016 03:21:05 -0600
> "Karl O. Pinc" <k...@meme.com> wrote:
> 
> > On Sat, 19 Nov 2016 12:58:47 +0100
> > Gilles Darold <gilles.dar...@dalibo.com> wrote:
> >   
> > > ... attached v14 of the patch. 
> 
> > patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3  
> 
> > Re-try the write of current_logfiles should it fail because the
> > system is too busy.  

> ---
> 
> patch_pg_current_logfile-v14.diff.doc_linux_default
> 
> Applies on top of
> patch_pg_current_logfile-v14.diff.pg_ctl_and_more_docs
> 
> Mentions, in the body of the docs, defaults and their impact on
> current_logfiles and pg_current_logfiles.  It seems appropriate
> to mention this in the main documentation and in the overall context
> of logging.

Attached is version 2 of this patch.  Adds an index entry.

patch_pg_current_logfile-v14.diff.doc_linux_default-v2

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
 top of
> p

patch_pg_current_logfile-v14.diff.doc_linux_default-v2
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 23 Nov 2016 03:21:05 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> On Sat, 19 Nov 2016 12:58:47 +0100
> Gilles Darold <gilles.dar...@dalibo.com> wrote:
> 
> > ... attached v14 of the patch.   

> patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3

> Re-try the write of current_logfiles should it fail because the
> system is too busy.

Attached are 2 more doc patchs.

---

patch_pg_current_logfile-v14.diff.pg_ctl_and_more_docs

Applies on top of
patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3

Mentions pg_ctl and how using it might affect whether logfile
paths are captured in the current_logfiles file.  I think
the default RedHat packaging captures logs this way.  The
Debian default is to ship with logging_collector=off
and to rely on pg_ctrl via pg_ctlcluster to manage
log writing.  Both RH and Debian then pass stderr to systemd
and that does log management.  I don't
recall other distros' practice.  If the default distro
packaging means that current_logfiles/pg_current_logfile()
"don't work" this could be an issue to address in the
documentation.  Certainly the systemd reliance on stderr
capture and it's subsuming of logging functionality could
also be an issue.

Cross reference the current_logfiles file in the pg_current_logfile()
docs.

Marks up stderr appropriately.

Adds index entries.

---

patch_pg_current_logfile-v14.diff.doc_linux_default

Applies on top of
patch_pg_current_logfile-v14.diff.pg_ctl_and_more_docs

Mentions, in the body of the docs, defaults and their impact on
current_logfiles and pg_current_logfiles.  It seems appropriate
to mention this in the main documentation and in the overall context
of logging.

Adds index entries.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v14.diff.pg_ctl_and_more_docs
Description: Binary data


patch_pg_current_logfile-v14.diff.doc_linux_default
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 23 Nov 2016 10:39:28 -0500
Tom Lane <t...@sss.pgh.pa.us> wrote:

> Robert Haas <robertmh...@gmail.com> writes:
> > On Wed, Nov 23, 2016 at 4:21 AM, Karl O. Pinc <k...@meme.com>
> > wrote:  
> >> The bool types on the stack in logfile_rotate() are
> >> my work.  Bools on the stack don't make sense as far
> >> as hardware goes, so the compiler's optimizer should change
> >> them to int.  This patch changes the bools to ints
> >> should that be to someone's taste.  
> 
> > That does not seem like a good idea from here.  Even if it buys some
> > microscopic speedup, in a place like this it won't matter.  It's
> > more important that the code be clear, and using an int where you
> > really intend a bool isn't an improvement as far as clarity goes.  
> 
> Not to mention that the "bools on the stack don't make sense" premise
> is bogus anyway.

Thanks.  I don't think I've paid attention since 8088 days, just always
used bools.  We'll drop that patch then.

But this reminds me.  I should have used a bool return value.

Attached is a version 2 of
patch_pg_current_logfile-v14.diff.deletion-v2


Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v14.diff.deletion-v2
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 23 Nov 2016 03:21:05 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> But also, you can't use strtok() to parse lbuffer because
> the path you're returning can contain a space. 

Maybe on the 2nd call to strtok() you could pass ""
as the 2nd argument?  That'd be a little wonky but
the man page does not say you can't have an empty
set of delimiters.  On the other hand strtok() is
not a perfect choice, you don't want to "collapse"
adjacent delimiters in the parsed string or ignore
leading spaces.  I'd prefer a strstr() solution.

Or strchr() even better.

Regards.

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Sat, 19 Nov 2016 12:58:47 +0100
Gilles Darold  wrote:

> ... attached v14 of the patch. 

Attached are patches for your consideration and review.
(Including your latest v14 patch for completeness.)

Some of the attached patches (like the GUC symbol
patch you've seen before) are marked to be submitted
as separate patches to the maintainers when we send
them code for review.  These need looking over by
somebody, I hope you, before they get sent on so
please comment on these if you're not going to look
at them or if you see a problem with them.   (Or if
you like them.  :)  Thanks.

I also have comments at the bottom regards problems
I see but haven't patched.

---

patch_pg_current_logfile-v14.diff

Applies on top of master.

The current patch.

---

patch_pg_current_logfile-v14.diff.startup_docs

For consideration of inclusion in "main" patch.

Applies on top of patch_pg_current_logfile-v14.diff

A documentation fix.

(Part of) my previous doc patch was wrong; current_logfiles is not
unconditionally written on startup.

---

patch_pg_current_logfile-v14.diff.bool_to_int

Do not include in "main" patch, submit to maintainers separately.

Applies on top of patch_pg_current_logfile-v14.diff

The bool types on the stack in logfile_rotate() are
my work.  Bools on the stack don't make sense as far
as hardware goes, so the compiler's optimizer should change
them to int.  This patch changes the bools to ints
should that be to someone's taste.

---

patch_pg_current_logfile-v14.diff.logdest_change

For consideration of inclusion in "main" patch.

Applies on top of patch_pg_current_logfile-v14.diff.

Fixes a bug where, when log_destination changes and the config
file is reloaded, a no-longer-used logfile path may be written
to the current_logfiles file.  The chain of events would be
as follows:

1) PG configured with csvlog in log_destination. Logs are written.

This makes last_csv_file_name non-NULL.


2) PG config changed to remove csvlog from log_destination
and SIGHUP sent.

This removes the csvlog path from current_logfiles but does not
make last_csv_file_name NULL.  last_csv_file_name has the old
path in it.


3) PG configured to add csvlog back to log_destination and
SIGHUP sent.

When csvlogging is turned back on, the stale csv path is written
to current_logfiles.  This is overwritten as soon as some csv logs
are written because the new csv logfile must be opened, but this is
still a problem.  Even if it happens to be impossible at the moment
to get past step 2 to step 3 without having some logs written it seems
a lot more robust to manually "expire" the last*_file_name variable
content when log_destination changes.


So what the patch does is "scrub" the "last_*file_name" variables
when the config file changes.

FWIW, I moved the logfile_writename() call toward the top of the
if block just to keep all the code which sorta-kinda involves
the old_log_destination variable together.

---

patch_pg_current_logfile-v14.diff.logdest_change-part2

Do not include in "main" patch, submit to maintainers separately.

Applies on top of patch_pg_current_logfile-v14.diff.logdest_change

Adds a PGLogDestination typedef.  A separate patch since this may be
overkill and more about coding style than anything else.

---

And now, a series of patches to fix the problem where, at least
potentially, logfile_writename() gets a ENFILE or EMFILE and
therefore a log file path does not ever get written to the
current_logfiles file.  The point of this patch series is to retry until
the content of current_logfiles is correct.

All of these are for consideration of inclusion in "main" patch.


patch_pg_current_logfile-v14.diff.retry_current_logfiles-part1

Applies on top of patch_pg_current_logfile-v14.diff.logdest_change

A documentation patch.  Notes that (even with retrying) the
current_logfiles content might not be right.


patch_pg_current_logfile-v14.diff.retry_current_logfiles-part2

Applies on top of
patch_pg_current_logfile-v14.diff.retry_current_logfiles-part1

Remove arguments from logfile_writename().  Use static storage
instead.  We always update last_file_name and last_csv_file_name
whenever logfile_writename() is called so there's not much of
a change here.



patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3

Applies on top of
patch_pg_current_logfile-v14.diff.retry_current_logfiles-part2

Re-try the write of current_logfiles should it fail because the
system is too busy.

---

patch_pg_current_logfile-v14.diff.backoff

Do not include in "main" patch, submit to maintainers separately.

Applies on top of
patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3

Introduces a backoff when retrying to write the current_logfiles file,
because retrying when failure is due to a busy system only makes the
system more busy.  This may well be over-engineering but I thought
I'd present the code and let more experienced people decide.

I have yet to really test this patch.

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Mon, 21 Nov 2016 13:17:17 -0500
Robert Haas  wrote:

> > I've a couple of other patches that do
> > a little cleanup on master that I'd also like to submit
> > along with your patch. 

> It would really be much better to submit anything that's not
> absolutely necessary for the new feature as a separate patch, rather
> than bundling things together.

My plan is separate patches, one email.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sat, 19 Nov 2016 18:59:49 +0100
Gilles Darold <gilles.dar...@dalibo.com> wrote:

> Le 19/11/2016 à 16:22, Karl O. Pinc a écrit :
> > Hi Gilles,
> >
> > On Tue, 15 Nov 2016 15:15:52 -0600
> > "Karl O. Pinc" <k...@meme.com> wrote:
> >  
> >>> On Mon, 7 Nov 2016 23:29:28 +0100
> >>> Gilles Darold <gilles.dar...@dalibo.com> wrote:
> >>>>   - Do not write current_logfiles when log_collector is activated
> >>>> but log_destination doesn't contained stderr or csvlog. This was
> >>>> creating an empty file that can confuse the user.   
> >> Whether to write current_logfiles only when there's a stderr or
> >> csvlog seems dependent up on whether the current_logfiles file is
> >> for human or program use.  If for humans, don't write it unless it
> >> has content. If for programs, why make the program always have to
> >> check to see if the file exists before reading it?  Failing to
> >> check is just one more cause of bugs.  
> > What are your thoughts on this?  I'm leaning toward current_logfiles
> > being for programs, not people.  So it should be present whenever
> > logging_collector is on.  
> 
> My though is that it is better to not have an empty file even if
> log_collector is on.

Thanks.   I wrote to be sure that you'd considered my thoughts.

I don't see a point in further discussion.  I may submit a separate
patch to the maintainers when we're ready to send them code so they can
see how the code looks with current_logfiles always in existence.

Further thoughts below.  No need to read them or respond.

> Programs can not be confused but human yes, if
> the file is present but empty, someone may want to check why this
> file is empty.

IMO if they want to know why it's empty they could read the
docs.

> Also having a file containing two lines with just the
> log format without path is worst for confusion than having an empty
> file.

I agree that it makes no sense to list log formats without paths.
 
> In other words, from a program point of view, to gather last log
> filename, existence of the file or not doesn't make any difference.
> This is the responsibility of the programer to cover all cases. In a
> non expert user point of view, there will always the question: why
> this file is empty? If the file is not present, the question will
> stay: how I to get current log filename?

As a non-expert looking at the default data_directory (etc.) almost
nothing makes sense.  I see the non-expert using the
pg_current_logfile() function from within Postgres.

I also see a lot of non-experts writing program to get log data and
then getting errors when the config changes and current_logfile goes
away.  Not having data in a opened file generally means a program
does nothing. an appropriate action when there are no log files
in the filesystem.  But not accounting for a non-existent file leads
to crashes.

Regards,


Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Tue, 15 Nov 2016 15:15:52 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> > On Mon, 7 Nov 2016 23:29:28 +0100
> > Gilles Darold <gilles.dar...@dalibo.com> wrote:  

> > >   - Do not write current_logfiles when log_collector is activated
> > > but log_destination doesn't contained stderr or csvlog. This was
> > > creating an empty file that can confuse the user. 

> Whether to write current_logfiles only when there's a stderr or csvlog
> seems dependent up on whether the current_logfiles file is for human
> or program use.  If for humans, don't write it unless it has content.
> If for programs, why make the program always have to check to see
> if the file exists before reading it?  Failing to check is just one
> more cause of bugs.

What are your thoughts on this?  I'm leaning toward current_logfiles
being for programs, not people.  So it should be present whenever
logging_collector is on.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sat, 19 Nov 2016 12:58:47 +0100
Gilles Darold  wrote:

> All patches you've submitted on tha v13 patch have been applied and
> are present in attached v14 of the patch. I have not included the
> patches about GUC changes because I'm not sure that adding a new file
> (include/utils/guc_values.h) just for that will be accepted or that it
> will not require a more global work to add other GUC values. However
> perhaps this patch can be submitted separately if the decision is not
> taken here.

Understood.  I've a couple of other patches that do
a little cleanup on master that I'd also like to submit
along with your patch.  This on the theory that
the maintainers will be looking at this code anyway
because your patch touches it.  All this can be submitted
for their review at once.  My approach is to be minimally invasive on
a per-patch basis (i.e. your patch) but add small patches
that make existing code "better" without touching
functionality.  (Deleting unnecessary statements, etc.)
The overall goal being a better code base.

I've found what I think is another bug, where a 
long-stale filename could be written to current_logfiles.
I've coded a patch.

I also have a patch to ensure that current_logfiles never
"skips" a logfile due to ENFILE or ENFILE in
logfile_writename() (only).

I'm done with all the coding and need to refactor on top
of your v14 patch and on top of each other.  I'll send all
patches back to you for your review.  (Unless you've
some objection.)  I may not get to this until Monday morning.

I haven't throughly tested the ENFILE/ENFILE patch.
I was planning on doing that while you looked over the
code.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Sun, 30 Oct 2016 02:04:59 -0500
"Karl O. Pinc" <k...@meme.com> wrote:

> Attached is a patch to be applied on top of your v10 patch
> which does basic fixup to logfile_writename().

I'm looking at the v13 patch and don't see a change I submitted
with a patch to v10.  You wrote:

snprintf(tempfn, sizeof(tempfn), "%s",
CURRENT_LOG_FILENAME);
strcat(tempfn, ".tmp");

I patched to:

snprintf(tempfn, sizeof(tempfn), "%s.tmp",
CURRENT_LOG_FILENAME);

As long as you're doing a snprintf() there's no point
in "risking" a buffer overflow by a subsequent strcat().
(Not that you're likely to ever get a buffer overflow.)
And why make two calls instead of 1?  That's what's
in my head.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Mon, 7 Nov 2016 23:29:28 +0100
Gilles Darold  wrote:

> Here is the v13 of the patch, ...

Attached is a doc patch to apply on top of v13.

It adds a lot more detail regards just what is
in the current_logfiles file and when it's
in current_logfiles.  I'd like review both for
language and accuracy, but I'd also like to
confirm that we really want the documented
behavior regards what's there at backend startup
v.s. what's there during normal runtime.  Seems
ok the way it is to me but...

This patch is also starting to put a lot of information
inside a graphical table.  I'm fine with this but
it's a bit of a departure from the other cells of
the table so maybe somebody else has a better
suggestion.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v13.diff.detail_docs
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Mon, 7 Nov 2016 23:29:28 +0100
Gilles Darold  wrote:

> Here is the v13 of the patch,

Just to keep things up to date...  Attached are 3 re-worked patches
that I see submitting along with the pg_current_logfile patch.
They apply on top of v13.

(I still have one more I'm working on to ensure that current_logfiles
always has valid content should an attempt to open it returns ENFILE
or EMFILE.)


patch_pg_current_logfile-v13.diff.writeonce

Writes the current_logfiles file only once when rotating both syslog
and csvlog logfiles. 

This patch pushes bool types onto the stack instead of using int.
This is a practice which is not optimal given traditional instruction
sets and compilers.  I used bool types out of clarity.  If the
compiler does not optimize the problem away it won't impact
performance anyway because the code is not run that often.
If you find this ugly change the bools to ints.

I believe this patch also fixes a bug where, when both syslog and
csvlog are on and the syslog is rotated due to size but the csvlog is
not rotated (for whatever reason), then the syslog filename fails to
be updated in the current_logfiles file content.


patch_pg_current_logfile-v13.diff.abstract_guc_part1

Creates symbols for some GUC values which appear in multiple
places in the code.


patch_pg_current_logfile-v13.diff.abstract_guc_part2

Uses symbols for some GUC values in the pg_current_logfile
patch.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v13.diff.writeonce
Description: Binary data


patch_pg_current_logfile-v13.diff.abstract_guc_part1
Description: Binary data


patch_pg_current_logfile-v13.diff.abstract_guc_part2
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sat, 12 Nov 2016 12:59:46 -0600
"Karl O. Pinc" <k...@meme.com> wrote:

> On Mon, 7 Nov 2016 23:29:28 +0100
> Gilles Darold <gilles.dar...@dalibo.com> wrote:
> > Here is the v13 of the patch,  

> >   - Do not write current_logfiles when log_collector is activated
> > but log_destination doesn't contained stderr or csvlog. This was
> > creating an empty file that can confuse the user.  
> 
> Good catch.

I don't see a good reason to go past 80 characters on a line here.
Patch attached to be applied on top of v13.

Whether to write current_logfiles only when there's a stderr or csvlog
seems dependent up on whether the current_logfiles file is for human
or program use.  If for humans, don't write it unless it has content.
If for programs, why make the program always have to check to see
if the file exists before reading it?  Failing to check is just one
more cause of bugs.

A casual read of the v13 code does not show me that the current_logfiles
file is deleted when the config file is changed so that stderr and
csvlog is no longer output and the user sends a SIGHUP.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v13.diff.80char
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Mon, 7 Nov 2016 23:29:28 +0100
Gilles Darold  wrote:


> Here is the v13 of the patch,

>   - I've reverted the patch removing the call to logfile_writename in
> open_csvlogfile function, it is required to write log filename at
> startup when log_destination is set to csvlog. I could not find a
> better place right now, but will try to see if we can avoid the call
> to logfile_writename().

Why is calling logfile_writename() in open_csvlogfile() a problem?

I've not thought too deeply but this is what's in my mind.

The only issue that leaps to mind is overall code simplicity.  But
because csv files are opened "on-demand", only when there's something
in the queue to get written to a csv file, it's not clear how to avoid
calling logfile_writename() "on-demand" as well.  To do otherwise might
risk putting the name of a csv logfile into current_logfiles when that
csv log file does not yet exist.  Or you could wait, and have a csv
log file in existance but not have it reflected in the content of
current_logfiles for some time period.  But why?

If there is a problem in code clarity it might be that the csv log files
are not created until there's a csv log entry in the queue.  Change this
and other confusion, if there really is any, goes away.

>   - Do not write current_logfiles when log_collector is activated but
> log_destination doesn't contained stderr or csvlog. This was creating
> an empty file that can confuse the user.

Good catch.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Fri, 4 Nov 2016 16:58:45 +0100
Gilles Darold  wrote:

> I attached a v12 patch 

Attached is a comment patch which improves the comment
describing CURRENT_LOG_FILENAME.  It's been bugging me.
I should have made this change long ago when I looked
at all the other code comments but neglected to.

The comment now matches other documentation.

This applies on top of your v12 patch.

I'm thinking through the v12 patch and email.
I'm in general agreement but want to make sure
I really understand all the code paths.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v12.diff.symbolcomment
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Mon, 31 Oct 2016 10:19:18 +0100
Gilles Darold <gilles.dar...@dalibo.com> wrote:

> Le 31/10/2016 à 04:35, Karl O. Pinc a écrit :

> > Attached is a patch to apply on top of the v10 patch.
> >
> > It updates current_logfiles only once per log rotation.
> > I see no reason to open and write the file twice
> > if both csvlog and stderr logging is happening.  
> 
> I do not take the effort to do that because log rotation is not
> something that occurs too often and I don't want to change the
> conditional "time_based_rotation" code lines in logfile_rotate() for
> readability. My though was also that on high load, log rotation is
> automatically disabled so logfile_writename() is not called and there
> will be no additional I/O. But why not, if commiters want to save this
> additional I/O, this patch can be applied.

Ok.  You didn't put this into your v11 patch so it can be submitted to
the committers as a separate patch.

> > I don't understand why you're calling 
> > logfile_writename(last_file_name, last_csv_file_name);
> > in the SIGHUP code.  Presumably you've already
> > written the old logfile names to current_logfiles.
> > On SIGHUP you want to write the new names, not
> > the old ones.  But the point of the SIGHUP code
> > is to look for changes in logfile writing and then
> > call logfile_rotate() to make those changes.  And
> > logfile_rotate() calls logfile_writename() as appropriate.
> > Shouldn't the logfile_writename call in the SIGHUP
> > code be eliminated?  
> 
> No, when you change the log_destination and reload configuration you
> have to refresh the content of current_logfiles, in this case no new
> rotation have been done and you have to write last_file_name and/or
> last_csv_file_name.

I don't understand.  Please explain what's wrong with the
picture I have of how logging operates on receipt of SIGHUP.
Here is my understanding:

The system is running normally, current_logfiles exists and contains
the log file paths of the logs presently being written into.  These
paths end with the file names in last_file_name and/or
last_csv_file_name.  (I'm assuming throughout my description here that
log_destination is writing to the file system at all.)

A SIGHUP arrives.  The signal handler, sigHupHandler(), sets the
got_SIGHUP flag.  Nothing else happens.

The logging process wakes up due to the signal.
Either there's also log data or there's not.  If there's
not:

The logging process goes through it's processing loop and finds,
at line 305 of syslogger.c, got_SIGHUP to be true.
Then it proceeds to do a bunch of assignment statements.
If it finds that the log directory or logfile name changed
it requests immediate log file rotation.  It does this by
setting the request_rotation flag.  If log_destination
or logging_collector changed request_rotation is not set.

Then, your patch adds a call to logfile_writename().
But nothing has touched the last_file_name or last_csv_file_name
variables.  You rewrite into current_logfiles what's
already in current_logfiles.


If there is log data in the pipe on SIGHUP
and it's csv log data then if there's a csv
log file open that's the file that's written to.
last_csv_file_name does not change so current_logfiles
does not need to be re-written.  If there is no csv
log file open then open_csvlogfile() is called
and it calls logfile_writename().  No need to
call logfile_writename() again when processing the
SIGHUP.

If there is log data in the pipe on SIGHUP
and it's stderr log data then the currently open
stderr log file is written to.  This is already
in current_logfiles so no need to call logfile_writename().


Looking at what happens after your call to logfile_writename()
in SysLoggerMain() there's no changes to the log files
without calling logfile_writename.  

If there's stderr
log messages in the pipe these get written to the currently
open stderr log file until log rotation changes the file
name.  This either happens immediately because
request_rotation was set, or it waits.

If there's csv log messages in the pipe then these are either
written to the currently open log file or, when no
csv log file is open, open_csvlogfile() calls logfile_writename().
Either way, no need to re-write current_logfiles until
log rotation.

I'll respond to the rest of this email later, hopefully later
today.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 3 Nov 2016 18:34:50 -0500
"Karl O. Pinc" <k...@meme.com> wrote:

> On Mon, 31 Oct 2016 09:26:27 +0100
> Gilles Darold <gilles.dar...@dalibo.com> wrote:
> 
> > Le 30/10/2016 à 08:04, Karl O. Pinc a écrit :  
> 
> > > Have you given any thought to my proposal to change
> > > CURRENT_LOG_FILENAME to LOG_METAINFO_FILE?
> > Yes, I don't think the information logged in this file are kind of
> > meta information and CURRENT_LOG_FILENAME seems obvious. 

> ... maybe the right name is LOG_METAINFO_DATAFILE.

CURRENT_LOGFILES_FILENAME is good, but a bit long.

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Mon, 31 Oct 2016 09:26:27 +0100
Gilles Darold <gilles.dar...@dalibo.com> wrote:

> Le 30/10/2016 à 08:04, Karl O. Pinc a écrit :

> > Have you given any thought to my proposal to change
> > CURRENT_LOG_FILENAME to LOG_METAINFO_FILE?  
> Yes, I don't think the information logged in this file are kind of
> meta information and CURRENT_LOG_FILENAME seems obvious.

To me, the CURRENT_LOG_FILENAME symbol should contain the name 
of the current log file.  It does not.  The CURRENT_LOG_FILENAME symbol
holds the name of the file which itself contains the name of 
the log file(s) being written, plus the log file
structure of each log file.

IMO, the name of the log files being written, as well as
the type of data structure written into each log file,
are meta-information about the logging data.  So maybe
the right name is LOG_METAINFO_DATAFILE.

If you're not happy with making this change that's fine.
If not, I'd like to make mention of the symbol name to
the committers.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 2 Nov 2016 07:55:45 -0500
"Karl O. Pinc" <k...@meme.com> wrote:

> On Wed, 2 Nov 2016 10:07:34 +0100
> Gilles Darold <gilles.dar...@dalibo.com> wrote:
> 
> > Please have a look at line  1137 on HEAD of syslogger.c 

> Ok.  Thanks.  Sorry for the confusion.

And yes, we did talk about this before.  I should have remembered.

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 2 Nov 2016 10:07:34 +0100
Gilles Darold  wrote:

> Please have a look at line  1137 on HEAD of syslogger.c you will see
> that in case of failure function logfile_open() report a FATAL or LOG
> error with message:
> 
> errmsg("could not open log file \"%s\": %m", filename);

Ok.  Thanks.  Sorry for the confusion.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Mon, 31 Oct 2016 09:26:27 +0100
Gilles Darold <gilles.dar...@dalibo.com> wrote:

> Le 30/10/2016 à 08:04, Karl O. Pinc a écrit :

> Attached patch v11 include your patch.
> 
> >
> > I have some questions about logfile_writename():
> >
> >  Why does the logfile_open() call fail silently?

> logfile_open() "fail silently" in logfile_writename(), like in other
> parts of syslogger.c where it is called, because this is a function()
> that already report a message when an error occurs ("could not open
> log file..."). I think I have already replied to this question.

Please apply the attached patch on top of your v11 patch.
It simulates a logfile_open() failure.  Upon simulated failure you do
not get a "currrent_logfile" file, and neither do you get any
indication of any problems anywhere in the logs.  It's failing
silently.

To test I create a cluster, start the server, and look for
current_logfile and at the logs.

(I finally got around to writing down the process I use to install
and run a patched server, instead of just poking it with a
stick until it works every time I get back to hacking pg.  
I'd be happy to share my process with you
if you're interested.  If you cannot reproduce my results please
share with me your procedure for cluster creation and runtime testing
so I can see why I find a problem and you don't.  Thank you.)

I don't expect to have a lot of time to work on pg in the next
36 hours.  After that I hope to push this through to completion.
I did want to get something back to you now, hence this email.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v11.diff.silentfail
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Sat, 29 Oct 2016 22:00:08 +0200
Gilles Darold  wrote:

> The attached v10 of the current_logfiles patch 

Attached is a patch to apply on top of the v10 patch.

It updates current_logfiles only once per log rotation.
I see no reason to open and write the file twice
if both csvlog and stderr logging is happening.

I have 2 more questions.

I don't understand why you're calling 
logfile_writename(last_file_name, last_csv_file_name);
in the SIGHUP code.  Presumably you've already
written the old logfile names to current_logfiles.
On SIGHUP you want to write the new names, not
the old ones.  But the point of the SIGHUP code
is to look for changes in logfile writing and then
call logfile_rotate() to make those changes.  And
logfile_rotate() calls logfile_writename() as appropriate.
Shouldn't the logfile_writename call in the SIGHUP
code be eliminated?

Second, and I've not paid really close attention here,
you're calling logfile_writename() with last_file_name
and last_csv_file_name in a number of places.  Are you
sure that last_file_name and last_csv_file_name is reset
to the empty string if stderr/csvlog logging is turned
off and the config file re-read?  You might be recording
that logging is going somewhere that's been turned off
by a config change.  I've not noticed last_file_name and
(especially) last_csv_file_name getting reset to the empty
string.

FYI, ultimately, in order to re-try writes to current_logfiles
after ENFILE and EMFILE failure, I'm thinking that I'll probably
wind up with logfile_writename() taking no arguments.  It will
always write last_file_name and last_csv_file_name.  Then it's
a matter of making sure that these variables contain accurate
values.  It would be helpful to let me know if you have any
insight regards config file re-read and resetting of these
variables before I dive into writing code which retries writes to
current_logfiles.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v10.diff.only_once
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 28 Jun 2016 11:06:24 +0200
Gilles Darold <gilles.dar...@dalibo.com> wrote:

> Le 07/04/2016 08:30, Karl O. Pinc a écrit :

> > "src/backend/postmaster/syslogger.c expects to see fopen() fail
> > with  
> ENFILE and EMFILE.  What will you do if you get these?"
> 
> - Nothing, if the problem occurs during the log rotate call, then
> log rotation file is disabled so logfile_writename() will not be
> called. Case where the problem occurs between the rotation call and
> the logfile_writename() call is possible but I don't think that it
> will be useful to try again. In this last case log filename will be
> updated during next rotation.

The case I'm interested in is when the rotation call succeeds but
you get ENFILE or EMFILE in logfile_writename() and current_logfiles
is not updated.

This looks like an ugly problem that only happens
sporadically under load.  If I've set log
rotation to, say, 1 week, and I'm post-processing my logs based
on the current_logfiles content, and the logs rotate but
current_logfiles is not updated, then I lose a week of log
post-processing.

I'm experimenting with some code that retries writing current_logfiles,
if it failed due to ENFILE or EMFILE, the next time a log message
is written.  If that's too often it'd be easy enough to add
a backoff counter that retries progressively less frequently
based on a "clock tick" per log message write.

However, per my last email, it'll be Tuesday before I really get
back to this.   Let me know if, instead, you want to jump in
and write the code, have a better idea, think this is a really 
stupid approach, or have other reasons why I should abandon
this plan.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sat, 29 Oct 2016 22:00:08 +0200
Gilles Darold  wrote:

> The attached v10 of the current_logfiles patch include your last
> changes on documentation but not the patch on v9 about the
> user-supplied GUC value. I think the v10 path is ready for committers
> and that the additional patch to add src/include/utils/guc_values.h
> to define user GUC values is something that need to be taken outside
> this one. Imo, thoses GUC values (stderr, csvlog) are not expected to
> change so often to require a global definition, but why not, if
> committers think this must be done I can add it to a v11 patch.

I agree that the guc_values.h patches should be submitted separately
to the committers, when your patch is submitted.  Creating symbols
for these values is a matter of coding style they should resolve.
(IMO it's not whether the values will change, it's whether
someone reading the code can read the letters "stdout" and know
to what they refer and where to find related usage elsewhere in 
the code.)

I'll keep up the guc_values.h patches and have them ready for
submission along with your patch.

I don't think your patch is quite ready for submission to
the committers.

Attached is a patch to be applied on top of your v10 patch
which does basic fixup to logfile_writename().

I have some questions about logfile_writename():

 Why does the logfile_open() call fail silently?
 Why not use ereport() here?  (With a WARNING level.)

 Why do the ereport() invocations all have a LOG level?
 You're not recovering and the errors are unexpected
 so I'd think WARNING would be the right level.
 (I previously, IIRC, suggested LOG level -- only if
 you are retrying and recovering from an error.)


Have you given any thought to my proposal to change
CURRENT_LOG_FILENAME to LOG_METAINFO_FILE?


I'm not sure I've looked at every bit of your patch
yet.  I won't have much time to do more real work
until after Tuesday morning.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v10.diff.logfile_writename
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Fri, 28 Oct 2016 10:03:37 +0200
Gilles Darold  wrote:

> ...
> v9 of the patch, attached here.

Attached are 2 more documentation patchs to apply on
top of your v9 patch.


patch_pg_current_logfile-v9.diff.doc_current_logfiles

Explains the current_logfiles file in the
narrative documentation.  It's not like I want
to toot our horn here.  I'm afraid that otherwise
no one will notice the feature.


patch_pg_current_logfile-v9.diff.doc_indexes

Fixes an index entry and add more.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v9.diff.doc_current_logfiles
Description: Binary data


patch_pg_current_logfile-v9.diff.doc_indexes
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Fri, 28 Oct 2016 10:03:37 +0200
Gilles Darold  wrote:

> ...
> the v9 of the patch, attached here.

I notice that there are a number of user-supplied GUC
values for log_destination that are repeatedly used,
both in the GUC code and in your patch.  These are
presently written as hardcoded strings.

Attached are 2 patches which abstract the values a
user is supposed to supply for log_destination.


patch_pg_current_logfile-v9.diff.guc_values-part1
This applies to both master HEAD and on top of your v9
patch.  It abstracts the user-supplied values within
the GUC code.

patch_pg_current_logfile-v9.diff.guc_values-part2
This applies on top of your v9 patch.

I couldn't find a good place to put the newly defined symbols
in the existing code so the part1 patch creates
src/include/utils/guc_values.h.  Someone who knows
the code better than me would be better able to judge
if making a new .h file is a good idea.  Likewise, I presume
that a "GUCV_" prefix for the new symbols is good, but this
too could use review.

The odd part about the part1 patch is that GUCV_EVENTLOG
is never used anywhere but in src/backend/utils/misc/guc.c.
But it is used twice there and it seemed like as long as
I was doing the rest of the log_destination values I should
abstract eventlog too.

If we use these patches I propose that we keep the
part1 patch and submit it separately to the committers.
Seems like it'd be easier to review/commit when the changes to
existing code are kept separate from new code.

> Thanks a lot.

Thank you also for considering my ideas.  :)

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v9.diff.guc_values-part2
Description: Binary data


patch_pg_current_logfile-v9.diff.guc_values-part1
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 27 Oct 2016 19:03:11 +0200
Gilles Darold  wrote:

> The current v8 of the patch

Perhaps instead of the define CURRENT_LOG_FILENAME
a better name for the symbol would be LOG_METAINFO_FILE?

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 27 Oct 2016 22:09:41 -0500
"Karl O. Pinc" <k...@meme.com> wrote:
> On Thu, 27 Oct 2016 19:03:11 +0200
> Gilles Darold <gilles.dar...@dalibo.com> wrote:
> 
> > The current v8 of the patch
> 
> For your consideration.
> 
> Attached is a patch to apply to v8 of your patch.
> 
> I moved the call to logfile_writename() in write_syslogger_file()
> into the open_csvlogfile().  That's where the new filename
> is generated and it makes sense to record the new name
> upon generation.

Er.  Actually attached the patch this time.

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v8.diff.open_csvlogfile
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Thu, 27 Oct 2016 19:03:11 +0200
Gilles Darold  wrote:

> The current v8 of the patch

For your consideration.

Attached is a patch to apply to v8 of your patch.

I moved the call to logfile_writename() in write_syslogger_file()
into the open_csvlogfile().  That's where the new filename
is generated and it makes sense to record the new name
upon generation.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 27 Oct 2016 19:03:11 +0200
Gilles Darold  wrote:

> The current v8 of the patch 

Attached is a patch to the v8 version of your patch.

It rewords some of the comments in the code.  Take the hunks
or leave them as you wish.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v8.diff.codecomments
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 27 Oct 2016 16:18:02 -0500
"Karl O. Pinc" <k...@meme.com> wrote:

> On Thu, 27 Oct 2016 19:03:11 +0200
> Gilles Darold <gilles.dar...@dalibo.com> wrote:
> 
> > The current v8 of the patch
> 
> Attached is a doc patch for your v8 patch.
> 
> Added , so the docs would build.
> 
> Added markup of "system values".

Drat.  The attached patch of my last email was against master.
New attached patch is to apply on top of the v8 patch.  Sorry.


Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v8.diff.patchv2
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 27 Oct 2016 19:03:11 +0200
Gilles Darold  wrote:

> The current v8 of the patch

Attached is a doc patch for your v8 patch.

Added , so the docs would build.

Added markup of "system values".


Hope to look at code soon!


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v8.diff.patchv1
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 27 Oct 2016 19:57:18 +0200
Christoph Berg <m...@debian.org> wrote:

> Re: Karl O. Pinc 2016-10-27 <20161027121141.6bd95...@slate.meme.com>
> > SELECT * from postgres.pg_current_logfile;

> We were discussing exactly that idea upthread before concluding that a
> function with a single return value is much easier to use.

Apologies for raising the dead this Halloween season.  :)

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 27 Oct 2016 19:03:11 +0200
Gilles Darold <gilles.dar...@dalibo.com> wrote:

> >> Re: Karl O. Pinc 2016-10-27
> >> <20161026222513.74cd3...@slate.meme.com>  

> > Your comment makes me wonder if pg_current_logfile(), without
> > arguments, should instead be "SHOW current_logfile;".

> -1, SHOW is used to display run-time parameters values

Another interface to consider might be a system catalog:

SELECT * from postgres.pg_current_logfile;

format | path
---+---
syslog | /some/where/log
cvslog | /some/where/log.csv

(2 rows)

Maybe good if the goal is "interactive use".  Seems like
overkill to me, but thought I'd present the idea
anyway.



Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 27 Oct 2016 11:07:43 +0200
Christoph Berg <m...@debian.org> wrote:

> Re: Karl O. Pinc 2016-10-27 <20161026222513.74cd3...@slate.meme.com>

> > But what if current_logfile contains only a single line?  What
> > sort of file format does the logfile have?  If you don't know
> > you can't process the logfile content.
> > 
> > When there's multiple lines in current_logfile your script might
> > be looking for a logfile in a particular format.  How is the
> > script supposed to know the file format of each logfile listed?  
> 
> My idea here would be to always write out two lines, the first for
> stderr, the second for csvlog, and leave the unused one empty. That's
> easy to parse from shell scripts.

That'd work.

> 
> > Regards the data structure to use to expose the file format
> > I can't vouch that "format path" is most future-proof.
> > It's what I came up with on the spur of the moment.
> > Something like: "format : path ",
> > where ":" is the field separator and each data element is
> > tagged, would still be parseable by the shell "read" built-in
> > so long as the path comes last. I don't really care about 
> > the exact data structure but I do think the file format
> > meta-information should be included.  
> 
> I guess that depends on how likely we think new log formats would be
> added in the future. My guess would be that it's rather unlikely, so
> going with simple file format makes sense.

Agreed.  I can't see adding any more meta-information other than
file format.

I'm partial to "format  path" over just line number, because
it's more explicit.  Either way works.

> > Why not just: SELECT pg_log_format();

> That function already exists: "show log_destination;"

Great, done!  :)

> > Therefore pg_current_logfile() without any arguments is, in the
> > sense of any sort of automated processing of the logfile content,
> > useless.  
> 
> The function without arguments is very useful for interactive use,
> which is the primary point of this patch in my opinion.

Your comment makes me wonder if pg_current_logfile(), without
arguments, should instead be "SHOW current_logfile;".

I think not, but have no rationale.  Could this be a good idea?

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 27 Oct 2016 00:31:56 +0200
Gilles Darold  wrote:

> Thanks a lot for the documentation fixes, I've also patched some of
> your changes, see v7 of the patch and explanations bellow.

Thanks.  Sorry if I've not kept up on your latest decisions.

> > Put pg_log_file in alphabetical order in the file name listing
> > and section body.  
> 
> This file is now named current_logfile, I have changed the named in
> the documentation, especially in storage.sgml. 

Since it now can contain multiple pathnames perhaps the name of the
file should be "current_logfiles"?  Seems more descriptive.

> One other change in documentation is the explanation of values stored
> in this file. This is a path: log_directory/log_filename, and no more
> the log file name only. This will help to get full path of the log at
> system command level. This is also the value returned by function the
> pg_current_logfile() to be able to read file directly with a simple:
> 
>  SELECT pg_read_file(pg_current_logfile());

Sounds good.

> > I also have a couple of preliminary comments.

> > Since pg_log_file may contain only one line, and that
> > line may be either the filename of the csv log file or
> > the file name of the stderr file name it's impossible
> > to tell whether that single file is in csv or stderr
> > format.  I suppose it might be possible based on file
> > name suffix, but Unix expressly ignores file name
> > extensions and it seems unwise to force dependence
> > on them.  Perhaps each line could begin with
> > the "type" of the file, either 'csv' or 'stderr'
> > followed by a space and the file name?   
> 
> The current_logfile may contain 2 lines, one for csvlog and an other
> for stderr when they are both defined in log_destination. As said
> above, the csvlog file will always have the .csv suffix, I guess it
> is enough to now the format but I agree that it will not be true in
> all cases. To keep things simple I prefer to only register the path
> to the log file, external tools can easily detect the format or can
> ask for the path to a specific log format using SELECT
> pg_current_logfile('stderr'); for example. This is my point of view,
> but if there's a majority to add the log format into the
> current_logfile why not.

Let me explain my reasoning:  The current_logfile file's location
is not typically going to change.  The cluster gets created and
current_logfile then has a known location.  

Having a known location in the filesystem for current_logfile is sort 
of the point of current_logfile.  A script knows where to find it,
to look at it's content and find out where to get the log file
it needs.  Execing a psql process or otherwise creating a database
connection, just in order to execute "SELECT
pg_current_logfile('stderr');" to discover where the current logs
are, is _way_ more complicated and error prone that reading
a file's content.

But what if current_logfile contains only a single line?  What
sort of file format does the logfile have?  If you don't know
you can't process the logfile content.

When there's multiple lines in current_logfile your script might
be looking for a logfile in a particular format.  How is the
script supposed to know the file format of each logfile listed?
Relying on a '.csv' file name extension is adequate, but
only under a number of assumptions.  What if PG adds another
logfile format that's different from stderr; how would this
new format be distinguished from stderr?  What if MS Excel
changes the CSV format (yet again) or a non MS Excel CSV
format is supported by PG, so that PG supports
multiple CSV formats; how would the different CSV
file formats be distinguished from each other.  Without
going on and on, there's good reasons why Unix does not
rely on filename extensions to determine the format of file
content.  Sure, scripts could always do "something", like
exec the "file" program, to discover, or at least guess
at, the format of the log file.  Or we could have PG
be forever dependent upon using filename extensions
when introducing new logfile formats.  There's always
kludges which will handle new circumstances.  But why
go there?  PG knows the format of the files it's writing into
current_logfile. 

My argument is strongly related to the "Explicit is better
than implicit" philosophy of design.
 
It's not hard to cleanly expose the logfile format to the 
reader of current_logfile; guessing would never be required
no matter the future of PG.
"current_logfile" must then be parsed, but if you _are_ going
to expose the file format then putting it into the same file
as the logfile pathnames is the way to go.

"format  path" is as easy to parse as looking for 
a ".csv" suffix, and a lot more clear and future proof.  (Even 
works with spaces in the "path", in shell, using the "read"
builtin.)  It does mean that every user of current_logfile _must_ 
parse.  If you don't put an explicit format into the file content
readers who already know what file 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 25 Oct 2016 22:30:48 -0500
"Karl O. Pinc" <k...@meme.com> wrote:

> Since pg_log_file may contain only one line, and that
> line may be either the filename of the csv log file or
> the file name of the stderr file name it's impossible
> to tell whether that single file is in csv or stderr
> format.  I suppose it might be possible based on file
> name suffix, but Unix expressly ignores file name
> extensions and it seems unwise to force dependence
> on them.  Perhaps each line could begin with
> the "type" of the file, either 'csv' or 'stderr'
> followed by a space and the file name?  
> 
> In other words,
> as long as you're making the content of pg_log_file
> a data structure that contains more than just a single
> file name you may as well make that data structure
> something well-defined, easily parseable in shell, extensible,
> and informative.

While you're at it, it wouldn't hurt to provide another
function that tells you the format of the file returned
by pg_current_logfile(), since pg_current_logfile()
called without arguments could return either a stderr
formatted file or a csvlog formatted file.

Or leave it for the future.  Just a thought.


Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Another new version of a doc patch to the v6 patch.

More better English.  *sigh*

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v6.diff.patchv4
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 25 Oct 2016 22:53:41 -0500
"Karl O. Pinc" <k...@meme.com> wrote:

> On Tue, 25 Oct 2016 22:30:48 -0500
> "Karl O. Pinc" <k...@meme.com> wrote:
>  
> > Hope to provide more feedback soon.  

Er, attached is yet another doc patch to the v6 patch.
Sorry about that.

Changes pg_current_logfile() detailed documentation.

Instead of saying that return values are undefined
I've documented that NULL is returned.  And
reworded, and shortened, my previous wording.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v6.diff.patchv3
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 25 Oct 2016 22:30:48 -0500
"Karl O. Pinc" <k...@meme.com> wrote:
 
> Hope to provide more feedback soon.

Before I forget:

"make check" fails, due to oid issues with pg_current_logfile().

You're writing Unix eol characters into pg_log_file.  (I think.)
Does this matter on MS Windows?  (I'm not up on MS Windows,
and haven't put any thought into this at all.  But didn't
want to forget about the potential issue.)

Now that pg_log_file contains multiple lines shouldn't
it be called pg_log_files?

In the docs, other functions that take optional arguments
show up as multiple rows.  Attached is a new version of
my patch to the v6 patch which fixes this and supplies
a slightly better short description of pg_log_filename().

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v6.diff.patchv2
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 18 Oct 2016 14:18:36 +0200
Gilles Darold  wrote:

> Here is the v6 of the patch, here is the description of the
> pg_current_logfile() function, I have tried to keep thing as simple as
> possible:
> 
> pg_current_logfile( [ destination text ] )
> -

Attached is a doc patch to apply on top of your v6 patch.

Changes are, roughly:

Put pg_log_file in alphabetical order in the file name listing
and section body.

Put pg_current_logfile in alphabetical order in the function
name listing and section body.

1 argument functions don't seem to have a parameter name
when listed in documentation tables, just a data type,
so I got rid of the parameter name for pg_current_logfile().

Cleaned up the wording and provided more detail.

Added hyperlink to log_destination config parameter.

Added markup to various system values.  The markup does
not stand out very well in the html docs, but that's a different
issue and should be fixed by changing the markup processing.
(I used the markup found in the log_destination()
config parameter docs.)

pg_current_logfile does not seem related to pg_listening_channels
or pg_notification_queue_usage so I moved the latter 2 index
entries closer to their text.


I also have a couple of preliminary comments.

It seems like the code is testing for whether the
logfile is a CSV file by looking for a '.csv' suffix
on the end of the file name.  This seems a little fragile.
Shouldn't it be looking at something set internally when the
log_destination config declaration is parsed?

Since pg_log_file may contain only one line, and that
line may be either the filename of the csv log file or
the file name of the stderr file name it's impossible
to tell whether that single file is in csv or stderr
format.  I suppose it might be possible based on file
name suffix, but Unix expressly ignores file name
extensions and it seems unwise to force dependence
on them.  Perhaps each line could begin with
the "type" of the file, either 'csv' or 'stderr'
followed by a space and the file name?  

In other words,
as long as you're making the content of pg_log_file
a data structure that contains more than just a single
file name you may as well make that data structure
something well-defined, easily parseable in shell, extensible,
and informative.

Hope to provide more feedback soon.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index d1a5b96..54e10af 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15443,6 +15443,12 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   log file in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15480,12 +15486,6 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
-   pg_current_logfile( destination text)
-   text
-   current log file used by the logging collector
-  
-
-  
session_user
name
session user name
@@ -15667,6 +15667,27 @@ SET search_path TO schema , schema, ..

 

+pg_current_logfile
+   
+
+   
+pg_current_logfile returns, as text
+the name of the log file currently in use by the logging collector.
+Log collection must be active or the return value is undefined.  When
+the  configuration parameter
+contains both csvlog and stderr
+pg_current_logfile returns the stderr filename.
+When  does not
+contain stderr the csv filename is returned.  When it
+does not contains csvlog the stderr filename is
+returned.  To request a specific file format supply either
+csvlog or stderr as the value of the
+optional, type text, parameter. The return value is
+undefined when the log format requested is not a configured log
+destination.
+   
+
+   
 pg_my_temp_schema

 
@@ -15692,23 +15713,6 @@ SET search_path TO schema , schema, ..
 pg_notification_queue_usage

 
-   
-pg_current_logfile
-   
-
-   
-pg_current_logfile returns the name of the
-current log file used by the logging collector, as text.
-Log collection must be active or the return value is undefined. When
-csvlog is used as log destination, the csv filename is returned, when
-it is set to stderr, the stderr filename is returned. When both are
-used, it returns the stderr filename.
-There is an optional parameter of type text to determines
-the log filename to return following the log destination, values can
-be 'csvlog' or 'stderr'. When the log format asked is not used as log
-destination then the return value is undefined.
-   
-

 pg_listening_channels returns a set of names of
 asynchronous 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Mon, 3 Oct 2016 13:35:16 +0900
Michael Paquier  wrote:
> 
> Moved to next CF, the patch still applies. Karl, you have registered
> to review this patch a couple of months back but nothing happened. I
> have removed your name for now. If you have time, don't hesitate to
> come back to it.

Right.  Hope to get back to it soon.  Won't register until I'm ready
to look at it.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 28 Jun 2016 11:06:24 +0200
Gilles Darold  wrote:

> Thank you very much for the patch review and please apologies this too
> long response delay. I was traveling since end of April and totally
> forgotten this patch. I have applied all your useful feedbacks on the
> patch and attached a new one (v4) to this email :

Hi Gilles,

My schedule is really full at the moment.  I'll get to this
when I have a bit of time.  If you get anxious please write
and I'll see about making faster progress.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] PQsendQuery+PQgetResult+PQsetSingleRowMode limitations and support

[Karl O. Pinc]

On Mon, 11 Apr 2016 19:25:20 +0200
"Shulgin, Oleksandr" <oleksandr.shul...@zalando.de> wrote:

> On Mon, Apr 11, 2016 at 7:15 PM, Karl O. Pinc <k...@meme.com> wrote:

> > > Not sure about the part
> > > where you call PQsetSingleRowMode() again after seeing
> > > PGRES_TUPLES_OK: doesn't look to me like you need or want to do
> > > that.  You should only call it immediately after PQsendQuery().  
> >
> > You're quite right.  All but the first PQsetSingleRowMode()
> > calls fail.
> >
> > This seems unfortunate.   What if I submit several SQL statements
> > with one PQsendQuery() call and I only want some of the statements
> > executed in single row mode?  
> 
> I would assume that if you know for which of the statements you want
> the single row mode, then you as well can submit them as separate
> PQsendQuery() calls.

Agreed.  Although I suppose it's possible to know which statements
you want in single row mode but not know how to parse those
statements out of some big string of queries.  Not my problem.  ;-)

> > When the docs here say "query" what they really mean is "set of
> > statements submitted in a single libpq call".  
> 
> Which are the same things more or less, I'm not sure that the extended
> explanation you suggest makes it less confusing.

I'll try to remember to cc-you if and when I send in a doc patch
so you can see if there's any improvement.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] PQsendQuery+PQgetResult+PQsetSingleRowMode limitations and support

[Karl O. Pinc]

On Mon, 11 Apr 2016 15:55:53 +0200
"Shulgin, Oleksandr" <oleksandr.shul...@zalando.de> wrote:

> On Fri, Apr 1, 2016 at 7:53 PM, Karl O. Pinc <k...@meme.com> wrote:
> >
> > On Fri, 1 Apr 2016 05:57:33 +0200
> > "Shulgin, Oleksandr" <oleksandr.shul...@zalando.de> wrote:
> >  
> > > On Apr 1, 2016 02:57, "Karl O. Pinc" <k...@meme.com> wrote:  
> > > >
> > > > I assume there are no questions about supporting a
> > > > similar functionality only without PQsetSingleRowMode,
> > > > as follows:  
> > >
> > > Sorry, but I don't see what is your actual question here?  
> >
> > The question is whether or not the functionality of the first
> > script is supported.  I ask since Bruce was surprised to see
> > this working and questioned whether PG was intended to behave
> > this way.  
> 
> Well, according to the docs it should work, though I don't recall if
> I have really tried that at least once. 

Well, the code does work.  (Mostly, see below.)

Should I submit a regression test or something to ensure
that this usage is officially supported?  (A grep for
PQsetSingleRowMode in src/test/ finds no hits.)
Can I assume because it's documented it'll continue to work?
Where do I go from here?

> Not sure about the part
> where you call PQsetSingleRowMode() again after seeing
> PGRES_TUPLES_OK: doesn't look to me like you need or want to do
> that.  You should only call it immediately after PQsendQuery().

You're quite right.  All but the first PQsetSingleRowMode()
calls fail.

This seems unfortunate.   What if I submit several SQL statements
with one PQsendQuery() call and I only want some of the statements
executed in single row mode?  I'm not sure what the use case
would be but it seems sad that PQsetSingleRowMode() is per
libpq call and not per sql statement.  It seems a little late
to change the API now.  (On the other hand, fewer calls = less
overhead, especially on the network.  So maybe it's just as well
and any deficiencies are best left for future work.)


For the record, here is where I got confused:

I find the docs unclear.  (I've plans to send in a patch, but
I think I'll wait until after finishing as a reviewer for
somebody else's patch.  That is in process now.)

The docs say:

"To enter single-row mode, call PQsetSingleRowMode immediately after a
successful call of PQsendQuery (or a sibling function). This mode
selection is effective only for the currently executing query."
(http://www.postgresql.org/docs/devel/static/libpq-single-row-mode.html)

Now, if the mode selection is effective only for the currently
executing query then if you call PQSetSingleRowMode() only
once after PQsendQuery() then single row mode will only be on
for the first query, when multiple queries are supplied in
the string passed to PQsendQuery().  The other queries won't
be executed in single row mode.

When the docs here say "query" what they really mean is "set of
statements submitted in a single libpq call".


> > Thanks for the clarification.  For some reason I recently
> > got it into my head that the libpq buffering was on the server side,
> > which is really strange since I long ago determined it was
> > client side.  
> 
> There are also a number of cases where the caching will happen on the
> server side: 



> Less obvious is when you have a set-returning-function and use it like
> "SELECT * FROM srffunc()", this will cause the intermediate result to
> be materialized in a tuple store on the server side before it will be
> streamed to the client.  On the other hand, if you use the same
> function as "SELECT srffunc()" you are going to get the same results
> streamed to the client. I've seen this a number of times already and
> I doesn't look like a fundamental limitation of the execution engine
> to me, rather an implementation deficiency.

That is very interesting.  Thanks.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 7 Apr 2016 01:13:51 -0500
"Karl O. Pinc" <k...@meme.com> wrote:

> On Wed, 6 Apr 2016 23:37:09 -0500
> "Karl O. Pinc" <k...@meme.com> wrote:
> 
> > On Wed, 6 Apr 2016 22:26:13 -0500
> > "Karl O. Pinc" <k...@meme.com> wrote:  
> > > On Wed, 23 Mar 2016 23:22:26 +0100
> > > Gilles Darold <gilles.dar...@dalibo.com> wrote:
> > > 
> > > > Thanks for the reminder, here is the v3 of the patch after a
> > > > deeper review and testing. It is now registered to the next
> > > > commit fest under the System Administration topic.

> This is what I see at the moment.  I'll wait for replies
> before looking further and writing more.

Er, one more thing.  Isn't it true that in logfile_rotate()
you only need to call store_current_log_filename() when
logfile_open() is called with a "w" mode, and never need to
call it when logfile_open() is called with an "a" mode?


Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 6 Apr 2016 23:37:09 -0500
"Karl O. Pinc" <k...@meme.com> wrote:

> On Wed, 6 Apr 2016 22:26:13 -0500
> "Karl O. Pinc" <k...@meme.com> wrote:
> > On Wed, 23 Mar 2016 23:22:26 +0100
> > Gilles Darold <gilles.dar...@dalibo.com> wrote:
> >   
> > > Thanks for the reminder, here is the v3 of the patch after a
> > > deeper review and testing. It is now registered to the next
> > > commit fest under the System Administration topic.   

> Ok.  I've built it (but not tested it).  Some comments.

The logic in src/backend/postmaster/syslogger.c
looks good to me.  (The log file is created before you
create the pg_curr_log file, so the worst thing to happen
is that the user gets the old log file, never a non-existent
file.)

In two places in src/backend/postmaster/syslogger.c,
before you call store_current_log_filename()
you have a comment that's more than 80 characters
long.  Please fix.  (But see below for more.)
(http://www.postgresql.org/docs/devel/static/source-format.html)

Given the name store_current_log_filename() the code
comment is not particularly useful.  You might
consider removing the comment.  You might also
consider changing store_currrent_log_filename()
to something like write_pg_log_file().  A little
shorter, and maybe more descriptive.  Maybe enough
so that you can get rid of the comment.

(Are there other functions that create similar files?
Maybe there is a naming convention you can follow?)

(I like comments, but the pg coding style uses fewer
of them than I might use.  Hence my notes above.)

Also, your patch seems to be using spaces, not tabs.
You want tabs.
See the formatting url above.  (I forget whether
the docs use spaces or tabs.  Check the code.)

Other thoughts:

You're going to have to do something for MS Windows
EOL conventions like in logfile_open() of
src/backend/postmaster/syslogger.  You can't just
use a "\n".

The initial pg_log_file file is created by the postmaster.
Subsequent work is done by a logging subprocess.
Are there any permission implications?

src/backend/postmaster/syslogger.c expects to see
fopen() fail with ENFILE and EMFILE.  What will you
do if you get these?  Can you do the same thing
that the log rotation code does and try to update
pg_log_file the next time something logs?  You'd
have to set a flag somewhere and test (in the regular
logging code) since presumably
the next time something is logged the log rotation
code (where all your code is) would no longer be
executed.  This would leave the user with a "stale"
pg_log_file, but I'm not sure what else to do.

Regards the ereport() log level for when you can't
create pg_log_file at all, WARNING seems a bit severe
since LOG is used when the log stops rotating for some
reason.  But there does not seem to be anything else that
fits so WARNING is ok with me.  (See: include/utils/elog.h)

(I'd use LOG if you have to defer the updating of
pg_log_file.  But logging at all here could be problematic.
You wouldn't want to trigger more even more logging
and some sort of tight little loop.  It might be best
_not_ to log in this case.)

Have you given any thought as to when logfile_rotate()
is called?  Since logfile_rotate() itself logs with ereport(),
it would _seem_ safe to ereport() from within your
store_current_log_filename(), called from within
logfile_rotate().  All the same, it wouldn't hurt to be
sure that calling ereport() from within your code
can't trigger another invocation of logfile_rotate()
leading to recursive awfulness.

The indentation of the ereport(), in the part that
continues over multiple lines, in store_current_log_filename()
seems too much.  I think the continued lines should line up with
the "W" in WARNING.

This is what I see at the moment.  I'll wait for replies
before looking further and writing more.

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 6 Apr 2016 22:26:13 -0500
"Karl O. Pinc" <k...@meme.com> wrote:
> On Wed, 23 Mar 2016 23:22:26 +0100
> Gilles Darold <gilles.dar...@dalibo.com> wrote:
> 
> > Thanks for the reminder, here is the v3 of the patch after a deeper
> > review and testing. It is now registered to the next commit fest
> > under the System Administration topic. 

> I've not yet even tried building it, 

Ok.  I've built it (but not tested it).  Some comments.

The docs don't build.  You are missing a "<row"> at line
15197 of func.sgml.  (Patched against current HEAD.)

Is there any rationale for the placement of your function
in that documentation table?  I don't see any organizing principle
to the table so am wondering where it might best fit.
(http://www.postgresql.org/docs/devel/static/functions-info.html)

Perhaps you want to write?:

   
pg_current_logfile returns the name of the
   current log file used by the logging collector, as
   text. Log collection must be active or the
   return value is undefined.
   

(Removed "a" before "text", and added "or..." to the end.)

Unless you want to define the return value to be NULL when
log collection is not active.  This might be cleaner.
I say this because I'm tempted to add "This can be tested
for with current_setting('logging_collector')='on'." to
the end of the paragraph.  But adding such text means
that the "logging_collector" setting is documented in multiple
places, in some sense, and such redundancy is best
avoided when possible.

I don't see a problem with defining the return value to be
NULL -- so long as it's defined to be NULL when there is
no current log file.  This would be different from defining
it to be NULL when log collection is off.  Although not
very different it should be clear that using pg_currrent_logfile()
to test whether log collection is on is not a good practice.
Perhaps some text like?:

   
pg_current_logfile returns the name of the
   current log file used by the logging collector, as
   text. NULL is returned
   and a NOTICE raised when no log file exists.
   

(I'm going to have to think some more about the raising of the
notice, and of the other error handling in your code.
I've not paid it any attention and error handling is always
problematic.)

Regards,

Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Wed, 23 Mar 2016 23:22:26 +0100
Gilles Darold  wrote:

> Thanks for the reminder, here is the v3 of the patch after a deeper
> review and testing. It is now registered to the next commit fest under
> the System Administration topic.

I am going to try reviewing your patch.  I don't feel entirely
confident, but should be able to provide at least some help.

I've not yet even tried building it, but the the first thing I notice
is that you're going to need to use pgrename() of src/port/dirmod.c
in order to get an atomic update of the pg_log_file file.

I believe this is the right approach here.  Any other program
will always see a fully-formed file content.

The typical way this works is: you make a new file with new
content, then rename the new file to the old file name.
This makes the new file name go away and the old file
content go away and, effectively, replaces
the content of your file with new content.

You'll want to look at other places where pg uses pgrename()
to see what sort of name you should give to the new file.
If it was me, I'd just stick a dot in front, calling it
".pg_log_file" but we want to be consistent with existing
practice.

I'd imagine that you'd create the new file in the same directly
as pg_log_file, that being the usual way to ensure that both
files are in the same file system (a requirement).  But when
you're looking at other uses of pgrename() in the existing code
it wouldn't hurt to see check what that code is doing regards
placement of the new file in the file system.

If you have any guidance/scripts/whatever which support
testing your patch please send it my way.  Thanks.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] PQsendQuery+PQgetResult+PQsetSingleRowMode limitations and support

[Karl O. Pinc]

On Fri, 1 Apr 2016 05:57:33 +0200
"Shulgin, Oleksandr" <oleksandr.shul...@zalando.de> wrote:

> On Apr 1, 2016 02:57, "Karl O. Pinc" <k...@meme.com> wrote:
> >
> > I assume there are no questions about supporting a
> > similar functionality only without PQsetSingleRowMode,
> > as follows:  
> 
> Sorry, but I don't see what is your actual question here?

The question is whether or not the functionality of the first
script is supported.  I ask since Bruce was surprised to see
this working and questioned whether PG was intended to behave
this way.

> Both code examples are going to compile and work, AFAICS. The
> difference is that the latter will try to fetch the whole result set
> into client's memory before returning you a PGresult.

Thanks for the clarification.  For some reason I recently
got it into my head that the libpq buffering was on the server side,
which is really strange since I long ago determined it was
client side.




Karl <k...@meme.com>
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] PQsendQuery+PQgetResult+PQsetSingleRowMode limitations and support

[Karl O. Pinc]

Hi,

Bruce Momjian suggested I write and ask about using libpq
to submit multiple SQL statements to the backend, and
then get results for each of the submitted statements,
row-by-row without server-side caching of the results.

Bruce wrote:
> I think this would be good
> to post to hackers to get a general discussion of the limitations of
> this approach and allow to communicate if this is something we want
> those interfaces to support.  My guess is this never used to work, but
> now it does.

As I read the documentation this functionality is supported.
(Although I do believe that the wording could be more clear.)

http://www.postgresql.org/docs/9.5/static/libpq-single-row-mode.html

And (I suppose):
http://www.postgresql.org/docs/9.5/static/libpq-async.html

FWIW, I would use such functionality to support an
interactive interface for users wishing to write SQL
and query the db directly.  Like psql does, only not
from the command line.

The following example program exhibits this functionality.
It runs on Debian Jesse (8.3) postgresql 9.4 (from the
Debian repos).

--
/*
 * byrow.c
 *
 *  Test that libpq, the PostgreSQL frontend library, can be given
 *  multiple statements and get the results of executing each,
 *  row-by-row without server side buffering.
 */
#include 
#include 
#include 

int stmtcnt = 0;

static void
exit_nicely(PGconn *conn)
{
  PQfinish(conn);
  exit(1);
}

static void
setting_failed(PGconn *conn)
{
  fprintf(stderr, "Unable to enter single row mode: %s\n",
  PQerrorMessage(conn));
  exit_nicely(conn);
}

int
main(int argc, char **argv)
{
  const char *conninfo;
  PGconn *conn;
  PGresult   *res;
  int first = 1;
  int nFields;
  int i,
j;

  /* Construct some statements to execute. */
  char *stmts = "select * from pg_database;\n"
"select * from pg_roles;\n"
"select count(*) from pg_tables;\n";

  /*
   * If the user supplies a parameter on the command line, use it as
   * the conninfo string; otherwise default to setting
   * dbname=postgres and using environment variables or defaults for
   * all other connection parameters.
   */
  if (argc > 1)
conninfo = argv[1];
  else
conninfo = "dbname = postgres";

  /* Make a connection to the database */
  conn = PQconnectdb(conninfo);

  /* Check to see that the backend connection was successfully made */
  if (PQstatus(conn) != CONNECTION_OK)
{
  fprintf(stderr, "Connection to database failed: %s",
  PQerrorMessage(conn));
  exit_nicely(conn);
}

  /* Send our statements off to the server. */
  if (!PQsendQuery(conn, stmts))
{
  fprintf(stderr, "Sending statements to server failed: %s\n",
  PQerrorMessage(conn));
  exit_nicely(conn);
}
  
  /* We want results row-by-row. */
  if (!PQsetSingleRowMode(conn))
{
  setting_failed(conn);
}

  /* Loop through the results of our statements. */
  while (res = PQgetResult(conn))
{

  switch (PQresultStatus(res))
{
case PGRES_TUPLES_OK:  /* No more rows from current query. */
  {
/* We want the next statement's results row-by-row also. */
if (!PQsetSingleRowMode(conn))
  {
PQclear(res);
setting_failed(conn);
  }
first = 1;
break;
  }

case PGRES_SINGLE_TUPLE:
  {
if (first)
  {
/* Produce a "nice" header" */
printf("\n%s\nResults of statement number %d:\n\n",
   "-"
   "-",
   stmtcnt++);

/* print out the attribute names */
nFields = PQnfields(res);
for (i = 0; i < nFields; i++)
  printf("%-15s", PQfname(res, i));
printf("\n\n");

first = 0;
  }

/* print out the row */
for (j = 0; j < nFields; j++)
  printf("%-15s", PQgetvalue(res, 0, j));
printf("\n");

break;
  }
default:
  /* Always call PQgetResult until it returns null, even on
   * error. */
  {
fprintf(stderr,
"Query execution failed: %s", PQerrorMessage(conn));
  }
}

  PQclear(res);
}

  /* close the connection to the database and cleanup */
  PQfinish(conn);

  return 0;
}
--

(You may recognize much of the code above because it was cribbed
from the libpq docs example #1.)


I assume there are no questions about supporting a
similar functionality only without PQsetSingleRowMode,
as follows:
--
/*
 * testmultistmt.c
 *
 *  Test that 

Re: [HACKERS] backup.sgml-cmd-v003.patch

[Karl O. Pinc]

On 11/08/2013 02:12:56 PM, Robert Haas wrote:
 On Fri, Nov 8, 2013 at 1:33 PM, Joshua D. Drake 
 j...@commandprompt.com
 wrote:
  superuser privileges; it's the selective-dump case where you can
 often
  get by without them.  I've attached a proposed patch along these
 lines
  for your consideration.
  That's fair.
  Should I go ahead and apply that portion, then?
  I am certainly not opposed.
 
 OK, done.

So  Joshusa/Ivan  Do you want to send another
version of the patch with the committed changes
omitted for further review?


Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein


-- 
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] backup.sgml-cmd-v003.patch

[Karl O. Pinc]

On 11/08/2013 03:42:56 PM, Joshua D. Drake wrote:
 
 On 11/08/2013 12:18 PM, Karl O. Pinc wrote:
 
  On 11/08/2013 02:12:56 PM, Robert Haas wrote:
  On Fri, Nov 8, 2013 at 1:33 PM, Joshua D. Drake
  j...@commandprompt.com
  wrote:

  Should I go ahead and apply that portion, then?
  I am certainly not opposed.
 
  OK, done.
 
  So  Joshusa/Ivan  Do you want to send another
  version of the patch with the committed changes
  omitted for further review?
 
 
 I have no problem having Ivan submit another patch, outside of the 
 multiple patch requirement. If you guys will accept a single patch
 file 
 with the agreed changes, then we are good.

I can't say what will be accepted and haven't really kept
track of what has been accepted.  If there's more that
you want to patch then send that and we'll
work from there.



Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein


-- 
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] backup.sgml-cmd-v003.patch

[Karl O. Pinc]

Hi Robert,

On 09/27/2013 05:56:52 AM, Robert Haas wrote:

 1. Attempting to encourage people to consider custom format dumps.
  
 What's important is what you can do...

Your critique seems obvious in retrospect.  Sorry you had
to step in here and do my job.  The above point is particularly
salient.  I will try to keep it in mind in the future.

Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein


-- 
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] backup.sgml-cmd-v003.patch

[Karl O. Pinc]

On 09/26/2013 12:15:25 PM, Ivan Lezhnjov IV wrote:
 
 On Sep 3, 2013, at 6:56 AM, Karl O. Pinc k...@meme.com wrote:
 
  On 07/31/2013 12:08:12 PM, Ivan Lezhnjov IV wrote:
  
  Patch filename: backup.sgml-cmd-v003.patch
  
  The third version of this patch takes into consideration feedback
  received after original submission (it can be read starting from
 this
  message http://www.postgresql.org/message-id/CA
  +Tgmoaq-9D_mst113TdW=ar8mpgbc+x6t61azk3emhww9g...@mail.gmail.com)
  
  Essentially, it addresses the points that were raised in community
  feedback and offers better worded statements that avoid implying
 that
  some features are being deprecated when it isn't the case. We also
  spent some more time polishing other details, like making
 adjustments
  to the tone of the text so that it sounds more like a manual, and 
  less
  like a blog post. More importantly, this chapter now makes it 
 clear
  that superuser privileges are not always required to perform a
  successful backup because in practice as long as the role used to 
  make
  a backup has sufficient read privileges on all of the objects a
 user
  is interested in it's going to work just fine. We also mention and
  show examples of usage for pg_restore and pigz alongside with 
 gzip,
  and probably something else too.

  ---
  
  Cleaned up and clarified here and there.
  
  The bit about OIDs being depreciated might properly belong in 
  a separate patch.  The same might be said about adding mention of
 pigz.
  If you submit these as separate patch file attachments
  they can always be applied in a single commit, but the reverse is 
  more work for the committer.  (Regardless, I see no reason to
  have separate commitfest entries or anything other than multiple
  attachments to the email that finalizes our discussion.)
 
 Hello,
 
 took me a while to get here, but a lot has been going on...

No worries.
 
 Okay, I'm new and I don't know why a single patch like this is more
 work for a commiter? Just so I understand and know.

Different committers have different preferences but the general rule
is that it's work to split a patch into pieces if you don't like the
whole thing but it's easy to apply a bunch of small patches and
commit them all at once.  Further each commit should represent
a single feature or conceptual change.  Again, preferences vary
but I like to think that a good rule is that 1 commit should
be able to be described in a sentence, and not a run-on sentence
either that says I did this and I also did that and something else.
So if there's a question in your mind about whether a committer
will want your entire change, or if your patch changes unrelated
things then it does not hurt to submit it as separate patches.
All the patches can be attached to a single email and part of
a single commitfest tracking entry, usually.  No need to get
crazy.  These are just things to think about.

In your case I see 3 things happening:

oid depreciation

custom format explanation

pigz promotion


  My thought is that the part beginning with The options in detail
  are: should not describe all the possibilities for the --format
  option, that being better left to the reference section.  Likewise,
  this being prose, it might be best to describe all the options
  in-line, instead of presented as a list.  I have left it as-is
  for you to improve as seen fit.
 
 Agreed, it probably looks better as a sentence.

Looks good.

 
  
  I have frobbed your programlisting to adjust the indentation and
  line-wrap style.  I submit it here for consideration in case this
  style is attractive.  This is nothing but conceit.  We should use
 the
  same style used elsewhere in the documentation.  

 Looks good to me.

I fixed the missing \ I messed up on last time
and slightly re-worded the previous sentence.

I've grep-ed through the sgml looking for multi-line shell scripts
and found only 1 (in sepgsql.sgm).  I don't see a conflict with
the formatting/line-break convention used in the patch, although
it does differ slightly in indentation.  I'm leaving the shell
script formatting in the patch as-is for the committer to judge.
(I like the way it looks but it is not a traditional style.)

 
  
  I don't know that it's necessary to include pigz examples, because
 it
  sounds like pigz is a drop-in gzip replacement.  I've left your
  examples in, in case you feel they are necessary.
 
 We do. We believe it can encourage more people to consider using it.
 The way we see it, most people seem to be running mutlicore systems
 these days, yet many simply are not aware of pigz.

Ok.  It's your patch.

  
  The existing text of the SQL Dump section could use some alteration
 to
  reduce redundancy and add clarity.  I'm thinking specifically of
  mention of pg_restore as being required to restore custom format
  backups and of the default pg_dump output being not just plain
 text
  but being a collection of SQL commands.  Yes, the latter is obvious
  upon reflection

Re: [HACKERS] backup.sgml-cmd-v003.patch

[Karl O. Pinc]

On 09/02/2013 10:56:54 PM, Karl O. Pinc wrote:
 I have frobbed your programlisting to adjust the indentation and
 line-wrap style.  

Oops.  Somehow left a \ out of this.  Anyhow, you get the idea.



Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein


-- 
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] updatable/deletable terminology

[Karl O. Pinc]

On 08/07/2013 08:19:03 PM, Peter Eisentraut wrote:
 We have these two error messages:
 
 To make the view updatable, provide an unconditional ON UPDATE DO
 INSTEAD rule or an INSTEAD OF UPDATE trigger.
 
 and
 
 To make the view updatable, provide an unconditional ON DELETE DO
 INSTEAD rule or an INSTEAD OF DELETE trigger.
 
 I think it's a bit strange to claim that adding a DELETE rule/trigger
 makes a view *updatable*.  I suspect someone thought they would apply
 the term updatable in an SQL standard sense, but that seems
 backwards,
 because you get to these error conditions exactly because the view as
 defined was not Updatable(tm).

Isn't the problem here that you need a word, instead of updateable,
to indicate that table content may be changed (insert/update/
delete) through the view?

So...   to allow the view to influence table content


Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Doc Patch: Subquery section to say that subqueries can't modify data

[Karl O. Pinc]

Hi,

The attached documentation patch, doc-subqueries-v1.patch,
applies against head.

I wanted to document that subqueries can't modify data.
This is mentioned in the documentation for SELECT and
implied elsewhere but I was looking for something more
than an 'in-passing' mention.  

(I wrote a bad query,
modifying data in a subquery, couldn't recall where
it was documented that you can't do this, and couldn't
find the answer from the TOC or the index.  Now that
there's lots of statements with RETURNING clauses
it's natural to want to use them in subqueries.)

There seemed to be no good place to put this in the
tutorial section of the documentation.  I wound
up adding a small, 2 paragraph, section describing subqueries to
the chapter on queries.  Although the first paragraph
echos what's already documented the utility of
subqueries is such that it's nice to have a
place in the tutorial that serves as a single point of
reference.

The alternative seemed to be to put the 2nd paragraph
in 9.22. Subquery Expressions, and this didn't seem
to fit well.

The last 2 sentences of the first paragraph are
something in the way of helpful hints and may not
be appropriate, or even accurate.  I've left them in 
for review.

Regards,

Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/queries.sgml b/doc/src/sgml/queries.sgml
index d7b0d73..acc6686 100644
--- a/doc/src/sgml/queries.sgml
+++ b/doc/src/sgml/queries.sgml
@@ -549,7 +549,7 @@ SELECT * FROM my_table AS m WHERE my_table.a gt; 5;-- wrong
 SELECT * FROM people AS mother JOIN people AS child ON mother.id = child.mother_id;
 /programlisting
  Additionally, an alias is required if the table reference is a
- subquery (see xref linkend=queries-subqueries).
+ subquery (see xref linkend=queries-subquery-derived-tables).
 /para
 
 para
@@ -590,10 +590,10 @@ SELECT a.* FROM (my_table AS a JOIN your_table AS b ON ...) AS c
 /para
/sect3
 
-   sect3 id=queries-subqueries
-titleSubqueries/title
+   sect3 id=queries-subquery-derived-tables
+titleSubquery Derived Tables/title
 
-indexterm zone=queries-subqueries
+indexterm zone=queries-subquery-derived-tables
  primarysubquery/primary
 /indexterm
 
@@ -1315,6 +1315,46 @@ SELECT DISTINCT ON (replaceableexpression/replaceable optional, replaceab
  /sect1
 
 
+ sect1 id=queries-subqueries
+  titleSubqueries/title
+
+  indexterm zone=queries-subqueries
+   primarysubquery/primary
+  /indexterm
+
+  indexterm zone=queries-subqueries
+   primarysub-select/primary
+  /indexterm
+
+  para
+   Subqueries, also called sub-selects, are queries written within
+   parenthesis in the text of larger queries.  The values produced by
+   subqueries may be scalar, used within expressions as described
+   in xref linkend=sql-syntax-scalar-subqueries, or tabular.  When
+   tabular, subqueries may substitute for tables, as described
+   in xref linkend=queries-subquery-derived-tables, generate array
+   content, as described
+   in xref linkend=sql-syntax-array-constructors, have their
+   result content tested within expressions, as described
+   in xref linkend=functions-subquery, or be used in other
+   contexts.  Often either joins or subqueries can be used to produce
+   different query plans yielding identical output.  Which technique
+   is appropriate depends upon circumstance but it is worth noting
+   that more work has gone into query planner join optimization.
+  /para
+
+  para
+   Subqueries may not modify database
+   content.  link linkend=queries-withCommon Table
+   Expressions/link are one way to integrate data returned by data
+   modification statements,
+   i.e. commandINSERT/command/commandUPDATE/command/commandDELETE/command
+   statements with literalRETURNING/literal clauses, into larger
+   queries.
+  /para
+ /sect1
+
+
  sect1 id=queries-union
   titleCombining Queries/title
 

-- 
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] Re: Doc Patch: Subquery section to say that subqueries can't modify data

[Karl O. Pinc]

Good points.

On 08/06/2013 05:15:28 PM, David Johnston wrote:
 Instead of simply expanding the section on sub-queries, which may
 still be
 worthwhile, it seems that we have effectively introduced a new kind
 of
 query - namely one that mixes both query DDL and update DDL into a
 kind of
 hybrid query.  An entire section describing the means to implement
 these
 queries and the limitations thereof would seem advisable as the
 current
 material is spread throughout the documentation.

The proposed patch is an attempt to provide a base upon
which to build such a section.

 
 Some areas to address would:
 
 Select queries that cause/utilize:
 
 function-based modifications
 CTE-based modifications
 FDW/dblink-based modifications

While it'd be nice to have a full set of examples and an
exhaustive list of what constitutes modification and
what does not it should be enough to state where, in
this sort of hybrid query, modification is allowed
and where not.  Of course more detail is needed if
the different kinds of modification above are restricted
in different ways.

 
 I guess the main question is if someone were to put this together
 would it
 likely be included in the queries section of the documentation. 

If this section is not to be part of the Query chapter then it
surely also does not belong in the Data Manipulation chapter
(or the Data Definition chapter),
if for no other reason than the information presented in the
Query chapter is necessary to understand the subject.
To me that means it needs it's own chapter, probably immediately
following the Query chapter.  I can't think what to call
such a chapter.

 The proposed patch; while warranting a technical review (namely that
 the
 presence of functions in a sub-select can cause the sub-query to
 update the
 database), seems to add one more place to go find this information
 without
 adding a central index or summary that someone learning the system
 could
 directly comprehend/learn as opposed to it being some
 allowed/disallowed
 side-effect to something else.

I'm less worried about data modifying functions than I am
about the patch's language regards other sorts of modifications.
Although unstated, it should be clear that data modifying
functions that are executed with a SELECT statement do modify data.
Where the patch is lacking is noting that schema alterations and FDW 
modifications also have restrictions.  I don't feel particularly 
qualified regards where either are allowed, or not, although
I could probably get it right after some research.

Regards,

Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein


-- 
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] Make targets of doc links used by phpPgAdmin static

[Karl O. Pinc]

On 06/05/2013 09:13:45 PM, Peter Eisentraut wrote:
 On Tue, 2013-06-04 at 22:27 -0500, Karl O. Pinc wrote:
  On 06/04/2013 10:16:20 PM, Peter Eisentraut wrote:
   On Tue, 2013-05-07 at 23:18 -0400, Alvaro Herrera wrote:
Peter Eisentraut wrote:
 On Tue, 2013-05-07 at 00:32 -0500, Karl O. Pinc wrote:
  Attached is a documentation patch against head which makes
  static the targets of the on-line PG html documentation 
 that
  are referenced by the phpPgAdmin help system.e
 
 done

I wonder about backpatching this to 9.2 ?
   
   done
  
  Will this be in the next point release?  Or just when
  will it go live?
 
 I don't know when it goes to the web site, but it will be in the next
 point release.

Ok.  Thanks.

 
  This is not a huge problem but it does break some
  existing links into the 9.2 PG docs.
 
 Well, if it doesn't help you, I can back it out again.

It doesn't make me miserable and it sounds like
other people want it.  In theory the automatically
generated anchors could change and break things
anyway.  I'm happy to let somebody
else decide what to do.

Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein



-- 
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] Make targets of doc links used by phpPgAdmin static

[Karl O. Pinc]

On 06/04/2013 10:16:20 PM, Peter Eisentraut wrote:
 On Tue, 2013-05-07 at 23:18 -0400, Alvaro Herrera wrote:
  Peter Eisentraut wrote:
   On Tue, 2013-05-07 at 00:32 -0500, Karl O. Pinc wrote:
Attached is a documentation patch against head which makes
static the targets of the on-line PG html documentation that
are referenced by the phpPgAdmin help system.e
   
   done
  
  I wonder about backpatching this to 9.2 ?
 
 done

Will this be in the next point release?  Or just when
will it go live?

This is not a huge problem but it does break some
existing links into the 9.2 PG docs.

Regards,

Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Make targets of doc links used by phpPgAdmin static

[Karl O. Pinc]

Hi,

Attached is a documentation patch against head which makes
static the targets of the on-line PG html documentation that
are referenced by the phpPgAdmin help system.

Apply with patch -p1 at the top of the pg code tree.

The phpPgAdmin project is a web interface into PG.  It
contains help links which reference the PG on-line docs.  At
present, each time there's a new PG release many of the
internal ids within PGs html doc pages change, and the
phpPgAdmin code must track such changes.  This
patch makes static those ids referenced by phpPgAdmin.

Of course phpPgAdmin will always need to adjust to changes
in the PG docs but this patch will eliminate periodic
annoying scutwork.

Regards,

Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index e9135bf..bae2e97 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -258,7 +258,7 @@ CREATE TABLE products (
even if the value came from the default value definition.
   /para
 
-  sect2
+  sect2 id=ddl-constraints-check-constraints
titleCheck Constraints/title
 
indexterm
@@ -482,7 +482,7 @@ CREATE TABLE products (
/tip
   /sect2
 
-  sect2
+  sect2 id=ddl-constraints-unique-constraints
titleUnique Constraints/title
 
indexterm
@@ -569,7 +569,7 @@ CREATE TABLE products (
/para
   /sect2
 
-  sect2
+  sect2 id=ddl-constraints-primary-keys
titlePrimary Keys/title
 
indexterm
@@ -1168,7 +1168,7 @@ CREATE TABLE circles (
here.
   /para
 
-  sect2
+  sect2 id=ddl-alter-adding-a-column
titleAdding a Column/title
 
indexterm
@@ -1212,7 +1212,7 @@ ALTER TABLE products ADD COLUMN description text CHECK (description lt;gt; '')
   /tip
   /sect2
 
-  sect2
+  sect2 id=ddl-alter-removing-a-column
titleRemoving a Column/title
 
indexterm
@@ -1239,7 +1239,7 @@ ALTER TABLE products DROP COLUMN description CASCADE;
/para
   /sect2
 
-  sect2
+  sect2 id=ddl-alter-adding-a-constraint
titleAdding a Constraint/title
 
indexterm
@@ -1267,7 +1267,7 @@ ALTER TABLE products ALTER COLUMN product_no SET NOT NULL;
/para
   /sect2
 
-  sect2
+  sect2 id=ddl-alter-removing-a-constraint
titleRemoving a Constraint/title
 
indexterm
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index bc1cd59..60fa1a8 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -145,7 +145,7 @@
 /para
/sect2
 
-   sect2
+   sect2 id=extend-type-system-domains
 titleDomains/title
 
 para


-- 
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] Doc patch, normalize search_path in index

[Karl O. Pinc]

On 01/25/2013 12:35:49 PM, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  I have applied a modified version of your patch that creates
 separate
  secondary index references for search_path.
 
 This patch seems pretty bizarre.  What is the difference between a
 configuration parameter and a run-time setting?  Why would you
 point people to two different places for those two terms?

One was the setting/querying of the parameter, the other the
purpose/impact of the setting.  Somewhere in there I'm also
thinking it's a matter of the lexical token used v.s.
the functionality manipulated.  One would have an _ in
between the words of the index entry, the other wouldn't.
The distinction would get all messed up by the browser 
when it underlined hyperlinks.

I no longer recall the point
in making the distinction, although I believe that it came
from something to do with the vocabulary already in place
involving indexing settings and what they do.  Or maybe I made it up.
In any case I don't think that the patch the Peter pushed 
when he closed the commitfest entry made any distinction
between the token and the concept.  Given that the difference
in sort order is the presence/absence of an _, and the
way hyperlinks are represented, I think this made the
the resulting index look best.

There was also a use of search path to secure functions
indexing wrapped in with the rest of the patch.

Hope this helps.



Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein



-- 
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] [PATCH] PL/Python: Add spidata to all spiexceptions

[Karl O. Pinc]

On 01/09/2013 01:08:39 PM, Jan Urbański wrote:

  I can see arguments to be made for both sides.  I'm asking that you
  think it through to the extent you deem necessary and make
  changes or not.  At that point it should be ready to send
  to a committer.  (I'll re-test first, if you make any changes.)
 
 Oh my, I have dropped the ball on this one in the worst manner
 possible. 
 Sorry!

My fault too.  I've been thinking I should write to remind you.

 
 I actually still prefer to keep the signatures of
 PLy_get_spi_sqlerrcode 
 and PLy_get_spi_error_data similar, so I'd opt for keeping the patch 
 as-is :)

I will send it on to a committer.

Regards,


Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein



-- 
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] Submission Review: User control over psql error stream

[Karl O. Pinc]

Hi Allastair,

On 12/28/2012 02:33:03 PM, Alastair Turner wrote:

 Sorry for the slow reply ...

Such is life.

 The discussion needs to be a little broader than stdout and stderr,
 there are currently three output streams from psql:
  - stdout - prompts, not tabular output such as the results from \set
 and \c
  - stderr - errors, notices, ...
  - query output - result from queries and \ commands which return
 tables such as \l - this is what is currently piped or redirected by
 \o
 
 I see a patch like this adding a fourth - diagnostic output. While
 this would probably be the same as stderr initially but I think that
 the option to make them subtly different should be kept open.

What is the distinction between diagnostic output and stderr?
My thought would be to distinguish between the server error
stream and any errors that psql might generate.

(I have some thoughts on syntax but am not yet ready to respond
to your proposal.)

Regards,

Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein



-- 
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] Doc patch, further describe and-mask nature of the permission system

[Karl O. Pinc]

On 12/16/2012 12:56:22 AM, Peter Eisentraut wrote:
 On Mon, 2012-12-10 at 20:48 -0600, Karl O. Pinc wrote:
  On 11/14/2012 02:35:54 PM, Karl O. Pinc wrote:
   On 11/13/2012 08:50:55 PM, Peter Eisentraut wrote:
On Sat, 2012-09-29 at 01:16 -0500, Karl O. Pinc wrote:
 This patch makes some sweeping statements.

Unfortunately, they are wrong.
   
   I will see if anything can be salvaged.
  
  Here's another try.
  (I bundled changes to both paragraphs into a single
  patch.)
  
  grants-of-roles-are-additive_v3.patch
 
 I don't get the point of this change, especially why you are trying 
 to
 liken the roles system to the object hierarchy, when they are clearly
 different and unrelated.

It seems to me the that the permission system follows the object system
hierarchy in those cases where different levels of the object
hierarchy may have identical permissions.  The exceptions being
permissions like USAGE, which seems to be a convenient common lexical
token but mean (and need to mean) something entirely different
at each level of the object hierarchy.   ALL is also confuses the
issue, since it means all permissions which work at this level
of the object hierarchy and not all permissions so, say,
granting ALL to a database says nothing about INSERT permission.

I'm (clearly) not steeped in the pg permission system, but it
does seem that where permissions are shared between levels
of the object hierarchy there is a consistency in the
resulting interaction when granting/revoking at different
levels of the object hierarchy.  Perhaps this is ipso facto
(counterexamples being automatically designated as
not shared by nature of the premise :)
or perhaps more an artifact of my attention than the
result of any sort of design.  Anyway, my intent is to point
out this consistency.  Since the way in which interactions
between permissions set at different levels of the object
hierarchy is sometimes useful I go on to describe how to
replicate the behavior and apply it outside the object
hierarchy.

In any case I thought the elaboration would be helpful.  
I had a few minutes and cooked it up.  If you don't don't think
it should go in then reject it.  As noted already in the
docs, permissions are different at different levels of the
object hierarchy, but similar enough to describe in one place.
I was hoping to provide a possible framework for thinking
about permission interactions between object hierarchy levels 
where such occur.  Without any sort of framework everything
becomes a special case and it's hard to keep track of.

Thanks for spending time on it.  If there's anything about
it that appeals then I will continue to work under
your direction.

Regards,

Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein




-- 
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] Add big fat caution to pg_restore docs regards partial db restores

[Karl O. Pinc]

On 12/16/2012 12:51:06 AM, Peter Eisentraut wrote:

 I'm going to set this patch as returned with feedback for now.

Ok.  At this point I don't have a vision for improving it
so it might sit there untouched.   Maybe someone else
will step forward and make it better.

Regards,

Karl k...@meme.com
Free Software:  You don't pay back, you pay forward.
 -- Robert A. Heinlein



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers