Re: [HACKERS] Re: [HACKERS] Patch für MAP_HUGETLB for mmap() shared memory

[Andres Freund]

Hi,

On 2013-06-28 23:03:22 -0400, Bruce Momjian wrote:
> Did we decide against specifying huge pages in Postgres?

I don't think so. We need somebody to make some last modifications to
the patch though and Christian doesn't seem to have the time atm.

I think the bigger memory (size of the per process page table) and #cpus
(number of pg backends => number of page tables allocated) the more
imporant it gets to implement this. We already can spend gigabytes of
memory on those on big systems.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

[HACKERS] [PATCH] big test separation POC

[Fabien COELHO]

Dear hackers,

Per various discussion about the potential impact of Robins non regression 
tests, here is a poc patch to separate big tests from others.


"paralle_schedule" holds usual tests, "big_schedule" holds big tests.

The makefile derives serial_schedule, parallel_big_schedule and 
serial_big_schedule from the above descriptions.


Then:
 - make check in regress does usual tests
 - make bigcheck in regress does both usual and big tests

I'm not sure about what happens under vpath.

I have no opinion about what should be considered a big test.

--
Fabien.diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 7309b00..9243c7b 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -86,7 +86,7 @@ regress_data_files = \
 	$(wildcard $(srcdir)/output/*.source) \
 	$(filter-out $(addprefix $(srcdir)/,$(input_files)),$(wildcard $(srcdir)/sql/*.sql)) \
 	$(wildcard $(srcdir)/data/*.data) \
-	$(srcdir)/parallel_schedule $(srcdir)/serial_schedule $(srcdir)/resultmap
+	$(srcdir)/parallel_schedule $(srcdir)/big_schedule $(srcdir)/resultmap
 
 install-tests: all install install-lib installdirs-tests
 	$(MAKE) -C $(top_builddir)/contrib/spi install
@@ -132,6 +132,26 @@ tablespace-setup:
 ## Run tests
 ##
 
+# derive schedules
+derived_schedules = serial_schedule parallel_big_schedule serial_big_schedule
+
+serial_schedule: parallel_schedule
+	echo '# this file is automatically generated, do not edit!' > $@
+	egrep '^(test|ignore):' $< | \
+	while read op list ; do \
+	  for test in $$list ; do \
+	echo "$$op $$test" ; \
+	  done ; \
+	done >> $@
+
+parallel_big_schedule: parallel_schedule big_schedule
+	echo '# this file is automatically generated, do not edit!' > $@
+	cat $^ >> $@
+
+serial_big_schedule: serial_schedule big_schedule
+	echo '# this file is automatically generated, do not edit!' > $@
+	cat $^ >> $@
+
 REGRESS_OPTS = --dlpath=. $(EXTRA_REGRESS_OPTS)
 
 check: all tablespace-setup
@@ -152,11 +172,11 @@ runcheck: check
 runtest: installcheck
 runtest-parallel: installcheck-parallel
 
-bigtest: all tablespace-setup
-	$(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/serial_schedule numeric_big
+bigtest: all tablespace-setup serial_big_schedule
+	$(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/serial_big_schedule
 
-bigcheck: all tablespace-setup
-	$(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) numeric_big
+bigcheck: all tablespace-setup parallel_big_schedule
+	$(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_big_schedule $(MAXCONNOPT)
 
 
 ##
@@ -166,7 +186,7 @@ bigcheck: all tablespace-setup
 clean distclean maintainer-clean: clean-lib
 # things built by `all' target
 	rm -f $(OBJS) refint$(DLSUFFIX) autoinc$(DLSUFFIX) dummy_seclabel$(DLSUFFIX)
-	rm -f pg_regress_main.o pg_regress.o pg_regress$(X)
+	rm -f pg_regress_main.o pg_regress.o pg_regress$(X) $(derived_schedules)
 # things created by various check targets
 	rm -f $(output_files) $(input_files)
 	rm -rf testtablespace
diff --git a/src/test/regress/big_schedule b/src/test/regress/big_schedule
new file mode 100644
index 000..4058499
--- /dev/null
+++ b/src/test/regress/big_schedule
@@ -0,0 +1,3 @@
+# these are big tests not run by default
+# these test are expected serial, only put one test per line
+test: numeric_big
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
deleted file mode 100644
index d6eaa7a..000
--- a/src/test/regress/serial_schedule
+++ /dev/null
@@ -1,139 +0,0 @@
-# src/test/regress/serial_schedule
-# This should probably be in an order similar to parallel_schedule.
-test: tablespace
-test: boolean
-test: char
-test: name
-test: varchar
-test: text
-test: int2
-test: int4
-test: int8
-test: oid
-test: float4
-test: float8
-test: bit
-test: numeric
-test: txid
-test: uuid
-test: enum
-test: money
-test: rangetypes
-test: strings
-test: numerology
-test: point
-test: lseg
-test: box
-test: path
-test: polygon
-test: circle
-test: date
-test: time
-test: timetz
-test: timestamp
-test: timestamptz
-test: interval
-test: abstime
-test: reltime
-test: tinterval
-test: inet
-test: macaddr
-test: tstypes
-test: comments
-test: geometry
-test: horology
-test: regex
-test: oidjoins
-test: type_sanity
-test: opr_sanity
-test: insert
-test: create_function_1
-test: create_type
-test: create_table
-test: create_function_2
-test: copy
-test: copyselect
-test: create_misc
-test: create_operator
-test: create_index
-test: create_view
-test: create_aggregate
-test: create_function_3
-test: create_cast
-test: constraints
-test: triggers
-test: inherit
-test: create_table_like
-test: typed_table
-test: vacuum
-test: drop_if_exists
-test: updatable_views
-test: sanity_check
-test: errors
-test: select
-test: select_into
-test: select_distinct
-test: select_distinct_on
-test: select_implicit
-test: select_having
-test: subselect
-test: union
-test: case
-test: join
-tes

Re: [HACKERS] New regression test time

[Alvaro Herrera]

Josh Berkus escribió:
> Hackers,
> 
> Per discussion on these tests, I ran "make check" against 9.4 head,
> applied all of the regression tests other than DISCARD.
> 
> Time for 3 "make check" runs without new tests: 65.9s
> 
> Time for 3 "make check runs with new tests: 71.7s
> 
> So that's an increase of about 10% in test runtime (or 2 seconds per run
> on my laptop),

I see two problems with this report:
1. it creates a new installation for each run,
2. it only uses the serial schedule.

I care more about the parallel schedule than the serial one, because
since it obviously runs in less time, then I can run it often and not
worry about how long it takes.  On the other hand, the cost of the extra
initdb obviously means that the percentage is a bit lower than if you
were to compare test run time without considering the initdb step.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] New regression test time

[Fabien COELHO]

How did you evaluate that coverage increased "greatly"? I am not
generally against these tests but I'd be surprised if the overall test
coverage improved noticeably by this. Which makes 10% runtime overhead
pretty hefty if the goal is to actually achieve a high coverage.


I was relying on Robins' numbers of coverage:


Those improvements rather likely end up being an improvement a good bit
less than one percent for the whole binary.


Yes, but it is a valuable percent nevertheless.

As I understand it, the coverage is about the tested command logic. A lot 
this logic is dedicated to check permissions (can you add an operator to 
this database? ...) and to verify required conditions (is the function 
proposed for operator has the right signature? does the operator overwrite 
an existing one? ...).


--
Fabien.


--
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] [GENERAL] pg_upgrade -u

[Peter Eisentraut]

On 6/28/13 9:43 PM, Bruce Momjian wrote:
> On Fri, Jun 28, 2013 at 09:15:31PM -0400, Peter Eisentraut wrote:
>> On 6/28/13 6:06 PM, Bruce Momjian wrote:
>>> On Wed, May 29, 2013 at 09:44:00AM -0400, Peter Eisentraut wrote:
 On 5/28/13 10:55 PM, Bruce Momjian wrote:
> Wow, I never realized other tools used -U for user, instead of -u. 
> Should I change pg_upgrade to use -U for 9.4?  I can keep supporting -u
> as an undocumented option.

 It seems to me that that option shouldn't be necessary anyway.
 pg_upgrade should somehow be able to find out by itself what the
 superuser of the old cluster was.
>>>
>>> Uh, any idea how to do that?
>>
>> select rolname from pg_authid where oid = 10;
> 
> Uh, how do I know what username to connect as to issue that query?

single-user mode?



-- 
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: [HACKERS] Patch für MAP_HUGETLB for mmap() shared memory

[Bruce Momjian]

Did we decide against specifying huge pages in Postgres?

---

On Tue, Oct 30, 2012 at 09:16:07PM +0100, Christian Kruse wrote:
> Hey,
> 
> ok, I think I implemented all of the changes you requested. All but
> the ia64 dependent, I have to do more research for this one.
> 
> 
> Greetings,
>  CK

> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index b4fcbaf..66ed10f 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -1049,6 +1049,37 @@ include 'filename'
>
>   
>  
> + 
> +  huge_tlb_pages (enum)
> +  
> +   huge_tlb_pages configuration parameter
> +  
> +  
> +   
> +Enables/disables the use of huge tlb pages. Valid values are
> +on, off and 
> try.
> +The default value is try.
> +   
> +
> +   
> +With huge_tlb_pages set to on
> +mmap() will be called with 
> MAP_HUGETLB.
> +If the call fails the server will fail fatally.
> +   
> +
> +   
> +With huge_tlb_pages set to off 
> we
> +will not use MAP_HUGETLB at all.
> +   
> +
> +   
> +With huge_tlb_pages set to try
> +we will try to use MAP_HUGETLB and fall back to
> +mmap() without MAP_HUGETLB.
> +   
> +  
> + 
> +
>   
>temp_buffers (integer)
>
> diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
> index df06312..f9de239 100644
> --- a/src/backend/port/sysv_shmem.c
> +++ b/src/backend/port/sysv_shmem.c
> @@ -27,10 +27,14 @@
>  #ifdef HAVE_SYS_SHM_H
>  #include 
>  #endif
> +#ifdef MAP_HUGETLB
> +#include 
> +#endif
>  
>  #include "miscadmin.h"
>  #include "storage/ipc.h"
>  #include "storage/pg_shmem.h"
> +#include "utils/guc.h"
>  
>  
>  typedef key_t IpcMemoryKey;  /* shared memory key passed to 
> shmget(2) */
> @@ -61,6 +65,19 @@ typedef int IpcMemoryId;   /* shared memory ID 
> returned by shmget(2) */
>  #define MAP_FAILED ((void *) -1)
>  #endif
>  
> +#ifdef MAP_HUGETLB
> +#  ifdef __ia64__
> +#define PG_HUGETLB_BASE_ADDR (void *)(0x8000UL)
> +#define PG_MAP_HUGETLB (MAP_HUGETLB|MAP_FIXED)
> +#  else
> +#define PG_HUGETLB_BASE_ADDR (void *)(0x0UL)
> +#define PG_MAP_HUGETLB MAP_HUGETLB
> +#  endif
> +#else
> +#  define PG_MAP_HUGETLB 0
> +#endif
> +
> +
>  
>  unsigned long UsedShmemSegID = 0;
>  void*UsedShmemSegAddr = NULL;
> @@ -73,7 +90,6 @@ static void IpcMemoryDelete(int status, Datum shmId);
>  static PGShmemHeader *PGSharedMemoryAttach(IpcMemoryKey key,
>IpcMemoryId *shmid);
>  
> -
>  /*
>   *   InternalIpcMemoryCreate(memKey, size)
>   *
> @@ -342,6 +358,155 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long 
> id2)
>  }
>  
>  
> +#ifdef MAP_HUGETLB
> +#define HUGE_PAGE_INFO_DIR  "/sys/kernel/mm/hugepages"
> +
> +/*
> + *   static long InternalGetFreeHugepagesCount(const char *name)
> + *
> + * Attempt to read the number of available hugepages from
> + * /sys/kernel/mm/hugepages/hugepages-/free_hugepages
> + * Will fail (return -1) if file could not be opened, 0 if no pages are 
> available
> + * and > 0 if there are free pages
> + *
> + */
> +static long
> +InternalGetFreeHugepagesCount(const char *name)
> +{
> + int fd;
> + char buff[1024];
> + size_t len;
> + long result;
> + char *ptr;
> +
> + len = snprintf(buff, 1024, "%s/%s/free_hugepages", HUGE_PAGE_INFO_DIR, 
> name);
> + if (len == 1024) /* I don't think that this will happen ever */
> + {
> + ereport(huge_tlb_pages == HUGE_TLB_TRY ? DEBUG1 : WARNING,
> + (errmsg("Filename %s/%s/free_hugepages is too 
> long", HUGE_PAGE_INFO_DIR, name),
> +  errcontext("while checking hugepage size")));
> + return -1;
> + }
> +
> + fd = open(buff, O_RDONLY);
> + if (fd <= 0)
> + {
> + ereport(huge_tlb_pages == HUGE_TLB_TRY ? DEBUG1 : WARNING,
> + (errmsg("Could not open file %s: %s", buff, 
> strerror(errno)),
> +  errcontext("while checking hugepage size")));
> + return -1;
> + }
> +
> + len = read(fd, buff, 1024);
> + if (len <= 0)
> + {
> + ereport(huge_tlb_pages == HUGE_TLB_TRY ? DEBUG1 : WARNING,
> + (errmsg("Error reading from file %s: %s", buff, 
> strerror(errno)),
> +  errcontext("while checking hugepage size")));
> + close(fd);
> + return -1;
> + }
> +
> + /*
> +  * If the content of free_hugepages is longer than or equal to 1024 
> bytes
> +  * the rest is irrelevant; we simply want to know if there are any
> +  * hugepages left
> +  */
> + if (len == 1024)
> + {
> + buff[1023] = 0;
> + }
> + else
> + 

Re: [HACKERS] "pg_ctl promote" exit status

[Bruce Momjian]

On Mon, Jan 28, 2013 at 09:46:32AM -0500, Peter Eisentraut wrote:
> On 1/26/13 4:44 PM, Aaron W. Swenson wrote:
> > You are right. Had I read a little further down, it seems that the
> > exit status should actually be 7.
> 
> 7 is OK for "not running", but what should we use when the server is not
> in standby mode?  Using the idempotent argument that we are discussing
> for the stop action, promoting a server that is not a standby should be
> a noop and exit successfully.  Not sure if that is what we want, though.

I looked at all the LSB return codes listed here and mapped them to
pg_ctl error situations:


https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html

Patch attached.  I did not touch the start/stop return codes.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
new file mode 100644
index 9045e00..7982340
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
*** get_pgpid(void)
*** 252,258 
  		{
  			write_stderr(_("%s: could not open PID file \"%s\": %s\n"),
  		 progname, pid_file, strerror(errno));
! 			exit(1);
  		}
  	}
  	if (fscanf(pidf, "%ld", &pid) != 1)
--- 252,258 
  		{
  			write_stderr(_("%s: could not open PID file \"%s\": %s\n"),
  		 progname, pid_file, strerror(errno));
! 			exit(6);
  		}
  	}
  	if (fscanf(pidf, "%ld", &pid) != 1)
*** get_pgpid(void)
*** 264,270 
  		else
  			write_stderr(_("%s: invalid data in PID file \"%s\"\n"),
  		 progname, pid_file);
! 		exit(1);
  	}
  	fclose(pidf);
  	return (pgpid_t) pid;
--- 264,270 
  		else
  			write_stderr(_("%s: invalid data in PID file \"%s\"\n"),
  		 progname, pid_file);
! 		exit(6);
  	}
  	fclose(pidf);
  	return (pgpid_t) pid;
*** read_post_opts(void)
*** 668,680 
  			if (optlines == NULL)
  			{
  write_stderr(_("%s: could not read file \"%s\"\n"), progname, postopts_file);
! exit(1);
  			}
  			else if (optlines[0] == NULL || optlines[1] != NULL)
  			{
  write_stderr(_("%s: option file \"%s\" must have exactly one line\n"),
  			 progname, postopts_file);
! exit(1);
  			}
  			else
  			{
--- 668,680 
  			if (optlines == NULL)
  			{
  write_stderr(_("%s: could not read file \"%s\"\n"), progname, postopts_file);
! exit(6);
  			}
  			else if (optlines[0] == NULL || optlines[1] != NULL)
  			{
  write_stderr(_("%s: option file \"%s\" must have exactly one line\n"),
  			 progname, postopts_file);
! exit(6);
  			}
  			else
  			{
*** find_other_exec_or_die(const char *argv0
*** 730,736 
  		   "but was not the same version as %s.\n"
  		   "Check your installation.\n"),
  		 target, full_path, progname);
! 		exit(1);
  	}
  
  	return found_path;
--- 730,736 
  		   "but was not the same version as %s.\n"
  		   "Check your installation.\n"),
  		 target, full_path, progname);
! 		exit(5);
  	}
  
  	return found_path;
*** do_start(void)
*** 813,819 
  	{
  		write_stderr(_("%s: could not start server: exit code was %d\n"),
  	 progname, exitcode);
! 		exit(1);
  	}
  
  	if (do_wait)
--- 813,819 
  	{
  		write_stderr(_("%s: could not start server: exit code was %d\n"),
  	 progname, exitcode);
! 		exit(7);
  	}
  
  	if (do_wait)
*** do_start(void)
*** 835,847 
  write_stderr(_("%s: could not start server\n"
  			   "Examine the log output.\n"),
  			 progname);
! exit(1);
  break;
  			case PQPING_NO_ATTEMPT:
  print_msg(_(" failed\n"));
  write_stderr(_("%s: could not wait for server because of misconfiguration\n"),
  			 progname);
! exit(1);
  		}
  	}
  	else
--- 835,847 
  write_stderr(_("%s: could not start server\n"
  			   "Examine the log output.\n"),
  			 progname);
! exit(7);
  break;
  			case PQPING_NO_ATTEMPT:
  print_msg(_(" failed\n"));
  write_stderr(_("%s: could not wait for server because of misconfiguration\n"),
  			 progname);
! exit(6);
  		}
  	}
  	else
*** do_status(void)
*** 1203,1212 
  	printf(_("%s: no server running\n"), progname);
  
  	/*
! 	 * The Linux Standard Base Core Specification 3.1 says this should return
! 	 * '3'
! 	 * http://refspecs.freestandards.org/LSB_3.1.1/LSB-Core-generic/LSB-Core-ge
! 	 * neric/iniscrptact.html
  	 */
  	exit(3);
  }
--- 1203,1210 
  	printf(_("%s: no server running\n"), progname);
  
  	/*
! 	 * The Linux Standard Base Core Specification 3.1 says this should return '3'
! 	 * https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html
  	 */
  	exit(3);
  }
*** pgwin32_CommandLine(bool registration)
*** 1253,1259 
  		if (ret != 0)
  		{
  			write_stderr(_("%s: could no

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

[Jeff Davis]

On Thu, 2013-06-27 at 15:59 +0200, Andres Freund wrote:
> Maybe the trick is to add a recovery.conf option to make postgres replay
> to the first restartpoint and then shutdown. At that point you can be
> sure there aren't any torn pages anymore (bugs aside).
> In fact that sounds like a rather useful pg_basebackup extension...

+1

Jeff Davis




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

[HACKERS] Outputting UTC offset with to_char()

[Bruce Momjian]

On Sun, Oct 21, 2012 at 05:40:40PM -0400, Andrew Dunstan wrote:
> 
> I'm not sure if this has come up before.
> 
> A client was just finding difficulties because to_char() doesn't
> support formatting the timezone part of a timestamptz numerically
> (i.e. as +-hhmm) instead of using a timezone name. Is there any
> reason for that? Would it be something worth having?

Great idea!  I have developed the attached patch to do this:

test=> SELECT to_char(current_timestamp, 'OF');
 to_char
-
 -04
(1 row)

test=> SELECT to_char(current_timestamp, 'TMOF');
 to_char
-
 -04
(1 row)

test=> SET timezone = 'Asia/Calcutta';
SET
test=> SELECT to_char(current_timestamp, 'OF');
 to_char
-
 +05:30
(1 row)

test=> SELECT to_char(current_timestamp, 'FMOF');
 to_char
-
 +5:30
(1 row)

I went with the optional colon and minutes because this is how we output
it:

test=> SELECT current_timestamp;
  now
---
 2013-06-28 22:02:24.773587-04
   ---
(1 row)

test=> set timezone = 'Asia/Calcutta';
SET
test=> SELECT current_timestamp;
   now
--
 2013-06-29 07:32:29.157565+05:30
   --
(1 row)

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
new file mode 100644
index 7c009d8..5765ddf
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT SUBSTRING('XY1234Z', 'Y*?([0-9]{1
*** 5645,5650 
--- 5645,5654 
  tz
  lower case time-zone name
 
+
+ OF
+ time-zone offset
+

   
  
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
new file mode 100644
index 7b85406..4c272ef
*** a/src/backend/utils/adt/formatting.c
--- b/src/backend/utils/adt/formatting.c
*** typedef enum
*** 600,605 
--- 600,606 
  	DCH_MS,
  	DCH_Month,
  	DCH_Mon,
+ 	DCH_OF,
  	DCH_P_M,
  	DCH_PM,
  	DCH_Q,
*** static const KeyWord DCH_keywords[] = {
*** 746,751 
--- 747,753 
  	{"MS", 2, DCH_MS, TRUE, FROM_CHAR_DATE_NONE},
  	{"Month", 5, DCH_Month, FALSE, FROM_CHAR_DATE_GREGORIAN},
  	{"Mon", 3, DCH_Mon, FALSE, FROM_CHAR_DATE_GREGORIAN},
+ 	{"OF", 2, DCH_OF, FALSE, FROM_CHAR_DATE_NONE},		/* O */
  	{"P.M.", 4, DCH_P_M, FALSE, FROM_CHAR_DATE_NONE},	/* P */
  	{"PM", 2, DCH_PM, FALSE, FROM_CHAR_DATE_NONE},
  	{"Q", 1, DCH_Q, TRUE, FROM_CHAR_DATE_NONE}, /* Q */
*** static const int DCH_index[KeyWord_INDEX
*** 874,880 
  	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
  	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
  	-1, -1, -1, -1, -1, DCH_A_D, DCH_B_C, DCH_CC, DCH_DAY, -1,
! 	DCH_FX, -1, DCH_HH24, DCH_IDDD, DCH_J, -1, -1, DCH_MI, -1, -1,
  	DCH_P_M, DCH_Q, DCH_RM, DCH_, DCH_TZ, DCH_US, -1, DCH_WW, -1, DCH_Y_YYY,
  	-1, -1, -1, -1, -1, -1, -1, DCH_a_d, DCH_b_c, DCH_cc,
  	DCH_day, -1, DCH_fx, -1, DCH_hh24, DCH_iddd, DCH_j, -1, -1, DCH_mi,
--- 876,882 
  	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
  	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
  	-1, -1, -1, -1, -1, DCH_A_D, DCH_B_C, DCH_CC, DCH_DAY, -1,
! 	DCH_FX, -1, DCH_HH24, DCH_IDDD, DCH_J, -1, -1, DCH_MI, -1, DCH_OF,
  	DCH_P_M, DCH_Q, DCH_RM, DCH_, DCH_TZ, DCH_US, -1, DCH_WW, -1, DCH_Y_YYY,
  	-1, -1, -1, -1, -1, -1, -1, DCH_a_d, DCH_b_c, DCH_cc,
  	DCH_day, -1, DCH_fx, -1, DCH_hh24, DCH_iddd, DCH_j, -1, -1, DCH_mi,
*** DCH_to_char(FormatNode *node, bool is_in
*** 2502,2507 
--- 2504,2519 
  	s += strlen(s);
  }
  break;
+ 			case DCH_OF:
+ INVALID_FOR_INTERVAL;
+ sprintf(s, "%+0*ld", S_FM(n->suffix) ? 0 : 3, tm->tm_gmtoff / 3600);
+ s += strlen(s);
+ if (tm->tm_gmtoff % 3600 != 0)
+ {
+ 	sprintf(s, ":%02ld", (tm->tm_gmtoff % 3600) / 60);
+ 	s += strlen(s);
+ }
+ break;
  			case DCH_A_D:
  			case DCH_B_C:
  INVALID_FOR_INTERVAL;
*** DCH_from_char(FormatNode *node, char *in
*** 2915,2923 
  break;
  			case DCH_tz:
  			case DCH_TZ:
  ereport(ERROR,
  		(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 		 errmsg("\"TZ\"/\"tz\" format patterns are not supported in to_date")));
  			case DCH_A_D:
  			case DCH_B_C:
  			case DCH_a_d:
--- 2927,2936 
  break;
  			case DCH_tz:
  			case DCH_TZ:
+ 			case DCH_OF:
  ereport(ERROR,
  		(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 		 errmsg("\"TZ\"/\"tz\"/\"OF\" format patterns are not supported in to_date")));
  			case DCH_A_D:

Re: [HACKERS] [GENERAL] pg_upgrade -u

[Tom Lane]

Peter Eisentraut  writes:
> On 6/28/13 6:06 PM, Bruce Momjian wrote:
>> On Wed, May 29, 2013 at 09:44:00AM -0400, Peter Eisentraut wrote:
>>> pg_upgrade should somehow be able to find out by itself what the
>>> superuser of the old cluster was.

>> Uh, any idea how to do that?

> select rolname from pg_authid where oid = 10;

But that only works if you've already been able to connect, no?

regards, tom lane


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

Re: [HACKERS] New regression test time

[Stephen Frost]

* Josh Berkus (j...@agliodbs.com) wrote:
> So that's an increase of about 10% in test runtime (or 2 seconds per run
> on my laptop), in order to greatly improve regression test coverage.
> I'd say that splitting the tests is not warranted, and that we should go
> ahead with these tests on their testing merits, not based on any extra
> check time they might add.

For my 2c, +1 on this in general, in spite of the concerns.  Covering
cases that we don't is valuable in general and if we get a bit more
coverage for a few more seconds, it's worth it.

Also, if someone wants to split the test up, then they need to provide a
patch which does so.  I'm not against that, but I do not feel this
addition should be held up waiting for someone to implement such a
seperation- if anything, having the two things done independently would
probably be cleaner anyway.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [GENERAL] pg_upgrade -u

[Bruce Momjian]

On Fri, Jun 28, 2013 at 09:15:31PM -0400, Peter Eisentraut wrote:
> On 6/28/13 6:06 PM, Bruce Momjian wrote:
> > On Wed, May 29, 2013 at 09:44:00AM -0400, Peter Eisentraut wrote:
> >> On 5/28/13 10:55 PM, Bruce Momjian wrote:
> >>> Wow, I never realized other tools used -U for user, instead of -u. 
> >>> Should I change pg_upgrade to use -U for 9.4?  I can keep supporting -u
> >>> as an undocumented option.
> >>
> >> It seems to me that that option shouldn't be necessary anyway.
> >> pg_upgrade should somehow be able to find out by itself what the
> >> superuser of the old cluster was.
> > 
> > Uh, any idea how to do that?
> 
> select rolname from pg_authid where oid = 10;

Uh, how do I know what username to connect as to issue that query?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
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 more regression tests for CREATE OPERATOR

[Robins Tharakan]

On 27 June 2013 09:00, Robert Haas  wrote:

> On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz  wrote:
> > OK, so I think this patch can be committed, I will change the status.
>
> We have a convention that roles created by the regression tests needs
> to have "regress" or something of the sort in the name, and that they
> need to be dropped by the regression tests.  The idea is that if
> someone runs "make installcheck" against an installed server, it
> should pass - even if you run it twice in succession.  And also, it
> shouldn't be likely to try to create (and then drop!) a role name that
> already exists.
>
> Setting this to "Waiting on Author".
>
> Hi Robert,

Attached is an updated patch that prepends 'regress' before role names.

As for dropping ROLEs is concerned, all the roles created in the previous
patch were within transactions. So didn't have to explicitly drop any ROLEs
at the end of the script.
--
Robins Tharakan


regress_createoperator_v2.patch
Description: Binary data

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

Re: [HACKERS] [GENERAL] pg_upgrade -u

[Peter Eisentraut]

On 6/28/13 6:06 PM, Bruce Momjian wrote:
> On Wed, May 29, 2013 at 09:44:00AM -0400, Peter Eisentraut wrote:
>> On 5/28/13 10:55 PM, Bruce Momjian wrote:
>>> Wow, I never realized other tools used -U for user, instead of -u. 
>>> Should I change pg_upgrade to use -U for 9.4?  I can keep supporting -u
>>> as an undocumented option.
>>
>> It seems to me that that option shouldn't be necessary anyway.
>> pg_upgrade should somehow be able to find out by itself what the
>> superuser of the old cluster was.
> 
> Uh, any idea how to do that?

select rolname from pg_authid where oid = 10;



-- 
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] COPY and Volatile default expressions

[Simon Riggs]

On 24 June 2013 10:21, Kohei KaiGai  wrote:

> Hi Simon,
>
> I checked this patch. One thing I could comment on is, do you think it is
> a good
> idea to have oid of exception function list on
> contain_volatile_functions_walker()?
>
> The walker function is static thus here is no impact for other caller, and
> its
> "context" argument is unused.
> My proposition is to enhance 2nd argument of
> contain_volatile_functions_walker()
> to deliver list of exceptional functions, then
> contain_volatile_functions_not_nextval()
> calls contain_volatile_functions_walker() with
> list_make1_oid(F_NEXTVAL_OID) to
> handle nextval() as exception.
> Otherwise, all we need to do is put NIL as 2nd argument.
>
> It kills code duplication and reduces future maintenance burden.
> How about your opinion?
>

That approach is more flexible than the one in the patch, I agree.

Ultimately, I see this as a choice between a special purpose piece of code
(as originally supplied) and a much more generic facility for labelling
functions as to whether they contain SQL or not, per the SQL standard as
Jaime suggests. There's not much mileage in something in between.

So I'm mid way through updating the patch to implement the generic facility.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior

['Bruce Momjian']

On Sat, Jan 26, 2013 at 11:31:49AM +0530, Amit Kapila wrote:
> On Friday, January 25, 2013 8:36 PM Bruce Momjian wrote:
> > On Fri, Sep 14, 2012 at 02:04:51PM +, Amit kapila wrote:
> > > On Thu, 6 Sep 2012 14:50:05 -0400 Robert Hass wrote:
> > >
> > > On Tue, Aug 28, 2012 at 6:40 AM, Amit Kapila
> >  > > com> wrote:
> > > >>   AFAICT during Update also, it doesn't contain useful. The only
> > chance it
> > > >> would have contain something useful is when it goes for
> > EvalPlanQual and
> > > >> then again comes to check for constraints. However these
> > attributes get
> > > >> filled in heap_update much later.
> > > >
> > > >> So now should the fix be that it returns an error for system
> > column
> > > >> reference except for OID case?
> > >
> > > > +1.
> > >
> > >
> > >
> > > 1. I think in this scenario the error for system column except for
> > tableOID
> > > should be thrown at Create/Alter time.
> > >
> > > 2. After setting OID in ExecInsert/ExecUpdate may be setting of same
> > inside
> > > heap functions can be removed.
> > >
> > >But for now I have kept them as it is.
> > >
> > >
> > >
> > > Please find the Patch for bug-fix.
> > >
> > > If this is okay, I shall send you the test cases for same.
> > 
> > Did we ever make any progress on this?
> 
> I have done the initial analysis and prepared a patch, don't know if
> anything more I can do until
> someone can give any suggestions to further proceed on this bug. 

So, I guess we never figured this out.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
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] [NOVICE] index refuses to build

[Bruce Momjian]

On Sun, Aug 26, 2012 at 09:47:01AM -0400, Bruce Momjian wrote:
> On Thu, Dec 29, 2011 at 10:40:19PM -0500, Tom Lane wrote:
> > Merlin Moncure  writes:
> > > On Thu, Dec 29, 2011 at 5:10 PM, Jean-Yves F. Barbier <12u...@gmail.com> 
> > > wrote:
> > >> CREATE INDEX tst1m_name_lu_ix ON tst1m(unaccent(name));
> > >> ERROR:  functions in index expression must be marked IMMUTABLE
> > 
> > > your problem is the unaccent function.  it's defined stable because
> > > the rules function it depends on can change after the index is built
> > > -- that would effectively introduce index corruption.  it's possible
> > > to bypass that restriction, but are you sure that's what you want to
> > > do?
> > 
> > Hmm ... it's clear why unaccent(text) is only stable, because it depends
> > on the current search_path to find the "unaccent" dictionary.  But I
> > wonder whether it was an oversight that unaccent(regdictionary, text)
> > is stable and not immutable.  We don't normally mark functions as stable
> > just because you could in principle change their behavior by altering
> > some outside-the-database configuration files.
> 
> Should we change the function signature for unaccent(regdictionary,
> text)?

Did we decide not to do this?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
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] [GENERAL] pg_upgrade -u

[Bruce Momjian]

On Thu, May 30, 2013 at 08:42:21AM -0400, Ray Stell wrote:
>
> On May 29, 2013, at 11:07 AM, Bruce Momjian wrote:
>
> > On Wed, May 29, 2013 at 08:59:42AM -0400, Ray Stell wrote:
> >>> [ moved to hacker ] The question is whether hard-wiring these
> >>> helps more than it hurts, and which ones should be hard-wired.
>
> I seems to me that superuser is exactly that special case and that if
> an alternate superuser is hardwired in the src cluster then -u/-U and
> that specific value will be required on both sides of pg_upgrade, no
> variability is needed and perhaps not possible.  You're point is well
> taken for port.

You have convinced me.  The attached, applied patch for PG 9.4 passes
the command-line-supplied username into the created analyze script.

I have also attached a sample output analyze script.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 1f67e60..0376fcb
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** void
*** 459,464 
--- 459,471 
  create_script_for_cluster_analyze(char **analyze_script_file_name)
  {
  	FILE	   *script = NULL;
+ 	char	   *user_specification = "";
+ 
+ 	if (os_info.user_specified)
+ 	{
+ 		user_specification = pg_malloc(strlen(os_info.user) + 7);
+ 		sprintf(user_specification, "-U \"%s\" ", os_info.user);
+ 	}
  
  	*analyze_script_file_name = pg_malloc(MAXPGPATH);
  
*** create_script_for_cluster_analyze(char *
*** 501,507 
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %sthis script and run:%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "echo %s\"%s/vacuumdb\" --all %s%s\n", ECHO_QUOTE, new_cluster.bindir,
  	/* Did we copy the free space files? */
  			(GET_MAJOR_VERSION(old_cluster.major_version) >= 804) ?
  			"--analyze-only" : "--analyze", ECHO_QUOTE);
--- 508,515 
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %sthis script and run:%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "echo %s\"%s/vacuumdb\" %s--all %s%s\n", ECHO_QUOTE,
! 			new_cluster.bindir, user_specification,
  	/* Did we copy the free space files? */
  			(GET_MAJOR_VERSION(old_cluster.major_version) >= 804) ?
  			"--analyze-only" : "--analyze", ECHO_QUOTE);
*** create_script_for_cluster_analyze(char *
*** 522,528 
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %s--%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "\"%s/vacuumdb\" --all --analyze-only\n", new_cluster.bindir);
  	fprintf(script, "echo%s\n", ECHO_BLANK);
  	fprintf(script, "echo %sThe server is now available with minimal optimizer statistics.%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
--- 530,537 
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %s--%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "\"%s/vacuumdb\" %s--all --analyze-only\n",
! 			new_cluster.bindir, user_specification);
  	fprintf(script, "echo%s\n", ECHO_BLANK);
  	fprintf(script, "echo %sThe server is now available with minimal optimizer statistics.%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
*** create_script_for_cluster_analyze(char *
*** 543,549 
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %s---%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "\"%s/vacuumdb\" --all --analyze-only\n", new_cluster.bindir);
  	fprintf(script, "echo%s\n\n", ECHO_BLANK);
  
  #ifndef WIN32
--- 552,559 
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %s---%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "\"%s/vacuumdb\" %s--all --analyze-only\n",
! 			new_cluster.bindir, user_specification);
  	fprintf(script, "echo%s\n\n", ECHO_BLANK);
  
  #ifndef WIN32
*** create_script_for_cluster_analyze(char *
*** 556,562 
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %s-%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "\"%s/vacuumdb\" --all %s\n", new_cluster.bindir,
  	/* Did we copy the free space files? */
  			(GET_MAJOR_VERSION(old_cluster.major_version) >= 804) ?
  			"--analyze-only" : "--analyze");
--- 566,573 
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %s-%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "\"%s/vacuumdb\" %s--all %s\n", new_cluster.bindir,
! 			user_specification,
  	/* Did we copy the free space files? */
  			(GET_MAJOR_VERSION(old_cluster.major_version) >= 804) ?
  			"--analyze-only" : "--analyze");
*** create_script_for_cluster_analyze(char *
*** 573,578 
--- 584,592 
  			   *analyze_script_fi

[HACKERS] psql and pset without any arguments

[Gilles Darold]

Hi,

I was looking at psql 8.3 documention about \pset options and saw that
there was the following note :

"Note: It is an error to call \pset without any arguments. In the
future this case might show the current status of all printing options."

I looked backward and forward to find that this note is present in all
versions since 7.1 up to 9.3, maybe it is time to add this little feature.

I've attached a patch to add the usage of the \pset command without any
arguments to displays current status of all printing options instead of
the error message. Here is a sample output:

(postgres@[local]:5494) [postgres] > \pset
Output format is aligned.
Border style is 2.
Expanded display is used automatically.
Null display is "NULL".
Field separator is "|".
Tuples only is off.
Title is unset.
Table attributes unset.
Line style is unicode.
Pager is used for long output.
Record separator is .
(postgres@[local]:5494) [postgres] >

To avoid redundant code I've added a new method printPsetInfo() so that
do_pset() and exec_command() will used the same output message, they are
all in src/bin/psql/command.c. For example:

(postgres@[local]:5494) [postgres] > \pset null 'NULL'
Null display is "NULL".
(postgres@[local]:5494) [postgres] >

The patch print all variables information from struct printTableOpt when
\pset is given without any arguments and also update documentation.

Let me know if there's any additional work to do on this basic patch or
something that I've omitted.

Best regards,

-- 
Gilles Darold
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 574db5c..a3bf555 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2272,13 +2272,9 @@ lo_import 152801
 
 
 
-
-
-It is an error to call \pset without any
-arguments. In the future this case might show the current status
-of all printing options.
+\pset without any arguments displays current status
+ of all printing options.
 
-
 
 
   
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 351e684..daf7ac7 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -68,6 +68,7 @@ static int	strip_lineno_from_funcdesc(char *func);
 static void minimal_error_message(PGresult *res);
 
 static void printSSLInfo(void);
+static bool printPsetInfo(const char *param, struct printQueryOpt *popt);
 
 #ifdef WIN32
 static void checkWin32Codepage(void);
@@ -1045,8 +1046,16 @@ exec_command(const char *cmd,
 
 		if (!opt0)
 		{
-			psql_error("\\%s: missing required argument\n", cmd);
-			success = false;
+			size_t i;
+			/* list all variables */
+			static const char *const my_list[] = {"format", "border",
+"expanded", "null", "fieldsep", "tuples_only", "title",
+"tableattr", "linestyle", "pager", "recordsep", NULL};
+			for (i = 0; my_list[i] != NULL; i++) {
+printPsetInfo(my_list[i], &pset.popt);
+			}
+
+			success = true;
 		}
 		else
 			success = do_pset(opt0, opt1, &pset.popt, pset.quiet);
@@ -2275,8 +2284,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			return false;
 		}
 
-		if (!quiet)
-			printf(_("Output format is %s.\n"), _align2string(popt->topt.format));
 	}
 
 	/* set table line style */
@@ -2295,10 +2302,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			psql_error("\\pset: allowed line styles are ascii, old-ascii, unicode\n");
 			return false;
 		}
-
-		if (!quiet)
-			printf(_("Line style is %s.\n"),
-   get_line_style(&popt->topt)->name);
 	}
 
 	/* set border style/width */
@@ -2306,9 +2309,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 	{
 		if (value)
 			popt->topt.border = atoi(value);
-
-		if (!quiet)
-			printf(_("Border style is %d.\n"), popt->topt.border);
 	}
 
 	/* set expanded/vertical mode */
@@ -2320,15 +2320,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			popt->topt.expanded = ParseVariableBool(value);
 		else
 			popt->topt.expanded = !popt->topt.expanded;
-		if (!quiet)
-		{
-			if (popt->topt.expanded == 1)
-printf(_("Expanded display is on.\n"));
-			else if (popt->topt.expanded == 2)
-printf(_("Expanded display is used automatically.\n"));
-			else
-printf(_("Expanded display is off.\n"));
-		}
 	}
 
 	/* locale-aware numeric output */
@@ -2338,13 +2329,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			popt->topt.numericLocale = ParseVariableBool(value);
 		else
 			popt->topt.numericLocale = !popt->topt.numericLocale;
-		if (!quiet)
-		{
-			if (popt->topt.numericLocale)
-puts(_("Showing locale-adjusted numeric output."));
-			else
-puts(_("Locale-adjusted numer

Re: backend hangs at immediate shutdown (Re: [HACKERS] Back-branch update releases coming in a couple weeks)

[Robert Haas]

On Fri, Jun 28, 2013 at 6:00 PM, Alvaro Herrera
 wrote:
> MauMau escribió:
>
> Hi,
>
>> I did this.  Please find attached the revised patch.  I modified
>> HandleChildCrash().  I tested the immediate shutdown, and the child
>> cleanup succeeded.
>
> Thanks, committed.
>
> There are two matters pending here:
>
> 1. do we want postmaster to exit immediately after sending the SIGKILL,
> or hang around until all children are gone?
>
> 2. how far do we want to backpatch this, if at all?

Considering that neither Tom nor I was convinced that this was a good
idea AT ALL, I'm surprised you committed it in the first place.  I'd
be more inclined to revert it than back-patch it, but at least if we
only change it in HEAD we have some chance of finding out whether or
not it is in fact a bad idea before it's too late to change our mind.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] [GENERAL] pg_upgrade -u

[Bruce Momjian]

On Wed, May 29, 2013 at 09:44:00AM -0400, Peter Eisentraut wrote:
> On 5/28/13 10:55 PM, Bruce Momjian wrote:
> > Wow, I never realized other tools used -U for user, instead of -u. 
> > Should I change pg_upgrade to use -U for 9.4?  I can keep supporting -u
> > as an undocumented option.
> 
> It seems to me that that option shouldn't be necessary anyway.
> pg_upgrade should somehow be able to find out by itself what the
> superuser of the old cluster was.

Uh, any idea how to do that?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
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] New regression test time

[Andres Freund]

On 2013-06-28 14:46:10 -0700, Josh Berkus wrote:
> 
> > How did you evaluate that coverage increased "greatly"? I am not
> > generally against these tests but I'd be surprised if the overall test
> > coverage improved noticeably by this. Which makes 10% runtime overhead
> > pretty hefty if the goal is to actually achieve a high coverage.
> 
> I was relying on Robins' numbers of coverage:

Those improvements rather likely end up being an improvement a good bit
less than one percent for the whole binary.

> If we someday add so many tests that "make check" takes over a minute on
> a modern laptop, then maybe it'll be worth talking about splitting the
> test suite into "regular" and "extended".  However, it would require 15
> more patch sets the size of Robins' to get there, so I don't see that
> it's worth the trouble any time soon.

Was it actually an assert enabled build that you tested?

We currently can run make check with stuff like CLOBBER_CACHE_ALWAYS
which finds bugs pretty regularly. If we achieve a high coverage we
quite possibly can't anymore, at least not regularly.
So I actually think having two modes makes sense. Then we could also
link stuff like isolationtester automatically into the longer running
mode...

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] [GENERAL] pg_upgrade -u

[Bruce Momjian]

On Tue, May 28, 2013 at 09:08:03PM -0700, Joshua D. Drake wrote:
> 
> On 05/28/2013 07:55 PM, Bruce Momjian wrote:
> 
> >>Perhaps just documenting the behavior is all that is needed, but -U is
> >>everywhere and I think that's a good thing.
> >
> >[ moved to hacker ]
> >
> >Wow, I never realized other tools used -U for user, instead of -u.
> >Should I change pg_upgrade to use -U for 9.4?  I can keep supporting -u
> >as an undocumented option.
> 
> Yes, -U makes the most sense as that is what is used with the other
> tools. I think you should just drop -u, this isn't something people
> are doing everyday (like psql). The backward compatibility argument
> is pretty slim.

Done for 9.4.  I also simplified some of the option values in --help and
the docs.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


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

Re: backend hangs at immediate shutdown (Re: [HACKERS] Back-branch update releases coming in a couple weeks)

[Alvaro Herrera]

MauMau escribió:

Hi,

> I did this.  Please find attached the revised patch.  I modified
> HandleChildCrash().  I tested the immediate shutdown, and the child
> cleanup succeeded.

Thanks, committed.

There are two matters pending here:

1. do we want postmaster to exit immediately after sending the SIGKILL,
or hang around until all children are gone?

2. how far do we want to backpatch this, if at all?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] New regression test time

[Josh Berkus]

> How did you evaluate that coverage increased "greatly"? I am not
> generally against these tests but I'd be surprised if the overall test
> coverage improved noticeably by this. Which makes 10% runtime overhead
> pretty hefty if the goal is to actually achieve a high coverage.

I was relying on Robins' numbers of coverage:

Patch to add more regression tests to ASYNC (/src/backend/commands/async).
Take code-coverage from 39% to 75%.

Please find attached a patch to take code-coverage of ALTER OPERATOR
FAMILY.. ADD / DROP (src/backend/commands/opclasscmds.c) from 50% to 87%.

Please find attached a patch to take code-coverage of LOCK TABLE (
src/backend/commands/lockcmds.c) from 57% to 84%.

... etc.

If we someday add so many tests that "make check" takes over a minute on
a modern laptop, then maybe it'll be worth talking about splitting the
test suite into "regular" and "extended".  However, it would require 15
more patch sets the size of Robins' to get there, so I don't see that
it's worth the trouble any time soon.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] New regression test time

[Andres Freund]

On 2013-06-28 14:01:23 -0700, Josh Berkus wrote:
> Per discussion on these tests, I ran "make check" against 9.4 head,
> applied all of the regression tests other than DISCARD.
> 
> Time for 3 "make check" runs without new tests: 65.9s
> 
> Time for 3 "make check runs with new tests: 71.7s
> 
> So that's an increase of about 10% in test runtime (or 2 seconds per run
> on my laptop), in order to greatly improve regression test coverage.

How did you evaluate that coverage increased "greatly"? I am not
generally against these tests but I'd be surprised if the overall test
coverage improved noticeably by this. Which makes 10% runtime overhead
pretty hefty if the goal is to actually achieve a high coverage.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
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 some regression tests for SEQUENCE

[Josh Berkus]

On 06/28/2013 02:15 PM, Robins Tharakan wrote:
> Seems like thats because of a recent (15th May 2013) patch
> (b14206862278347a379f2bb72d92d16fb9dcea45) that changed the error message
> that is printed. Its just one line of difference actually.
> 
> Let me know if this is otherwise good to go.
> I'll checkout the latest revision and submit this patch again.
> 

I was only checking test timing, per my earlier email.  I haven't looked
at the tests themselves at all.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] Add some regression tests for SEQUENCE

[Josh Berkus]

On 06/28/2013 02:15 PM, Robins Tharakan wrote:
> Seems like thats because of a recent (15th May 2013) patch
> (b14206862278347a379f2bb72d92d16fb9dcea45) that changed the error message
> that is printed. Its just one line of difference actually.
> 
> Let me know if this is otherwise good to go.
> I'll checkout the latest revision and submit this patch again.
> 

I was only checking test timing, per my earlier email.  I haven't looked
at the tests themselves at all.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] Add some regression tests for SEQUENCE

[Robins Tharakan]

Seems like thats because of a recent (15th May 2013) patch
(b14206862278347a379f2bb72d92d16fb9dcea45) that changed the error message
that is printed. Its just one line of difference actually.

Let me know if this is otherwise good to go.
I'll checkout the latest revision and submit this patch again.

--
Robins Tharakan


On 28 June 2013 15:53, Josh Berkus  wrote:

> On 05/07/2013 03:40 PM, Robins Tharakan wrote:
> > Hi,
> >
> > Have provided an updated patch as per Fabien's recent response on
> > Commitfest site.
> > Any and all feedback is appreciated.
>
> The updated patch is giving a FAILURE for me:
>
> parallel group (19 tests):  limit temp plancache conversion rowtypes
> prepare without_oid copy2 xml returning rangefuncs polymorphism with
> domain truncate largeobject sequence alter_table plpgsql
>  plancache... ok
>  limit... ok
>  plpgsql  ... ok
>  copy2... ok
>  temp ... ok
>  domain   ... ok
>  rangefuncs   ... ok
>  prepare  ... ok
>  without_oid  ... ok
>  conversion   ... ok
>  truncate ... ok
>  alter_table  ... ok
>  sequence ... FAILED
>
> Thoughts?
>
> --
> Josh Berkus
> PostgreSQL Experts Inc.
> http://pgexperts.com
>

Re: [HACKERS] Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Nicholas White]

I've fixed the problems you mentioned (see attached) - sorry, I was a bit
careless with the docs.


> +   null_values = (Bitmapset *) WinGetPartitionLocalMemory(
> +   winobj,
> +   BITMAPSET_SIZE(words_needed));
> +   Assert(null_values);
>
> This certainly seems ugly - isn't there a way to accomplish this
> without having to violate the Bitmapset abstraction?

Indeed, it's ugly. I've revised it slightly to:

> null_values = (Bitmapset *) WinGetPartitionLocalMemory(
>winobj,
>BITMAPSET_SIZE(words_needed));
> null_values->nwords = (int) words_needed;

...which gives a proper bitmap. It's hard to break this into a factory
method like bms_make_singleton as I'm getting the (zero'ed) partition local
memory from one call, then forcing a correct bitmap's structure on it.
Maybe bitmapset.h needs an bms_initialise(void *, int num_words) factory
method? You'd still have to use the BITMAPSET_SIZE macro to get the correct
amount of memory for the void*. Maybe the factory method could take a
function pointer that would allocate memory of the given size (e.g.
Bitmapset* initialize(void* (allocator)(Size_t), int num_words) ) - this
means I could still use the partition's local memory.

I don't think the solution would be tidier if I re-instated the
leadlag_context struct with a single Bitmapset member. Other Bitmapset
usage seems to just call bms_make_singleton then bms_add_member over and
over again - which afaict will call palloc every BITS_PER_BITMAPWORD calls,
which is not really what I want.

Thanks -

Nick


lead-lag-ignore-nulls.patch
Description: Binary data

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

[HACKERS] New regression test time

[Josh Berkus]

Hackers,

Per discussion on these tests, I ran "make check" against 9.4 head,
applied all of the regression tests other than DISCARD.

Time for 3 "make check" runs without new tests: 65.9s

Time for 3 "make check runs with new tests: 71.7s

So that's an increase of about 10% in test runtime (or 2 seconds per run
on my laptop), in order to greatly improve regression test coverage.
I'd say that splitting the tests is not warranted, and that we should go
ahead with these tests on their testing merits, not based on any extra
check time they might add.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] Add more regression tests for dbcommands

[Robins Tharakan]

Hi Andres,

Just an aside, your point about CONNECTION LIMIT was something that just
didn't come to my mind and is probably a good way to test ALTER DATABASE
with CONNECTION LIMIT.

Its just that that actually wasn't what I was testing there. That
'CONNECTION LIMIT' test was coupled with CREATE DATABASE because I wanted
to test that 'branch' in the CREATE DATABASE function just to ensure that
there was a regression test that tests CONNECTION LIMIT specifically with
CREATE DATABASE. That's all. A check to confirm whether connection limit
restrictions actually got enforced was something I missed, but well, its
out of the window for now anyway.

--
Robins Tharakan


On 26 June 2013 06:34, Andres Freund  wrote:

> Hi,
>
> I am generally a bit unsure whether the regression tests you propose
> aren't a bit too verbose. Does any of the committers have an opinion
> about this?
>
> My feeling is that they are ok if they aren't slowing down things much.
>
> On 2013-06-26 01:55:53 -0500, Robins Tharakan wrote:
> > The CREATE DATABASE test itself was checking whether the 'CONNECTION
> LIMIT'
> > was working. Removed that as well.
>
> You should be able to test that by setting the connection limit to 1 and
> then try to connect using \c. The old connection is only dropped after
> the new one has been successfully performed.
>
> Greetings,
>
> Andres Freund
>
> --
>  Andres Freund http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Re: [HACKERS] Add some regression tests for SEQUENCE

[Josh Berkus]

On 05/07/2013 03:40 PM, Robins Tharakan wrote:
> Hi,
> 
> Have provided an updated patch as per Fabien's recent response on
> Commitfest site.
> Any and all feedback is appreciated.

The updated patch is giving a FAILURE for me:

parallel group (19 tests):  limit temp plancache conversion rowtypes
prepare without_oid copy2 xml returning rangefuncs polymorphism with
domain truncate largeobject sequence alter_table plpgsql
 plancache... ok
 limit... ok
 plpgsql  ... ok
 copy2... ok
 temp ... ok
 domain   ... ok
 rangefuncs   ... ok
 prepare  ... ok
 without_oid  ... ok
 conversion   ... ok
 truncate ... ok
 alter_table  ... ok
 sequence ... FAILED

Thoughts?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] PostgreSQL 9.3 latest dev snapshot

[Stefan Kaltenbrunner]

On 06/28/2013 06:51 PM, Alvaro Herrera wrote:
> Magnus Hagander escribió:
> 
>> However, if oyu're looking for a snapshot, please use the one on the
>> ftpsite. Generating those snapshots on the git server is slow and
>> expensive...
> 
> Maybe we should redirect those gitweb snapshot URLs to the FTP site?

maybe - but I can actually see the (rare) usecase of being able to
create a snapshot on a per-commit base, so redirecting to something that
is more of a "basic verified snapshot tarball once a day" seems wrong to
me, despite the fact that I think that using those is a better idea in
general.


Stefan


-- 
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] Fix conversion for Decimal arguments in plpython functions

[Claudio Freire]

On Fri, Jun 28, 2013 at 5:14 PM, Steve Singer  wrote:
> On 06/27/2013 05:04 AM, Szymon Guz wrote:
>>
>> On 27 June 2013 05:21, Steve Singer > > wrote:
>>
>> On 06/26/2013 04:47 PM, Szymon Guz wrote:
>>
>>
>>
>>
>>
>>
>> Hi Steve,
>> thanks for the changes.
>>
>> You're idea about common code for decimal and cdecimal is good, however
>> not good enough. I like the idea of common code for decimal and cdecimal.
>> But we need class name, not the value.
>>
>> I've changed the code from str(x) to x.__class__.__name__ so the function
>> prints class name (which is Decimal for both packages), not the value. We
>> need to have the class name check. The value is returned by the function and
>> is a couple of lines lower in the file.
>>
>> patch is attached.
>>
>
> I think the value is more important than the name, I want to the tests to
> make sure that the conversion is actually converting properly.  With your
> method of getting the class name without the module we can have both.
>
> The attached patch should print the value and the class name but not the
> module name.


Why not forego checking of the type, and instead check the interface?

plpy.info(x.as_tuple())

Should do.

>>> d  = decimal.Decimal((0,(3,1,4),-2))
>>> d.as_tuple()
DecimalTuple(sign=0, digits=(3, 1, 4), exponent=-2)
>>> d.as_tuple() == (0,(3,1,4),-2)
True
>>> d = decimal.Decimal("3.14")
>>> d.as_tuple()
DecimalTuple(sign=0, digits=(3, 1, 4), exponent=-2)
>>> d.as_tuple() == (0,(3,1,4),-2)
True
>>>


-- 
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] Fix conversion for Decimal arguments in plpython functions

[Szymon Guz]

On 28 June 2013 22:14, Steve Singer  wrote:

>
> I think the value is more important than the name, I want to the tests to
> make sure that the conversion is actually converting properly.  With your
> method of getting the class name without the module we can have both.
>
> The attached patch should print the value and the class name but not the
> module name.
>
> Steve
>

Hi Steve,
I agree, we can check both. This is quite a nice patch now, I've reviewed
it, all tests pass, works as expected. I think it is ready for committing.

thanks,
Szymon

Re: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]

[Dean Rasheed]

On 21 June 2013 06:16, David Fetter  wrote:
> Please find attached a patch which allows subqueries in the FILTER
> clause and adds regression testing for same.
>

This needs re-basing/merging following Robert's recent commit to make
OVER unreserved.

Regards,
Dean


-- 
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] Fix conversion for Decimal arguments in plpython functions

[Steve Singer]

On 06/27/2013 05:04 AM, Szymon Guz wrote:
On 27 June 2013 05:21, Steve Singer > wrote:


On 06/26/2013 04:47 PM, Szymon Guz wrote:






Hi Steve,
thanks for the changes.

You're idea about common code for decimal and cdecimal is good, 
however not good enough. I like the idea of common code for decimal 
and cdecimal. But we need class name, not the value.


I've changed the code from str(x) to x.__class__.__name__ so the 
function prints class name (which is Decimal for both packages), not 
the value. We need to have the class name check. The value is returned 
by the function and is a couple of lines lower in the file.


patch is attached.



I think the value is more important than the name, I want to the tests 
to make sure that the conversion is actually converting properly.  With 
your method of getting the class name without the module we can have both.


The attached patch should print the value and the class name but not the 
module name.


Steve



thanks,
Szymon








diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index aaf758d..da27874
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 308,321 

   
  
   

!PostgreSQL real, double,
!and numeric are converted to
!Python float.  Note that for
!the numeric this loses information and can lead to
!incorrect results.  This might be fixed in a future
!release.

   
  
--- 308,326 

   
  
+ 	 
+   
+PostgreSQL real and double are converted to
+Python float.
+   
+  
+ 
   

!PostgreSQL numeric is converted to
!Python Decimal. This type is imported from 
! 	   cdecimal package if it is available. If cdecimal
! 	   cannot be used, then decimal.Decimal will be used.

   
  
diff --git a/src/pl/plpython/expected/plpython_types.out b/src/pl/plpython/expected/plpython_types.out
new file mode 100644
index 4641345..e602336
*** a/src/pl/plpython/expected/plpython_types.out
--- b/src/pl/plpython/expected/plpython_types.out
*** CONTEXT:  PL/Python function "test_type_
*** 213,248 
  (1 row)
  
  CREATE FUNCTION test_type_conversion_numeric(x numeric) RETURNS numeric AS $$
! plpy.info(x, type(x))
  return x
  $$ LANGUAGE plpythonu;
! /* The current implementation converts numeric to float. */
  SELECT * FROM test_type_conversion_numeric(100);
! INFO:  (100.0, )
  CONTEXT:  PL/Python function "test_type_conversion_numeric"
   test_type_conversion_numeric 
  --
! 100.0
  (1 row)
  
  SELECT * FROM test_type_conversion_numeric(-100);
! INFO:  (-100.0, )
  CONTEXT:  PL/Python function "test_type_conversion_numeric"
   test_type_conversion_numeric 
  --
!-100.0
  (1 row)
  
  SELECT * FROM test_type_conversion_numeric(50.5);
! INFO:  (50.5, )
  CONTEXT:  PL/Python function "test_type_conversion_numeric"
   test_type_conversion_numeric 
  --
   50.5
  (1 row)
  
  SELECT * FROM test_type_conversion_numeric(null);
! INFO:  (None, )
  CONTEXT:  PL/Python function "test_type_conversion_numeric"
   test_type_conversion_numeric 
  --
--- 213,264 
  (1 row)
  
  CREATE FUNCTION test_type_conversion_numeric(x numeric) RETURNS numeric AS $$
! plpy.info(x,x.__class__.__name__)
  return x
  $$ LANGUAGE plpythonu;
! /* The current implementation converts numeric to Decimal. */
  SELECT * FROM test_type_conversion_numeric(100);
! INFO:  (Decimal('100'), 'Decimal')
  CONTEXT:  PL/Python function "test_type_conversion_numeric"
   test_type_conversion_numeric 
  --
!   100
  (1 row)
  
  SELECT * FROM test_type_conversion_numeric(-100);
! INFO:  (Decimal('-100'), 'Decimal')
  CONTEXT:  PL/Python function "test_type_conversion_numeric"
   test_type_conversion_numeric 
  --
!  -100
  (1 row)
  
  SELECT * FROM test_type_conversion_numeric(50.5);
! INFO:  (Decimal('50.5'), 'Decimal')
  CONTEXT:  PL/Python function "test_type_conversion_numeric"
   test_type_conversion_numeric 
  --
   50.5
  (1 row)
  
+ SELECT * FROM test_type_conversion_numeric(1234567890.0987654321);
+ INFO:  (Decimal('1234567890.0987654321'), 'Decimal')
+ CONTEXT:  PL/Python function "test_type_conversion_numeric"
+  test_type_conversion_numeric 
+ --
+ 1234567890.0987654321
+ (1 row)
+ 
+ SELECT * FROM test_type_conversion_numeric(-1234567890.0987654321);
+ INFO:  (Decimal('-1234567890.0987654321'), 'Decimal')
+ CONTEXT:  PL/Python function "test_type_conversion_numeric"
+  test_type_conversio

Re: [HACKERS] Add more regression tests for ASYNC

[Josh Berkus]

On 06/28/2013 12:42 PM, Josh Berkus wrote:
> On 05/13/2013 05:37 PM, Robins Tharakan wrote:
>> Hi,
>>
>> Patch to add more regression tests to ASYNC (/src/backend/commands/async).
>> Take code-coverage from 39% to 75%.
> 
> This isn't applying for me anymore due to changes in the
> parallel_schedule file.  Where did you mean this to execute in the
> sequence of tests?

Never mind, dirty checkout on my part.  Back to testing.


-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] changeset generation v5-01 - Patches & git tree

[Andres Freund]

On 2013-06-28 12:26:52 -0400, Tom Lane wrote:
> Robert Haas  writes:
> > On the other hand, I can't entirely shake the feeling that adding the
> > information into WAL would be more reliable.
> 
> That feeling has been nagging at me too.  I can't demonstrate that
> there's a problem when an ALTER TABLE is in process of rewriting a table
> into a new relfilenode number, but I don't have a warm fuzzy feeling
> about the reliability of reverse lookups for this.

I am pretty sure the mapping thing works, but it indeed requires some
complexity. And it's harder to debug because when you want to understand
what's going on the relfilenodes involved aren't in the catalog anymore.

> At the very least
> it's going to require some hard-to-verify restriction about how we
> can't start doing changeset reconstruction in the middle of a
> transaction that's doing DDL.

Currently changeset extraction needs to wait (and does so) till it found
a point where it has seen the start of all in-progress transactions. All
transaction that *commit* after the last partiall observed in-progress
transaction finished can be decoded.
To make that point visible for external tools to synchronize -
e.g. pg_dump - it exports the snapshot of exactly the moment when that
last in-progress transaction committed.

So, from what I gather there's a slight leaning towards *not* storing
the relation's oid in the WAL. Which means the concerns about the
uniqueness issues with the syscaches need to be addressed. So far I know
of three solutions:
1) develop a custom caching/mapping module
2) Make sure InvalidOid's (the only possible duplicate) can't end up the
   syscache by adding a hook that prevents that on the catcache level
3) Make sure that there can't be any duplicates by storing the oid of
   the relation in a mapped relations relfilenode

Opinions?

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
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 more regression tests for ASYNC

[Josh Berkus]

On 05/13/2013 05:37 PM, Robins Tharakan wrote:
> Hi,
> 
> Patch to add more regression tests to ASYNC (/src/backend/commands/async).
> Take code-coverage from 39% to 75%.

This isn't applying for me anymore due to changes in the
parallel_schedule file.  Where did you mean this to execute in the
sequence of tests?

--- src/test/regress/parallel_schedule
+++ src/test/regress/parallel_schedule
@@ -88,7 +88,7 @@
 # --
 # Another group of parallel tests
 # --
-test: alter_generic misc psql
+test: alter_generic misc psql async

 # rules cannot run concurrently with any test that creates a view
 test: rules

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] GIN improvements part2: fast scan

[Alexander Korotkov]

On Tue, Jun 25, 2013 at 2:20 AM, Alexander Korotkov wrote:

> 4. If we do go with a new function, I'd like to just call it "consistent"
>> (or consistent2 or something, to keep it separate form the old consistent
>> function), and pass it a tri-state input for each search term. It might not
>> be any different for the full-text search implementation, or any of the
>> other ones for that matter, but I think it would be a more understandable
>> API.
>
>
> Understandable API makes sense. But for now, I can't see even potentional
> usage of third state (exact false).
>

Typo here. I meant "exact true".


> Also, with preConsistent interface "as is" in some cases we can use old
> consistent method as both consistent and preConsistent when it implements
> monotonous boolean function. For example, it's consistent function for
> opclasses of arrays.
>

Now, I got the point of three state consistent: we can keep only one
consistent in opclasses that support new interface. exact true and exact
false values will be passed in the case of current patch consistent; exact
false and unknown will be passed in the case of current patch
preConsistent. That's reasonable.

--
With best regards,
Alexander Korotkov.

Re: [HACKERS] [9.3 CF 1] 2 Weeks In & The problem with Performance Patches

[Claudio Freire]

On Fri, Jun 28, 2013 at 4:16 PM, Josh Berkus  wrote:
> (2) ideas on how we can speed up/parallelize performance testing efforts
> are extremely welcome.


An official perf-test script in GIT, even if it only tests general
pg-bench-like performance, that can take two builds and compare them,
like unladen-swallow's perf.py[0][1], would enable regular folk
perform the testing on various hardware configurations and contribute
their results more easily. Results would be in standardized format,
which would help on the interpretation of those numbers, and patches
would also be able to add patch-specific tests to the script's
battery.

I had a bash script that ran a few tests I used when evaluating the
readahead patch. It's very very green, and very very obvious, so I
doubt it'd be useful, but if noone else has one, I could dig through
my backups.

The test handled the odd stuff about clearing the OS cache between
test runs, and stuff like that, which is required for meaningful
results. I think this is where an official perf test script can do
wonders: accumulate and share knowledge on testing methodology.

[0] http://code.google.com/p/unladen-swallow/wiki/Benchmarks
[1] http://code.google.com/p/unladen-swallow/source/browse/tests/perf.py


-- 
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] [9.3 CF 1] 2 Weeks In & The problem with Performance Patches

[Andres Freund]

On 2013-06-28 12:16:09 -0700, Josh Berkus wrote:
> - Remove PD_ALL_VISIBLE**

I think this is primarily delayed because people don't agree it's a good
idea.

> - Use posix_fallocate to build (new) WAL files**

I don't think this needs much further testing. We should just remove the
enable/disable knob and go ahead.

> - XLogInsert scaling

I don't see to many problems here so far. For a complex feature it's imo
already in a pretty good shape.
Some more eyes on the code would be good, given it touching lots of code
that's central for crash safety, but if not, I don't think it will block
things.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

[HACKERS] [9.3 CF 1] 2 Weeks In & The problem with Performance Patches

[Josh Berkus]

Folks,

We are starting the 3rd week of the commitfest tommorrow, and here's the
update on status.  In the last week, we are at the following status levels:

- 40 patches still Need Review,
including 11 which have no reviewer assigned.
- 23 patches are Waiting on Author, most of them
because they are still under discussion on this list.
- 17 patches are marked Ready for Committer,
including 10 of the 11 regression tests*
- 17 patches have been Committed
- 9 patches have been Returned with Feedback
- 1 patch has been Rejected

(* the tests actually need a test of the cumulative total time to run,
which I'll be working on today)

At current commit/return rates, this commifest will finish around July 28th.

What seems prepared to drag out this commitfest possibly beyond the end
of July are the several performance optimization patches, which include:

- Remove PD_ALL_VISIBLE**
- Improvement of checkpoint IO scheduler for stable transaction responses
- Use posix_fallocate to build (new) WAL files**
- Performance Improvement by reducing WAL for Update Operation**
- GIN improvements (4 patches)**
- XLogInsert scaling

Items marked ** are waiting on review.

The problem with these patches is that (a) people feel reluctant to
review them, since not too many hackers feel qualified to review
performance, and (b) the review will need to include performance
testing, which will make it take longer and likely bottleneck on
qualified testers.

So, two things:
(1) if you are qualified to review any of the above, please get started
*now*, and leave the non-performance patches for later;
(2) ideas on how we can speed up/parallelize performance testing efforts
are extremely welcome.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] fallocate / posix_fallocate for new WAL file creation (etc...)

[Josh Berkus]

On 06/20/2013 07:19 PM, Jeff Davis wrote:
> On Sun, 2013-06-16 at 23:53 -0500, Jon Nelson wrote:
>> Please find attached v5 of the patch, with the above correction in place.
>> The only change from the v4 patch is wrapping the if
>> (wal_use_fallocate) block in an #ifdef USE_POSIX_FALLOCATE.
>> Thanks!
> 
> Thank you.
> 
> Greg, are you still working on those tests?

Since Greg seems to be busy, what needs to be done to test this?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] Documentation/help for materialized and recursive views

[David Fetter]

On Fri, Jun 28, 2013 at 01:34:17PM -0400, Peter Eisentraut wrote:
> On 6/28/13 10:49 AM, Alvaro Herrera wrote:
> > Magnus Hagander escribió:
> > 
> >> They are already crosslinked under "see also". But that doesn't
> >> really help the guy doing "\h CREATE VIEW" in psql, which was the
> >> case where it was brought to my attention.
> > 
> > Maybe \h should somehow display the "see also" section?
> 
> You can run \! man from within psql,

And if you're on Windows, you're Sadly Out of Luck with that.  Is
there an equivalent we could #ifdef in for that platform?

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] [RFC] Minmax indexes

[Josh Berkus]

>> On Fri, Jun 28, 2013 at 2:18 PM, Jim Nasby  wrote:
>>> If we add a dirty flag it would probably be wise to allow for more
>>> than one
>>> value so we can do a clock-sweep. That would allow for detecting a range
>>> that is getting dirtied repeatedly and not bother to try and
>>> re-summarize it
>>> until later.

Given that I'm talking about doing the resummarization at VACUUM time, I
don't think that's justified.  That only makes sense if you're doing the
below ...

>>> Something else I don't think was mentioned... re-summarization should be
>>> somehow tied to access activity: if a query will need to seqscan a
>>> segment
>>> that needs to be summarized, we should take that opportunity to
>>> summarize at
>>> the same time while pages are in cache. Maybe that can be done in the
>>> backend itself; maybe we'd want a separate process.

Writing at SELECT time is something our users hate, with significant
justification.  This suggestion is possibly a useful optimization, but
I'd like to see the simplest possible version of minmax indexes go into
9.4, so that we can see how they work in practice before we start adding
complicated performance optimizations involving extra write overhead and
background workers.  We're more likely to engineer an expensive
de-optimization that way.

I wouldn't be unhappy to have the very basic minmax indexes -- the one
where we just flag pages as invalid if they get updated -- go into 9.4.
That would still work for large, WORM tables, which are a significant
and pervasive use case.  If Alvaro gets to it, I think the "updating the
range, rescan at VACUUM time" approach isn't much more complex, but if
he doesn't I'd still vote to accept the feature.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] Documentation/help for materialized and recursive views

[Alvaro Herrera]

Peter Eisentraut escribió:
> On 6/28/13 10:49 AM, Alvaro Herrera wrote:
> > Magnus Hagander escribió:
> > 
> >> They are already crosslinked under "see also". But that doesn't really
> >> help the guy doing "\h CREATE VIEW" in psql, which was the case where
> >> it was brought to my attention.
> > 
> > Maybe \h should somehow display the "see also" section?
> 
> You can run \! man from within psql, which should give you everything
> you need.  It's admittedly a bit obscure, but it's there.  Maybe it
> ought to be added to the initial help output.

That's a neat trick, but obviously it won't work in Windows.  Maybe we
ought to have \man ..

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] [RFC] Minmax indexes

[Jim Nasby]

On 6/28/13 12:26 PM, Claudio Freire wrote:

On Fri, Jun 28, 2013 at 2:18 PM, Jim Nasby  wrote:

On 6/17/13 3:38 PM, Josh Berkus wrote:


Why?  Why can't we just update the affected pages in the index?




The page range has to be scanned in order to find out the min/max values
for the indexed columns on the range; and then, with these data, update
the index.


Seems like you could incrementally update the range, at least for
inserts.  If you insert a row which doesn't decrease the min or increase
the max, you can ignore it, and if it does increase/decrease, you can
change the min/max.  No?

For updates, things are more complicated.  If the row you're updating
was the min/max, in theory you should update it to adjust that, but you
can't verify that it was the ONLY min/max row without doing a full scan.
   My suggestion would be to add a "dirty" flag which would indicate that
that block could use a rescan next VACUUM, and otherwise ignore changing
the min/max.  After all, the only defect to having min to low or max too
high for a block would be scanning too many blocks.  Which you'd do
anyway with it marked "invalid".



If we add a dirty flag it would probably be wise to allow for more than one
value so we can do a clock-sweep. That would allow for detecting a range
that is getting dirtied repeatedly and not bother to try and re-summarize it
until later.

Something else I don't think was mentioned... re-summarization should be
somehow tied to access activity: if a query will need to seqscan a segment
that needs to be summarized, we should take that opportunity to summarize at
the same time while pages are in cache. Maybe that can be done in the
backend itself; maybe we'd want a separate process.



This smells a lot like hint bits and all the trouble they bring.


Possibly; though if that's the case just having a second process do the work 
would probably eliminate most of that problem.
--
Jim C. Nasby, Data Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


--
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] Documentation/help for materialized and recursive views

[Peter Eisentraut]

On 6/28/13 10:49 AM, Alvaro Herrera wrote:
> Magnus Hagander escribió:
> 
>> They are already crosslinked under "see also". But that doesn't really
>> help the guy doing "\h CREATE VIEW" in psql, which was the case where
>> it was brought to my attention.
> 
> Maybe \h should somehow display the "see also" section?

You can run \! man from within psql, which should give you everything
you need.  It's admittedly a bit obscure, but it's there.  Maybe it
ought to be added to the initial help output.



-- 
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] Department of Redundancy Department: makeNode(FuncCall) division

[Peter Eisentraut]

On 6/28/13 11:30 AM, Robert Haas wrote:
> On Fri, Jun 28, 2013 at 10:31 AM, Tom Lane  wrote:
>> David Fetter  writes:
>>> Please find attached the latest patch.
>>
>> I remain of the opinion that this is simply a bad idea.  It is unlike
>> our habits for constructing other types of nodes, and makes it harder
>> not easier to find all the places that need to be updated when adding
>> another field to FuncCall.
> 
> I think it's a nice code cleanup.  I don't understand your objection.

Yeah, I was reading the patch thinking, yes, finally someone cleans that up.



-- 
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] Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Robert Haas]

On Fri, Jun 28, 2013 at 12:29 PM, Nicholas White  wrote:
>> This patch will need to be rebased over those changes
>
> See attached -

Thanks.  But I see a few issues...

+ [respect nulls]|[ignore nulls]

You fixed one of these but missed the other.

default are evaluated
with respect to the current row.  If omitted,
offset defaults to 1 and
-   default to null
+   IGNORE NULLS is specified then the function will
be evaluated
+   as if the rows containing nulls didn't exist.
   
  

This looks like you dropped a line during cut-and-paste.

+   null_values = (Bitmapset *) WinGetPartitionLocalMemory(
+   winobj,
+   BITMAPSET_SIZE(words_needed));
+   Assert(null_values);

This certainly seems ugly - isn't there a way to accomplish this
without having to violate the Bitmapset abstraction?

+* A slight edge case. Consider:
+*
+* A | lag(A, 1)
+* 1 |
+* 2 |  1
+*   |  ?

pgindent will reformat this into oblivion.  Use - markers around
the comment block as done elsewhere in the code where this is an
issue, or don't use ASCII art.

I haven't really reviewed the windowing-related code in depth; I
thought Jeff might jump back in for that part of it.  Jeff, is that
something you're planning to do?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] [RFC] Minmax indexes

[Claudio Freire]

On Fri, Jun 28, 2013 at 2:18 PM, Jim Nasby  wrote:
> On 6/17/13 3:38 PM, Josh Berkus wrote:

 Why?  Why can't we just update the affected pages in the index?
>>>
>>> >
>>> >The page range has to be scanned in order to find out the min/max values
>>> >for the indexed columns on the range; and then, with these data, update
>>> >the index.
>>
>> Seems like you could incrementally update the range, at least for
>> inserts.  If you insert a row which doesn't decrease the min or increase
>> the max, you can ignore it, and if it does increase/decrease, you can
>> change the min/max.  No?
>>
>> For updates, things are more complicated.  If the row you're updating
>> was the min/max, in theory you should update it to adjust that, but you
>> can't verify that it was the ONLY min/max row without doing a full scan.
>>   My suggestion would be to add a "dirty" flag which would indicate that
>> that block could use a rescan next VACUUM, and otherwise ignore changing
>> the min/max.  After all, the only defect to having min to low or max too
>> high for a block would be scanning too many blocks.  Which you'd do
>> anyway with it marked "invalid".
>
>
> If we add a dirty flag it would probably be wise to allow for more than one
> value so we can do a clock-sweep. That would allow for detecting a range
> that is getting dirtied repeatedly and not bother to try and re-summarize it
> until later.
>
> Something else I don't think was mentioned... re-summarization should be
> somehow tied to access activity: if a query will need to seqscan a segment
> that needs to be summarized, we should take that opportunity to summarize at
> the same time while pages are in cache. Maybe that can be done in the
> backend itself; maybe we'd want a separate process.


This smells a lot like hint bits and all the trouble they bring.


-- 
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] [RFC] Minmax indexes

[Jim Nasby]

On 6/17/13 3:38 PM, Josh Berkus wrote:

Why?  Why can't we just update the affected pages in the index?

>
>The page range has to be scanned in order to find out the min/max values
>for the indexed columns on the range; and then, with these data, update
>the index.

Seems like you could incrementally update the range, at least for
inserts.  If you insert a row which doesn't decrease the min or increase
the max, you can ignore it, and if it does increase/decrease, you can
change the min/max.  No?

For updates, things are more complicated.  If the row you're updating
was the min/max, in theory you should update it to adjust that, but you
can't verify that it was the ONLY min/max row without doing a full scan.
  My suggestion would be to add a "dirty" flag which would indicate that
that block could use a rescan next VACUUM, and otherwise ignore changing
the min/max.  After all, the only defect to having min to low or max too
high for a block would be scanning too many blocks.  Which you'd do
anyway with it marked "invalid".


If we add a dirty flag it would probably be wise to allow for more than one 
value so we can do a clock-sweep. That would allow for detecting a range that 
is getting dirtied repeatedly and not bother to try and re-summarize it until 
later.

Something else I don't think was mentioned... re-summarization should be 
somehow tied to access activity: if a query will need to seqscan a segment that 
needs to be summarized, we should take that opportunity to summarize at the 
same time while pages are in cache. Maybe that can be done in the backend 
itself; maybe we'd want a separate process.
--
Jim C. Nasby, Data Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


--
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] changeset generation v5-01 - Patches & git tree

[Simon Riggs]

On 28 June 2013 17:10, Robert Haas  wrote:


> > But to tell the truth, I'm mostly exercised about the non-unique
> > syscache.  I think that's simply a *bad* idea.
>
> +1.
>
> I don't think the extra index on pg_class is going to hurt that much,
> even if we create it always, as long as we use a purpose-built caching
> mechanism for it rather than forcing it through catcache.


Hmm, does seem like that would be better.


> The people
> who are going to suffer are the ones who create and drop a lot of
> temporary tables, but even there I'm not sure how visible the overhead
> will be on real-world workloads, and maybe the solution is to work
> towards not having permanent catalog entries for temporary tables in
> the first place.  In any case, hurting people who use temporary tables
> heavily seems better than adding overhead to every
> insert/update/delete operation, which will hit all users who are not
> read-only.
>

Thinks...

If we added a trigger that fired a NOTIFY for any new rows in pg_class that
relate to non-temporary relations that would optimise away any overhead for
temporary tables or when no changeset extraction was in progress.

The changeset extraction could build a private hash table to perform the
lookup and then LISTEN on a specific channel for changes.

That might work better than an index-plus-syscache.


> On the other hand, I can't entirely shake the feeling that adding the
> information into WAL would be more reliable.


I don't really like the idea of requiring the relid on the WAL record. WAL
is big enough already and we want people to turn this on, not avoid it.

This is just an index lookup. We do them all the time without any fear of
reliability issues.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] PostgreSQL 9.3 latest dev snapshot

[Alvaro Herrera]

Magnus Hagander escribió:

> However, if oyu're looking for a snapshot, please use the one on the
> ftpsite. Generating those snapshots on the git server is slow and
> expensive...

Maybe we should redirect those gitweb snapshot URLs to the FTP site?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] ALTER TABLE ... ALTER CONSTRAINT

[Alvaro Herrera]

Simon Riggs escribió:

> Looks like that really is a deficiency in my tool chain on OSX, rather than
> some bug/user error. Even at the very latest, very shiny version.
> 
> Latest versions of gcc trap the error, so I'll have to investigate an
> alternative.

Funnily enough, on Debian Wheezy with gcc 4.7.2 I don't get the warning,
and Andres with gcc 4.7.3 (from Debian unstable) does see it.  (Of
course, the 4.8 version shipped with unstable also shows it.)

Clang similarly requires pretty new versions to show the warning.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Nicholas White]

> This patch will need to be rebased over those changes

See attached -


lead-lag-ignore-nulls.patch
Description: Binary data

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

Re: [HACKERS] changeset generation v5-01 - Patches & git tree

[Tom Lane]

Robert Haas  writes:
> On the other hand, I can't entirely shake the feeling that adding the
> information into WAL would be more reliable.

That feeling has been nagging at me too.  I can't demonstrate that
there's a problem when an ALTER TABLE is in process of rewriting a table
into a new relfilenode number, but I don't have a warm fuzzy feeling
about the reliability of reverse lookups for this.  At the very least
it's going to require some hard-to-verify restriction about how we
can't start doing changeset reconstruction in the middle of a
transaction that's doing DDL.

regards, tom lane


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

Re: [HACKERS] Move unused buffers to freelist

[Robert Haas]

On Fri, Jun 28, 2013 at 12:10 PM, Greg Smith  wrote:
> This refactoring idea will make that hard to keep around.  I think this is
> OK though.  Switching to a latch based design should eliminate the
> bgwriter_delay, which means you won't have this worst case of a 200ms stall
> while heavy activity is incoming.

I'm a strong proponent of that 2 minute cycle, so I'd vote for finding
a way to keep it around.  But I don't think that (or 200 ms wakeups)
should be the primary thing driving the background writer, either.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] changeset generation v5-01 - Patches & git tree

[Robert Haas]

On Fri, Jun 28, 2013 at 11:56 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> I'm just talking out of my rear end here because I don't know what the
>> real numbers are, but it's far from obvious to me that there's any
>> free lunch here.  That having been said, just because indexing
>> relfilenode or adding relfilenodes to WAL records is expensive doesn't
>> mean we shouldn't do it.  But I think we need to know the price tag
>> before we can judge whether to make the purchase.
>
> Certainly, any of these solutions are going to cost us somewhere ---
> either up-front cost or more expensive (and less reliable?) changeset
> extraction, take your choice.  I will note that somehow tablespaces got
> put in despite having to add 4 bytes to every WAL record for that
> feature, which was probably of less use than logical changeset
> extraction will be.

Right.  I actually think we booted that one.  The database ID is a
constant for most people.  The tablespace ID is not technically
redundant, but in 99.99% of cases you could figure it out from the
database ID + relation ID.  The relation ID is where 99% of the
entropy is, but it probably only has 8-16 bits of entropy in most
real-world use cases.  If we were doing this over we might want to
think about storing a proxy for the relfilenode rather than the
relfilenode itself, but there's not much good crying over it now.

> But to tell the truth, I'm mostly exercised about the non-unique
> syscache.  I think that's simply a *bad* idea.

+1.

I don't think the extra index on pg_class is going to hurt that much,
even if we create it always, as long as we use a purpose-built caching
mechanism for it rather than forcing it through catcache.  The people
who are going to suffer are the ones who create and drop a lot of
temporary tables, but even there I'm not sure how visible the overhead
will be on real-world workloads, and maybe the solution is to work
towards not having permanent catalog entries for temporary tables in
the first place.  In any case, hurting people who use temporary tables
heavily seems better than adding overhead to every
insert/update/delete operation, which will hit all users who are not
read-only.

On the other hand, I can't entirely shake the feeling that adding the
information into WAL would be more reliable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Move unused buffers to freelist

[Greg Smith]

On 6/28/13 8:50 AM, Robert Haas wrote:

On Fri, Jun 28, 2013 at 12:52 AM, Amit Kapila  wrote:

4. Separate processes for writing dirty buffers and moving buffers to
freelist


I think this part might be best pushed to a separate patch, although I
agree we probably need it.


This might be necessary eventually, but it's going to make thing more 
complicated.  And I don't think it's a blocker for creating something 
useful.  The two most common workloads are:


1) Lots of low usage count data, typically data that is updated sparsely 
across a larger database.  These are helped by a process that writes 
dirty buffers in the background.  These benefit from the current 
background writer.  Kevin's system he was just mentioning again is the 
best example of this type that there's public data on.


2) Lots of high usage count data, because there are large hotspots in 
things like index blocks.  Most writes happen at checkpoint time, 
because the background writer won't touch them.  Because there are only 
a small number of re-usable pages, the clock sweep goes around very fast 
looking for them.  This is the type of workload that should benefit from 
putting buffers into the free list.  pgbench provides a simple example 
of this type, which is why Amit's tests using it have been useful.


If you had a process that tried to handle both background writes and 
freelist management, I suspect one path would be hot and the other 
almost idle in each type of system.  I don't expect that splitting those 
into two separate process would buy a lot of value, that can easily be 
pushed to a later patch.



The background writer would just
have a high and a low watermark.  When the number of buffers on the
freelist drops below the low watermark, the allocating backend sets
the latch and bgwriter wakes up and begins adding buffers to the
freelist.  When the number of buffers on the free list reaches the
high watermark, the background writer goes back to sleep.


This will work fine for all of the common workloads.  The main challenge 
is keeping the buffer allocation counting from turning into a hotspot. 
Busy systems now can easily hit 100K buffer allocations/second.  I'm not 
too worried about it because those allocations are making the free list 
lock a hotspot right now.


One of the consistently controversial parts of the current background 
writer is how it tries to loop over the buffer cache every 2 minutes, 
regardless of activity level.  The idea there was that on bursty 
workloads, buffers would be cleaned during idle periods with that 
mechanism.  Part of why that's in there is to deal with the relatively 
long pause between background writer runs.


This refactoring idea will make that hard to keep around.  I think this 
is OK though.  Switching to a latch based design should eliminate the 
bgwriter_delay, which means you won't have this worst case of a 200ms 
stall while heavy activity is incoming.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com


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

Re: [HACKERS] ALTER TABLE ... ALTER CONSTRAINT

[Simon Riggs]

On 24 June 2013 22:17, Simon Riggs  wrote:

> On 24 June 2013 21:42, Jeff Janes  wrote:
>
>> On Sun, Jun 23, 2013 at 8:58 PM, Abhijit Menon-Sen 
>> wrote:
>>
>>> At 2013-06-08 21:45:24 +0100, si...@2ndquadrant.com wrote:
>>> >
>>> > ALTER TABLE foo
>>> >ALTER CONSTRAINT fktable_fk_fkey DEFERRED INITIALLY IMMEDIATE;
>>>
>>> I read the patch (looks good), applied it on HEAD (fine), ran make check
>>> (fine), and played with it in a test database. It looks great, and from
>>> previous responses it's something a lot of people have wished for.
>>>
>>> I'm marking this ready for committer.
>>>
>>
>> After the commit, I'm now getting the compiler warning:
>>
>> tablecmds.c: In function 'ATPrepCmd':
>> tablecmds.c:2953: warning: 'pass' may be used uninitialized in this
>> function
>>
>>
>> case AT_AlterConstraint (line 3130) is the only case branch that does not
>> set pass.
>>
>
> The investigation is into why my current compiler didn't report that.
> Thanks though.
>

Looks like that really is a deficiency in my tool chain on OSX, rather than
some bug/user error. Even at the very latest, very shiny version.

Latest versions of gcc trap the error, so I'll have to investigate an
alternative.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] proposal: enable new error fields in plpgsql (9.4)

[Noah Misch]

On Fri, Jun 28, 2013 at 05:21:29PM +0200, Pavel Stehule wrote:
> 2013/6/28 Noah Misch :
> > Okay.  I failed to note the first time through that while the patch uses the
> > same option names for RAISE and GET STACKED DIAGNOSTICS, the existing option
> > lists for those commands differ:
> >
> > --RAISE option----GET STACKED DIAGNOSTICS option--
> > ERRCODE RETURNED_SQLSTATE
> > MESSAGE MESSAGE_TEXT
> > DETAIL  PG_EXCEPTION_DETAIL
> > HINTPG_EXCEPTION_HINT
> > CONTEXT PG_EXCEPTION_CONTEXT
> >
> > To be consistent with that pattern, I think we would use COLUMN, CONSTRAINT,
> > TABLE, TYPE and SCHEMA as the new RAISE options.
> 
> I understand to your motivation, but I am not sure. Minimally word
> "TYPE" is too general. I have not strong opinion in this area. maybe
> DATATYPE ??

I'm not positive either.  DATATYPE rather than TYPE makes sense.

> p.s. you cannot to specify CONTEXT in RAISE statement

Oops; right.

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


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

Re: [HACKERS] PostgreSQL 9.3 latest dev snapshot

[Stefan Kaltenbrunner]

On 06/27/2013 12:22 PM, Magnus Hagander wrote:
> On Tue, Jun 25, 2013 at 3:31 PM, Michael Paquier
>  wrote:
>>
>> On 2013/06/25, at 22:23, Fujii Masao  wrote:
>>
>>> On Tue, Jun 25, 2013 at 6:33 PM, Michael Paquier
>>>  wrote:
 On Tue, Jun 25, 2013 at 5:33 PM, Misa Simic  wrote:
> Hi,
>
> Where we can find latest snapshot for 9.3 version?
>
> We have taken latest snapshot from
> http://ftp.postgresql.org/pub/snapshot/dev/
>
> But it seems it is for 9.4 version...
 9.3 has moved to branch REL9_3_STABLE a couple of days ago.
>>>
>>> Yes. We can find the snapshot from REL9_3_STABLE git branch.
>>> http://git.postgresql.org/gitweb/?p=postgresql.git;a=shortlog;h=refs/heads/REL9_3_STABLE
>> Indeed, I completely forgot that you can download snapshots from 
>> postgresql.org's git. Simply use that instead of the FTP server now as long 
>> as 9.3 snapshots are not generated there.
> 
> In case somebody is still looking, snapshots are properly building for 9.3 
> now.
> 
> Those snapshots aren't identical to a download from git, as they've
> gone through a "make dist-prep" or whatever it's called. But they're
> pretty close.

there is more to that - those snapshots also will also only get
published if the source passed a full buildfarm run  as a basic form of
validation.


> 
> However, if oyu're looking for a snapshot, please use the one on the
> ftpsite. Generating those snapshots on the git server is slow and
> expensive...

definitly


Stefan


-- 
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] changeset generation v5-01 - Patches & git tree

[Tom Lane]

Robert Haas  writes:
> I'm just talking out of my rear end here because I don't know what the
> real numbers are, but it's far from obvious to me that there's any
> free lunch here.  That having been said, just because indexing
> relfilenode or adding relfilenodes to WAL records is expensive doesn't
> mean we shouldn't do it.  But I think we need to know the price tag
> before we can judge whether to make the purchase.

Certainly, any of these solutions are going to cost us somewhere ---
either up-front cost or more expensive (and less reliable?) changeset
extraction, take your choice.  I will note that somehow tablespaces got
put in despite having to add 4 bytes to every WAL record for that
feature, which was probably of less use than logical changeset
extraction will be.

But to tell the truth, I'm mostly exercised about the non-unique
syscache.  I think that's simply a *bad* idea.

regards, tom lane


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

Re: [HACKERS] Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Robert Haas]

On Fri, Jun 28, 2013 at 11:41 AM, Alvaro Herrera
 wrote:
> Robert Haas escribió:
>> On Thu, Jun 27, 2013 at 8:52 PM, Nicholas White  wrote:
>
>> The documentation you've added reads kind of funny to me:
>>
>> + [respect nulls]|[ignore nulls]
>>
>> Wouldn't we normally write that as [ [ RESPECT | IGNORE ] NULLS ] ?
>
> I think it should be
> [ { RESPECT | IGNORE } NULLS ]
> One of the leading keywords must be present.

Oh, yeah.  What he said.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] changeset generation v5-01 - Patches & git tree

[Alvaro Herrera]

Alvaro Herrera escribió:

> An INSERT wal record is:
> 
> typedef struct xl_heap_insert
> {
>   xl_heaptid  target; /* inserted tuple id */
>   boolall_visible_cleared;/* PD_ALL_VISIBLE was cleared */
>   /* xl_heap_header & TUPLE DATA FOLLOWS AT END OF STRUCT */
> } xl_heap_insert;

Oops.  xl_heaptid is not 6 bytes, but instead:

typedef struct xl_heaptid
{
RelFileNode node;
ItemPointerData tid;
} xl_heaptid;

typedef struct RelFileNode
{
Oid spcNode;
Oid dbNode;
Oid relNode;
} RelFileNode;  /* 12 bytes */

typedef struct ItemPointerData
{
BlockIdData ip_blkid;
OffsetNumber ip_posid;
};  /* 6 bytes */

typedef struct BlockIdData
{
uint16  bi_hi;
uint16  bi_lo;
} BlockIdData;  /* 4 bytes */

typedef uint16 OffsetNumber;

There's purposely no alignment padding anywhere, so xl_heaptid totals 22 bytes.


Therefore,

> So the fixed part is just 22 bytes + 5 bytes; tuple data follows that.
> So adding four more bytes could indeed be significant (but by how much,
> depends on the size of the tuple data).

4 extra bytes on top of 27 is 14% of added overhead (considering only
the xlog header.)

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] Review: query result history in psql

[Pavel Stehule]

Hello

I am not sure, this interface is really user friendly

there is not possible "searching" in history, and not every query push
to history some interesting content.

It require:

* simply decision if content should be stored in history or not,
* simply remove last entry (table) of history
* queries should be joined to content, only name is not enough

Regards

Pavel

2013/6/28 Maciej Gajewski :
> Thanks for checking the patch!
>
> So what's left to fix?
> * Moving the escaping-related functions to separate module,
> * applying your corrections.
>
> Did I missed anything?
>
> I'll submit corrected patch after the weekend.
>
> M
>


-- 
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] Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Alvaro Herrera]

Robert Haas escribió:
> On Thu, Jun 27, 2013 at 8:52 PM, Nicholas White  wrote:

> The documentation you've added reads kind of funny to me:
> 
> + [respect nulls]|[ignore nulls]
> 
> Wouldn't we normally write that as [ [ RESPECT | IGNORE ] NULLS ] ?

I think it should be
[ { RESPECT | IGNORE } NULLS ]
One of the leading keywords must be present.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] changeset generation v5-01 - Patches & git tree

[Alvaro Herrera]

Robert Haas escribió:
> On Fri, Jun 28, 2013 at 10:49 AM, Tom Lane  wrote:
> > Robert's idea sounds fairly reasonable to me; another 4 bytes per
> > insert/update/delete WAL entry isn't that big a deal, ...
> 
> How big a deal is it?  This is a serious question, because I don't
> know.  Let's suppose that the average size of an XLOG_HEAP_INSERT
> record is 100 bytes.  Then if we add 4 bytes, isn't that a 4%
> overhead?  And doesn't that seem significant?

An INSERT wal record is:

typedef struct xl_heap_insert
{
xl_heaptid  target; /* inserted tuple id */
boolall_visible_cleared;/* PD_ALL_VISIBLE was cleared */
/* xl_heap_header & TUPLE DATA FOLLOWS AT END OF STRUCT */
} xl_heap_insert;

typedef struct xl_heap_header
{
uint16  t_infomask2;
uint16  t_infomask;
uint8   t_hoff;
} xl_heap_header;

So the fixed part is just 7 bytes + 5 bytes; tuple data follows that.
So adding four more bytes could indeed be significant (but by how much,
depends on the size of the tuple data).  Adding a new pg_class index
would be larger in the sense that there are more WAL records, and
there's the extra vacuuming traffic; but on the other hand that would
only happen when tables are created.  It seems safe to assume that in
normal use cases the ratio of tuple insertion vs. table creation is
large.

The only idea that springs to mind is to have the new pg_class index be
created conditionally, i.e. only when logical replication is going to be
used.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] Department of Redundancy Department: makeNode(FuncCall) division

[Robert Haas]

On Fri, Jun 28, 2013 at 10:31 AM, Tom Lane  wrote:
> David Fetter  writes:
>> Please find attached the latest patch.
>
> I remain of the opinion that this is simply a bad idea.  It is unlike
> our habits for constructing other types of nodes, and makes it harder
> not easier to find all the places that need to be updated when adding
> another field to FuncCall.

I think it's a nice code cleanup.  I don't understand your objection.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Robert Haas]

On Thu, Jun 27, 2013 at 8:52 PM, Nicholas White  wrote:
>> The result of the current patch using lead
>
> Actually, I think I agree with you (and, FWIW, so does Oracle:
> http://docs.oracle.com/cd/E11882_01/server.112/e25554/analysis.htm#autoId18).
> I've refactored the window function's implementation so that (e.g.) lead(5)
> means the 5th non-null value away in front of the current row (the previous
> implementation was the last non-null value returned if the 5th rows in front
> was null). These semantics are slower, as the require the function to scan
> through the tuples discarding non-null ones. I've made the implementation
> use a bitmap in the partition context to cache whether or not a given tuple
> produces a null. This seems correct (it passes the regression tests) but as
> it stores row offsets (which are int64s) I was careful not to use bitmap
> methods that use ints to refer to set members. I've added more explanation
> in the code's comments. Thanks -

The documentation you've added reads kind of funny to me:

+ [respect nulls]|[ignore nulls]

Wouldn't we normally write that as [ [ RESPECT | IGNORE ] NULLS ] ?

I've committed the changes from Troels to avoid the grammar conflicts,
and I also took the opportunity to make OVER unreserved, as allowed by
his refactoring and per related discussion on other threads.  This
patch will need to be rebased over those changes (sorry), but
hopefully it'll help the review of this patch focus in on the issues
that are specific to RESPECT/IGNORE NULLS.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] extensible external toast tuple support

[Robert Haas]

On Fri, Jun 28, 2013 at 10:53 AM, Andres Freund  wrote:
>> Why does toast_insert_or_update() need to go through all the
>> rigamarole in toast_datum_differs()?  I would have thought that it
>> could simply treat any external-indirect value as needing to be
>> detoasted and retoasted, since the destination is the disk anyhow.
>
> We could do that, yes. But I think it might be better not to: If we
> simplify the tuples used in a query to not reference ondisk tuples
> anymore and we then UPDATE using that new version I would rather not
> retoast all the unchanged columns.
>
> I can e.g. very well imagine that we decide to resolve toasted Datums to
> indirect Datums during an UPDATE if there are multiple BEFORE UPDATE
> triggers to avoid detoasting in each and every one of them. Such a tuple
> will then passed to heap_update...

I must be missing something.  At that point, yes, you'd like to avoid
re-toasting unnecessarily, but ISTM you've already bought the farm.
Unless I'm misunderstanding the code as written, you'd just end up
writing the indirect pointer back out to disk in that scenario.
That's not gonna work...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] proposal: enable new error fields in plpgsql (9.4)

[Pavel Stehule]

2013/6/28 Noah Misch :
> On Fri, Jun 28, 2013 at 03:31:00PM +0200, Pavel Stehule wrote:
>> Hello
>>
>> 2013/6/28 Noah Misch :
>> > On Fri, Jun 28, 2013 at 07:49:46AM +0200, Pavel Stehule wrote:
>> >> 2013/6/28 Noah Misch :
>> >> > On Tue, Jun 25, 2013 at 03:56:27PM +0200, Pavel Stehule wrote:
>> >> >> cleaned patch is in attachment
>> >> >
>> >> > Of the five options you're adding to GET STACKED DIAGNOSTICS, four of 
>> >> > them
>> >> > appear in the SQL standard.  DATATYPE_NAME does not; I think we should 
>> >> > call it
>> >> > PG_DATATYPE_NAME in line with our other extensions in this area.
>> >> >
>> >>
>> >> yes, but It should be fixed in 9.3 enhanced fields too - it should be
>> >> consistent with PostgreSQL fields
>> >
>> > What else, specifically, should be renamed?  (Alternately, would you 
>> > prepare a
>> > new version of the patch incorporating the proper name changes?)
>>
>> I looked to source code, and identifiers in our source code are
>> consistent, so my comment hasn't sense. Yes, I agree, so only
>> identifier used in GET DIAGNOSTICS statement should be renamed. So,
>> only DATATYPE_NAME should be renamed to PG_DATATYPE_NAME.
>
> Okay.  I failed to note the first time through that while the patch uses the
> same option names for RAISE and GET STACKED DIAGNOSTICS, the existing option
> lists for those commands differ:
>
> --RAISE option----GET STACKED DIAGNOSTICS option--
> ERRCODE RETURNED_SQLSTATE
> MESSAGE MESSAGE_TEXT
> DETAIL  PG_EXCEPTION_DETAIL
> HINTPG_EXCEPTION_HINT
> CONTEXT PG_EXCEPTION_CONTEXT
>
> To be consistent with that pattern, I think we would use COLUMN, CONSTRAINT,
> TABLE, TYPE and SCHEMA as the new RAISE options.

I understand to your motivation, but I am not sure. Minimally word
"TYPE" is too general. I have not strong opinion in this area. maybe
DATATYPE ??

p.s. you cannot to specify CONTEXT in RAISE statement

Regards

Pavel

>
> --
> Noah Misch
> EnterpriseDB http://www.enterprisedb.com


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

Re: [HACKERS] changeset generation v5-01 - Patches & git tree

[Robert Haas]

On Fri, Jun 28, 2013 at 10:49 AM, Tom Lane  wrote:
> Robert's idea sounds fairly reasonable to me; another 4 bytes per
> insert/update/delete WAL entry isn't that big a deal, ...

How big a deal is it?  This is a serious question, because I don't
know.  Let's suppose that the average size of an XLOG_HEAP_INSERT
record is 100 bytes.  Then if we add 4 bytes, isn't that a 4%
overhead?  And doesn't that seem significant?

I'm just talking out of my rear end here because I don't know what the
real numbers are, but it's far from obvious to me that there's any
free lunch here.  That having been said, just because indexing
relfilenode or adding relfilenodes to WAL records is expensive doesn't
mean we shouldn't do it.  But I think we need to know the price tag
before we can judge whether to make the purchase.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] proposal: enable new error fields in plpgsql (9.4)

[Noah Misch]

On Fri, Jun 28, 2013 at 03:31:00PM +0200, Pavel Stehule wrote:
> Hello
> 
> 2013/6/28 Noah Misch :
> > On Fri, Jun 28, 2013 at 07:49:46AM +0200, Pavel Stehule wrote:
> >> 2013/6/28 Noah Misch :
> >> > On Tue, Jun 25, 2013 at 03:56:27PM +0200, Pavel Stehule wrote:
> >> >> cleaned patch is in attachment
> >> >
> >> > Of the five options you're adding to GET STACKED DIAGNOSTICS, four of 
> >> > them
> >> > appear in the SQL standard.  DATATYPE_NAME does not; I think we should 
> >> > call it
> >> > PG_DATATYPE_NAME in line with our other extensions in this area.
> >> >
> >>
> >> yes, but It should be fixed in 9.3 enhanced fields too - it should be
> >> consistent with PostgreSQL fields
> >
> > What else, specifically, should be renamed?  (Alternately, would you 
> > prepare a
> > new version of the patch incorporating the proper name changes?)
> 
> I looked to source code, and identifiers in our source code are
> consistent, so my comment hasn't sense. Yes, I agree, so only
> identifier used in GET DIAGNOSTICS statement should be renamed. So,
> only DATATYPE_NAME should be renamed to PG_DATATYPE_NAME.

Okay.  I failed to note the first time through that while the patch uses the
same option names for RAISE and GET STACKED DIAGNOSTICS, the existing option
lists for those commands differ:

--RAISE option----GET STACKED DIAGNOSTICS option--
ERRCODE RETURNED_SQLSTATE
MESSAGE MESSAGE_TEXT
DETAIL  PG_EXCEPTION_DETAIL
HINTPG_EXCEPTION_HINT
CONTEXT PG_EXCEPTION_CONTEXT

To be consistent with that pattern, I think we would use COLUMN, CONSTRAINT,
TABLE, TYPE and SCHEMA as the new RAISE options.

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


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

Re: [HACKERS] changeset generation v5-01 - Patches & git tree

[Andres Freund]

On 2013-06-28 10:49:26 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2013-06-28 08:41:46 -0400, Robert Haas wrote:
> >> The alternative I previously proposed was to make the WAL records
> >> carry the relation OID.  There are a few problems with that: one is
> >> that it's a waste of space when logical replication is turned off, and
> >> it might not be easy to only do it when logical replication is on.
> >> Also, even when logic replication is turned on, things that make WAL
> >> bigger aren't wonderful.  On the other hand, it does avoid the
> >> overhead of another index on pg_class.
> 
> > I personally favor making catalog modifications a bit more more
> > expensive instead of increasing the WAL volume during routine
> > operations.
> 
> This argument is nonsense, since it conveniently ignores the added WAL
> entries created as a result of additional pg_class index manipulations.

Huh? Sure, pg_class manipulations get more expensive. But in most
clusters pg_class modifications are by far the minority compared to the
rest of the updates performed.

> Robert's idea sounds fairly reasonable to me; another 4 bytes per
> insert/update/delete WAL entry isn't that big a deal, and it would
> probably ease many debugging tasks as well as what you want to do.
> So I'd vote for including the rel OID all the time, not conditionally.

Ok, I can sure live with that. I don't think it's a problem to make it
conditionally if we want to. Making it unconditional would sure make WAL
debugging in general more pleasant though.

> The real performance argument against the patch as you have it is that
> it saddles every PG installation with extra overhead for pg_class
> updates whether or not that installation ever has or ever will make use
> of changeset generation --- unlike including rel OIDs in WAL entries,
> which might be merely difficult to handle conditionally, it's flat-out
> impossible to turn such an index on or off.  Moreover, even if one is
> using changeset generation, the overhead is being imposed at the wrong
> place, ie the master not the slave doing changeset extraction.

There's no required slaves for doing changeset extraction
anymore. Various people opposed that pretty violently, so it's now all
happening on the master. Which IMHO turned out to be the right decision.

We can do it on Hot Standby nodes, but its absolutely not required.

> But that's not the only problem, nor even the worst one IMO.  I said
> before that a syscache with a non-unique key is broken by design, and
> I stand by that estimate.  Even assuming that this usage doesn't create
> bugs in the code as it stands, it might well foreclose future changes or
> optimizations that we'd like to make in the catcache code.

Since the only duplicate key that possibly can occur in that cache is
InvalidOid, I wondered whether we could define a 'filter' that prohibits
those ending up in the cache? Then the cache would be unique.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

[HACKERS] Re: Department of Redundancy Department: makeNode(FuncCall) division

[Noah Misch]

On Fri, Jun 28, 2013 at 10:31:16AM -0400, Tom Lane wrote:
> David Fetter  writes:
> > Please find attached the latest patch.
> 
> I remain of the opinion that this is simply a bad idea.  It is unlike
> our habits for constructing other types of nodes, and makes it harder
> not easier to find all the places that need to be updated when adding
> another field to FuncCall.

We have precedents in makeRangeVar() and makeDefElem().

For me, this change would make it slightly easier to visit affected code sites
after a change.  I could cscope for callers of makeFuncCall() instead of doing
"git grep 'makeNode(FuncCall)'".  The advantage could go either way depending
on your tooling, though.

By having each call site only mention the seldom-used fields for which it does
something special, the distinctive aspects of the call site stand out better.
That's a nice advantage.

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


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

Re: [HACKERS] extensible external toast tuple support

[Andres Freund]

On 2013-06-28 09:49:53 -0400, Robert Haas wrote:
> On Fri, Jun 28, 2013 at 9:23 AM, Robert Haas  wrote:
> > On Thu, Jun 27, 2013 at 12:56 PM, Andres Freund  
> > wrote:
> >> Please find attached the next version of the extensible toast
> >> support. There basically are two changes:
> >>
> >> * handle indirect toast tuples properly in heap_tuple_fetch_attr
> >>   and related places
> >> * minor comment adjustments
> >
> > It looks to me like you need to pass true, rather than false, as the
> > first argument to TrapMacro:
> >
> > +#define VARTAG_SIZE(tag) \
> > +   ((tag) == VARTAG_INDIRECT ? sizeof(varatt_indirect) :   \
> > +(tag) == VARTAG_ONDISK ? sizeof(varatt_external) : \
> > +TrapMacro(false, "unknown vartag"))

Heh. I was obviously trying to be too cute ;)

Good idea & thanks for committing it separately.

> Do you see external-indirect values getting used for anything other
> than logical replication?  Is there code to do so anywhere?

Yes and not really. I think we can reuse it to avoid repeated detoastings, even
in relatively normal queries. What I am absolutely not sure yet is how
to automatically decide when we want to keep the tuple in memory because
it will be beneficial, and when not because of the obvious memory
overhead. And how to keep track of the memory context used to allocate
the referenced data.
There are some usecases where I think it might be relatively easy to
decide its a win. E.g tuples passed to triggers if there are
multiple ones of them.

I've played a bit with using that functionality in C functions, but
nothing publishable, more to make sure things work.

> Why does toast_insert_or_update() need to go through all the
> rigamarole in toast_datum_differs()?  I would have thought that it
> could simply treat any external-indirect value as needing to be
> detoasted and retoasted, since the destination is the disk anyhow.

We could do that, yes. But I think it might be better not to: If we
simplify the tuples used in a query to not reference ondisk tuples
anymore and we then UPDATE using that new version I would rather not
retoast all the unchanged columns.

I can e.g. very well imagine that we decide to resolve toasted Datums to
indirect Datums during an UPDATE if there are multiple BEFORE UPDATE
triggers to avoid detoasting in each and every one of them. Such a tuple
will then passed to heap_update...


Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] Documentation/help for materialized and recursive views

[Magnus Hagander]

On Fri, Jun 28, 2013 at 4:49 PM, Alvaro Herrera
 wrote:
> Magnus Hagander escribió:
>
>> They are already crosslinked under "see also". But that doesn't really
>> help the guy doing "\h CREATE VIEW" in psql, which was the case where
>> it was brought to my attention.
>
> Maybe \h should somehow display the "see also" section?

I've been toying with the idea getting \h to show an actual http://
link to the reference page on the website, since most terminals lets
you deal with URLs easily lately. I haven't actually looked into how
feasible that would be though, but it would be interesting to check
out.  (With a toggle to turn it on/off of course)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


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

Re: [HACKERS] Documentation/help for materialized and recursive views

[Alvaro Herrera]

Magnus Hagander escribió:

> They are already crosslinked under "see also". But that doesn't really
> help the guy doing "\h CREATE VIEW" in psql, which was the case where
> it was brought to my attention.

Maybe \h should somehow display the "see also" section?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] changeset generation v5-01 - Patches & git tree

[Tom Lane]

Andres Freund  writes:
> On 2013-06-28 08:41:46 -0400, Robert Haas wrote:
>> The alternative I previously proposed was to make the WAL records
>> carry the relation OID.  There are a few problems with that: one is
>> that it's a waste of space when logical replication is turned off, and
>> it might not be easy to only do it when logical replication is on.
>> Also, even when logic replication is turned on, things that make WAL
>> bigger aren't wonderful.  On the other hand, it does avoid the
>> overhead of another index on pg_class.

> I personally favor making catalog modifications a bit more more
> expensive instead of increasing the WAL volume during routine
> operations.

This argument is nonsense, since it conveniently ignores the added WAL
entries created as a result of additional pg_class index manipulations.

Robert's idea sounds fairly reasonable to me; another 4 bytes per
insert/update/delete WAL entry isn't that big a deal, and it would
probably ease many debugging tasks as well as what you want to do.
So I'd vote for including the rel OID all the time, not conditionally.

The real performance argument against the patch as you have it is that
it saddles every PG installation with extra overhead for pg_class
updates whether or not that installation ever has or ever will make use
of changeset generation --- unlike including rel OIDs in WAL entries,
which might be merely difficult to handle conditionally, it's flat-out
impossible to turn such an index on or off.  Moreover, even if one is
using changeset generation, the overhead is being imposed at the wrong
place, ie the master not the slave doing changeset extraction.

But that's not the only problem, nor even the worst one IMO.  I said
before that a syscache with a non-unique key is broken by design, and
I stand by that estimate.  Even assuming that this usage doesn't create
bugs in the code as it stands, it might well foreclose future changes or
optimizations that we'd like to make in the catcache code.

If you don't want to change WAL contents, what I think you should do
is create a new cache mechanism (perhaps by extending the relmapper)
that caches relfilenode to OID lookups and acts entirely inside the
changeset-generating slave.  Hacking up the catcache instead of doing
that is an expedient kluge that will come back to bite us.

regards, tom lane


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

Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division

[David Fetter]

On Fri, Jun 28, 2013 at 10:31:16AM -0400, Tom Lane wrote:
> David Fetter  writes:
> > Please find attached the latest patch.
> 
> I remain of the opinion that this is simply a bad idea.  It is unlike
> our habits for constructing other types of nodes, and makes it harder
> not easier to find all the places that need to be updated when adding
> another field to FuncCall.

With utmost respect, this is just not true.

There's exactly one place that needs updating after adding another
field to FuncCall in the general case where the default value of the
field doesn't affect most setters of FuncCall, i.e. where the default
default is the right thing for current setters.  In specific cases
where such a field might need to be set to something other than its
default value, finding calls to makeFuncCall is just as easy, and with
some tools like cscope, even easier.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division

[Tom Lane]

David Fetter  writes:
> Please find attached the latest patch.

I remain of the opinion that this is simply a bad idea.  It is unlike
our habits for constructing other types of nodes, and makes it harder
not easier to find all the places that need to be updated when adding
another field to FuncCall.

regards, tom lane


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

Re: [HACKERS] changeset generation v5-01 - Patches & git tree

[Peter Eisentraut]

On 6/28/13 8:46 AM, Andres Freund wrote:
> I personally favor making catalog modifications a bit more more
> expensive instead of increasing the WAL volume during routine
> operations. I don't think index maintenance itself comes close to the
> biggest cost for DDL we have atm.

That makes sense to me in principle.


-- 
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] Department of Redundancy Department: makeNode(FuncCall) division

[David Fetter]

On Tue, Jun 25, 2013 at 02:54:55PM +0530, Jeevan Chalke wrote:
> On Tue, Jun 25, 2013 at 11:11 AM, Jeevan Chalke <
> jeevan.cha...@enterprisedb.com> wrote:
> 
> > Hi David,
> >
> > I hope this is the latest patch to review, right ?
> >
> > I am going to review it.
> >
> > I have gone through the discussion on this thread and I agree with Stephen
> > Frost that it don't add much improvements as such but definitely it is
> > going to be easy for contributors in this area as they don't need to go all
> > over to add any extra parameter they need to add. This patch simplifies it
> > well enough.
> >
> > Will post my review soon.
> >
> >
> Assuming *makeFuncArgs_002.diff* is the latest patch, here are my review
> points.
> 
> About this patch and feature:
> ===
> This patch tries to reduce redundancy when we need FuncCall expression. With
> this patch it will become easier to add new parameter if needed. We just
> need
> to put it's default value at centralized location (I mean in this new
> function)
> so that all other places need not require and changes. And this new
> parameter
> is handled by the new feature who demanded it keep other untouched.
> 
> Review comments on patch:
> ===
> 1. Can't apply with "git apply" command but able to do it with patch -p1.
> So no
> issues
> 2. Adds one whitespace error, hopefully it will get corrected once it goes
> through pgindent.
> 3. There is absolutely NO user visibility and thus I guess we don't need any
> test case. Also no documentation are needed.
> 4. Also make check went smooth and thus assumes that there is no issue as
> such.
> Even I couldn't find any issue with my testing other than regression suite.
> 5. I had a code walk-through over patch and it looks good to me. However,
> following line change seems unrelated (Line 186 in your patch)
> 
> ! $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "~~", $1,
> (Node *) n, @2);
> !
> 
> Changes required from author:
> ===
> It will be good if you remove unrelated changes from the patch and possibly
> all
> white-space errors.
> 
> Thanks

Thanks for the review!

Please find attached the latest patch.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index c487db9..245aef2 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -508,3 +508,28 @@ makeDefElemExtended(char *nameSpace, char *name, Node *arg,
 
return res;
 }
+
+/*
+ * makeFuncCall -
+ *
+ * Initialize a FuncCall struct with the information every caller must
+ * supply.  Any non-default parameters have to be handled by the
+ * caller.
+ *
+ */
+
+FuncCall *
+makeFuncCall(List *name, List *args, int location)
+{
+   FuncCall *n = makeNode(FuncCall);
+   n->funcname = name;
+   n->args = args;
+   n->location = location;
+   n->agg_order = NIL;
+   n->agg_star = FALSE;
+   n->agg_distinct = FALSE;
+   n->func_variadic = FALSE;
+   n->over = NULL;
+   return n;
+}
+
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5094226..24a585e 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10487,16 +10487,9 @@ a_expr:c_expr  
{ $$ = $1; }
}
| a_expr AT TIME ZONE a_expr%prec AT
{
-   FuncCall *n = makeNode(FuncCall);
-   n->funcname = 
SystemFuncName("timezone");
-   n->args = list_make2($5, $1);
-   n->agg_order = NIL;
-   n->agg_star = FALSE;
-   n->agg_distinct = FALSE;
-   n->func_variadic = FALSE;
-   n->over = NULL;
-   n->location = @2;
-   $$ = (Node *) n;
+   $$ = (Node *) 
makeFuncCall(SystemFuncName("timezone"),
+   
   list_make2($5, $1),
+   
   @2);
}
/*
 * These operators must be called out explicitly in order to 
make use
@@ -10548,113 +10541,65 @@ a_expr:  c_expr  
{ $$ = $1; }
   

Re: [HACKERS] extensible external toast tuple support

[Robert Haas]

On Fri, Jun 28, 2013 at 9:23 AM, Robert Haas  wrote:
> On Thu, Jun 27, 2013 at 12:56 PM, Andres Freund  
> wrote:
>> Please find attached the next version of the extensible toast
>> support. There basically are two changes:
>>
>> * handle indirect toast tuples properly in heap_tuple_fetch_attr
>>   and related places
>> * minor comment adjustments
>
> It looks to me like you need to pass true, rather than false, as the
> first argument to TrapMacro:
>
> +#define VARTAG_SIZE(tag) \
> +   ((tag) == VARTAG_INDIRECT ? sizeof(varatt_indirect) :   \
> +(tag) == VARTAG_ONDISK ? sizeof(varatt_external) : \
> +TrapMacro(false, "unknown vartag"))
>
> Still looking at the rest of this.

Why does toast_insert_or_update() need to go through all the
rigamarole in toast_datum_differs()?  I would have thought that it
could simply treat any external-indirect value as needing to be
detoasted and retoasted, since the destination is the disk anyhow.

Do you see external-indirect values getting used for anything other
than logical replication?  Is there code to do so anywhere?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] proposal: enable new error fields in plpgsql (9.4)

[Pavel Stehule]

Hello

2013/6/28 Noah Misch :
> On Fri, Jun 28, 2013 at 07:49:46AM +0200, Pavel Stehule wrote:
>> 2013/6/28 Noah Misch :
>> > On Tue, Jun 25, 2013 at 03:56:27PM +0200, Pavel Stehule wrote:
>> >> cleaned patch is in attachment
>> >
>> > Of the five options you're adding to GET STACKED DIAGNOSTICS, four of them
>> > appear in the SQL standard.  DATATYPE_NAME does not; I think we should 
>> > call it
>> > PG_DATATYPE_NAME in line with our other extensions in this area.
>> >
>>
>> yes, but It should be fixed in 9.3 enhanced fields too - it should be
>> consistent with PostgreSQL fields
>
> What else, specifically, should be renamed?  (Alternately, would you prepare a
> new version of the patch incorporating the proper name changes?)

I looked to source code, and identifiers in our source code are
consistent, so my comment hasn't sense. Yes, I agree, so only
identifier used in GET DIAGNOSTICS statement should be renamed. So,
only DATATYPE_NAME should be renamed to PG_DATATYPE_NAME.

Regards

Pavel

>
> Thanks,
> nm
>
> --
> Noah Misch
> EnterpriseDB http://www.enterprisedb.com


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

Re: [HACKERS] extensible external toast tuple support

[Robert Haas]

On Thu, Jun 27, 2013 at 12:56 PM, Andres Freund  wrote:
> Please find attached the next version of the extensible toast
> support. There basically are two changes:
>
> * handle indirect toast tuples properly in heap_tuple_fetch_attr
>   and related places
> * minor comment adjustments

It looks to me like you need to pass true, rather than false, as the
first argument to TrapMacro:

+#define VARTAG_SIZE(tag) \
+   ((tag) == VARTAG_INDIRECT ? sizeof(varatt_indirect) :   \
+(tag) == VARTAG_ONDISK ? sizeof(varatt_external) : \
+TrapMacro(false, "unknown vartag"))

Still looking at the rest of this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] proposal: enable new error fields in plpgsql (9.4)

[Noah Misch]

On Fri, Jun 28, 2013 at 07:49:46AM +0200, Pavel Stehule wrote:
> 2013/6/28 Noah Misch :
> > On Tue, Jun 25, 2013 at 03:56:27PM +0200, Pavel Stehule wrote:
> >> cleaned patch is in attachment
> >
> > Of the five options you're adding to GET STACKED DIAGNOSTICS, four of them
> > appear in the SQL standard.  DATATYPE_NAME does not; I think we should call 
> > it
> > PG_DATATYPE_NAME in line with our other extensions in this area.
> >
> 
> yes, but It should be fixed in 9.3 enhanced fields too - it should be
> consistent with PostgreSQL fields

What else, specifically, should be renamed?  (Alternately, would you prepare a
new version of the patch incorporating the proper name changes?)

Thanks,
nm

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


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

Re: [HACKERS] Move unused buffers to freelist

[Robert Haas]

On Fri, Jun 28, 2013 at 8:50 AM, Robert Haas  wrote:
> On Fri, Jun 28, 2013 at 12:52 AM, Amit Kapila  wrote:
>> Currently it wakes up based on bgwriterdelay config parameter which is by
>> default 200ms, so you means we should
>> think of waking up bgwriter based on allocations and number of elements left
>> in freelist?
>
> I think that's what Andres and I are proposing, yes.

Incidentally, I'm going to mark this patch Returned with Feedback in
the CF application.  I think this line of inquiry has potential, but
clearly there's a lot more work to do here before we commit anything,
and I don't think that's going to happen in the next few weeks.  But
let's keep discussing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Move unused buffers to freelist

[Robert Haas]

On Fri, Jun 28, 2013 at 12:52 AM, Amit Kapila  wrote:
> Currently it wakes up based on bgwriterdelay config parameter which is by
> default 200ms, so you means we should
> think of waking up bgwriter based on allocations and number of elements left
> in freelist?

I think that's what Andres and I are proposing, yes.

> As per my understanding Summarization of points raised by you and Andres
> which this patch should address to have a bigger win:
>
> 1. Bgwriter needs to be improved so that it can help in reducing usage count
> and finding next victim buffer
>(run the clock sweep and add buffers to the free list).

Check.

> 2. SetLatch for bgwriter (wakeup bgwriter) when elements in freelist are
> less.

Check.  The way to do this is to keep a variable in shared memory in
the same cache line as the spinlock protecting the freelist, and
update it when you update the free list.

> 3. Split the workdone globallock (Buffreelist) in StrategyGetBuffer
>(a spinlock for the freelist, and an lwlock for the clock sweep).

Check.

> 4. Separate processes for writing dirty buffers and moving buffers to
> freelist

I think this part might be best pushed to a separate patch, although I
agree we probably need it.

> 5. Bgwriter needs to be more aggressive, logic based on which it calculates
> how many buffers it needs to process needs to be improved.

This is basically overlapping with points already made.  I suspect we
could just get rid of bgwriter_delay, bgwriter_lru_maxpages, and
bgwriter_lru_multiplier altogether.  The background writer would just
have a high and a low watermark.  When the number of buffers on the
freelist drops below the low watermark, the allocating backend sets
the latch and bgwriter wakes up and begins adding buffers to the
freelist.  When the number of buffers on the free list reaches the
high watermark, the background writer goes back to sleep.  Some
experimentation might be needed to figure out what values are
appropriate for those watermarks.  In theory this could be a
configuration knob, but I suspect it's better to just make the system
tune it right automatically.

> 6. There can be contention around buffer mapping locks, but we can focus on
> it later
> 7. cacheline bouncing around the buffer header spinlocks, is there anything
> we can do to reduce this?

I think these are points that we should leave for the future.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] changeset generation v5-01 - Patches & git tree

[Andres Freund]

On 2013-06-27 21:52:03 -0700, Kevin Grittner wrote:
> Tried that, too, and problem persists.  The log shows the last
> commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb.

> I get the same failure, with primary key or unique index column
> showing as 0 in results.

I have run enough iterations of the test suite locally now that I am
confident it's not just happenstance that I don't see this :/. I am
going to clone your environment as closely as I can to see where the
issue might be as well as going over those codepaths...

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] Improvement of checkpoint IO scheduler for stable transaction responses

[KONDO Mitsumasa]

(2013/06/28 0:08), Robert Haas wrote:

On Tue, Jun 25, 2013 at 4:28 PM, Heikki Linnakangas
 wrote:
I'm pretty sure Greg Smith tried it the fixed-sleep thing before and
it didn't work that well.  I have also tried it and the resulting
behavior was unimpressive.  It makes checkpoints take a long time to
complete even when there's very little data to flush out to the OS,
which is annoying; and when things actually do get ugly, the sleeps
aren't long enough to matter.  See the timings Kondo-san posted
downthread: 100ms delays aren't going let the system recover in any
useful way when the fsync can take 13 s for one file.  On a system
that's badly weighed down by I/O, the fsync times are often
*extremely* long - 13 s is far from the worst you can see.  You have
to give the system a meaningful time to recover from that, allowing
other processes to make meaningful progress before you hit it again,
or system performance just goes down the tubes.  Greg's test, IIRC,
used 3 s sleeps rather than your proposal of 100 ms, but it still
wasn't enough.

Yes. In write phase, checkpointer writes numerous 8KB dirty pages in each
SyncOneBuffer(), therefore it can be well for tiny(100ms) sleep time. But
in fsync phase, checkpointer writes scores of relation files in each fsync(),
therefore it can not be well for tiny sleep. It shoud need longer sleep time
for recovery IO performance. If we know its best sleep time, we had better use 
previous fsync time. And if we want to prevent fast long fsync time, we had 
better change relation file size which is 1GB in default max size to smaller.


Go back to the subject. Here is our patches test results. Fsync + write patch was 
not good result in past result, so I retry benchmark in same condition. It seems 
to get good perfomance than past result.


* Performance result in DBT-2 (WH340)
   | TPS  90%tileAverage  Maximum
---+---
original_0.7   | 3474.62  18.348328  5.73936.977713
original_1.0   | 3469.03  18.637865  5.84241.754421
fsync  | 3525.03  13.872711  5.38228.062947
write  | 3465.96  19.653667  5.80440.664066
fsync + write  | 3586.85  14.459486  4.96027.266958
Heikki's patch | 3504.3   19.731743  5.76138.33814

* HTML result in DBT-2
http://pgstatsinfo.projects.pgfoundry.org/RESULT/

In attached text, I also describe in each checkpoint time. fsync patch was seemed 
to have longer time than not fsync patch. However, checkpoint schedule is on time 
in checkpoint_timeout and allowable time. I think that it is most important 
things in fsync phase that fast finished checkpoint is not but definitely and 
assurance write pages in end of checkpoint. So my fsync patch is not wrong 
working any more.


My write patch seems to have lot of riddle, so I try to investigate objective 
result and theory of effect.


Best regards,
--
Mitsumasa KONDO
NTT Open Source Software Center
* Performance result
   | TPS  90%tileAverage  Maximum  
---+---
original_0.7   | 3474.62  18.348328  5.73936.977713
original_1.0   | 3469.03  18.637865  5.84241.754421
fsync  | 3525.03  13.872711  5.38228.062947
write  | 3465.96  19.653667  5.80440.664066
fsync + write  | 3586.85  14.459486  4.96027.266958
Heikki's patch | 3504.3   19.731743  5.76138.33814 


* Checkpoint duration
# original_0.7
 instid |   start| flags | num_buffers | xlog_added | 
xlog_removed | xlog_recycled | write_duration | sync_duration | total_duration
++---+-++--+---++---+
 14 | 2013-06-19 15:15:24.658+09 | xlog  | 281 |  0 |   
 0 | 0 | 29.038 |  2.69 | 31.798
 14 | 2013-06-19 15:17:13.212+09 | xlog  | 177 |  0 |   
 0 |   300 |   17.9 | 0.886 | 18.818
 14 | 2013-06-19 15:18:45.525+09 | xlog  | 306 |  0 |   
 0 |   300 |  30.72 | 4.011 |  35.11
 14 | 2013-06-19 15:20:26.951+09 | xlog  | 215 |  0 |   
 0 |   300 | 21.952 | 2.148 | 24.197
 14 | 2013-06-19 15:21:56.425+09 | xlog  | 182 |  0 |   
 0 |   300 | 18.527 | 6.323 | 25.069
 14 | 2013-06-19 15:27:26.074+09 | xlog  |   15770 |  0 |   
 0 |   300 |335.431 |80.269 |420.405
 14 | 2013-06-19 15:42:26.272+09 | time  |   82306 |  0 |   
 0 |   300 | 209.34 |   119.159 |333.762
 14 | 2013-06-19 15:57:26.025+09 | time  |   88965 |  0 |   
 0 |   247 |211.095 |   125.846 |340.7

Re: [HACKERS] changeset generation v5-01 - Patches & git tree

[Andres Freund]

On 2013-06-28 08:41:46 -0400, Robert Haas wrote:
> On Fri, Jun 28, 2013 at 3:32 AM, Andres Freund  wrote:
> > What that means is that for every heap record in the target database in
> > the WAL we need to query pg_class to turn the relfilenode into a
> > pg_class.oid. So, we can easily replace syscache.c with some custom
> > caching code, but I don't think it's realistic to get rid of that
> > index. Otherwise we need to cache the entire pg_class in memory which
> > doesn't sound enticing.
> 
> The alternative I previously proposed was to make the WAL records
> carry the relation OID.  There are a few problems with that: one is
> that it's a waste of space when logical replication is turned off, and
> it might not be easy to only do it when logical replication is on.
> Also, even when logic replication is turned on, things that make WAL
> bigger aren't wonderful.  On the other hand, it does avoid the
> overhead of another index on pg_class.

I personally favor making catalog modifications a bit more more
expensive instead of increasing the WAL volume during routine
operations. I don't think index maintenance itself comes close to the
biggest cost for DDL we have atm.
It also increases the modifications needed to imporantant heap_*
functions which doesn't make me happy.

How do others see this tradeoff?

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
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] changeset generation v5-01 - Patches & git tree

[Robert Haas]

On Fri, Jun 28, 2013 at 3:32 AM, Andres Freund  wrote:
> What that means is that for every heap record in the target database in
> the WAL we need to query pg_class to turn the relfilenode into a
> pg_class.oid. So, we can easily replace syscache.c with some custom
> caching code, but I don't think it's realistic to get rid of that
> index. Otherwise we need to cache the entire pg_class in memory which
> doesn't sound enticing.

The alternative I previously proposed was to make the WAL records
carry the relation OID.  There are a few problems with that: one is
that it's a waste of space when logical replication is turned off, and
it might not be easy to only do it when logical replication is on.
Also, even when logic replication is turned on, things that make WAL
bigger aren't wonderful.  On the other hand, it does avoid the
overhead of another index on pg_class.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Group Commits Vs WAL Writes

[Atri Sharma]

>
> Yep.  To take a degenerate case, suppose that you had many small WAL
> records, say 64 bytes each, so more than 100 per 8K block.  If you
> flush those one by one, you're going to rewrite that block 100 times.
> If you flush them all at once, you write that block once.
>
> But even when the range is more than the minimum write size (8K for
> WAL), there are still wins.  Writing 16K or 24K or 32K submitted as a
> single request can likely be done in a single revolution of the disk
> head.  But if you write 8K and wait until it's done, and then write
> another 8K and wait until that's done, the second request may not
> arrive until after the disk head has passed the position where the
> second block needs to go.  Now you have to wait for the drive to spin
> back around to the right position.
>
> The details of course vary with the hardware in use, but there are
> very few I/O operations where batching smaller requests into larger
> chunks doesn't help to some degree.  Of course, the optimal transfer
> size does vary considerably based on the type of I/O and the specific
> hardware in use.

This makes a lot of sense. I was always under the impression that
batching small requests into larger requests bears the overhead of I/O
latency. But, it seems to be the other way round, which I have now
understood.

Thanks a ton,

Regards,

Atri


--
Regards,

Atri
l'apprenant


-- 
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] Group Commits Vs WAL Writes

[Atri Sharma]

>
> There is a spot on the disk to which the current WAL is destined to go.
> That spot on the disk is not going to be under the write-head for (say)
> another 6 milliseconds.
>
> Without commit_delay, I try to commit my record, but find that someone else
> is already on the lock (and on the fsync as well).  I have to wait for 6
> milliseconds before that person gets their commit done and releases the
> lock, then I can start mine, and have to wait another 8 milliseconds (7500
> rpm disk) for the spot to come around again, for a total of 14 milliseconds
> of latency.
>
> With commit_delay, I get my record in under the nose of the person who is
> already doing the delay, and they wake up and flush it for me in time to
> make the 6 millisecond cutoff.  Total 6 milliseconds latency for me.

Right. The example makes it very clear. Thanks for such a detailed explanation.

> One thing I tried a while ago (before the recent group-commit changes were
> made) was to record in shared memory when the last fsync finished, and then
> the next time someone needed to fsync, they would sleep until just before
> the write spot was predicted to be under the write head again
> (previous_finish + rotation_time - safety_margin, where rotation_time -
> safety_margin were represented by a single guc).  It worked pretty well on
> the system in which I wrote it, but seemed too brittle to be a general
> solution.

Can we look at researching a general formula for the above? It looks a
bit touchy, but could be made to work. Another option is to add a
probabilistic factor in the formula. Idk, it just seems to be a hunch
I have.

Regards,

Atri



--
Regards,

Atri
l'apprenant


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