Re: [HACKERS] [PATCH] Send catalog_xmin separately in hot standby feedback

[Craig Ringer]

On 5 September 2016 at 12:40, Craig Ringer  wrote:
> Hi all
>
> Currently hot standby feedback sends GetOldestXmin()'s result to the
> upstream as the required xmin. GetOldestXmin() returns a slot's
> catalog_xmin if that's the lowest xmin on the system.


Note that this patch changes the API to GetOldestXmin(), adding a new
boolean to allow it to disregard the catalog_xmin of slots.

Per Simon's feedback I'm going to split that out into a separate
patch, so will post a follow-up split one soon as the series.

-- 
 Craig Ringer   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] LSN as a recovery target

[Michael Paquier]

On Sun, Sep 4, 2016 at 11:30 PM, Simon Riggs  wrote:
> On 4 September 2016 at 14:32, Abhijit Menon-Sen  wrote:
>> At 2016-09-04 07:02:01 +0100, si...@2ndquadrant.com wrote:
>>>
>>> > By the way, what has been committed does not include the patch
>>> > adding the parsing context in case of an error as wanted upthread.
>>> > Perhaps that's not worth adding now as there is the GUC refactoring
>>> > potentially happening for the recovery parameters, so I don't mind
>>> > much. Just that's worth mentioning.
>>>
>>> Hmm, that was unintentional. If something stalls the recovery
>>> parameter project, please remind me to commit that as well.
>>
>> I'm aware of this, and will adjust accordingly in the GUC patch. Thanks
>> for the heads up.
>
> I noticed we don't mention what LSN is anywhere, so I'd like to apply
> the following doc patch also.

+1 for the idea. What do you think about adding a mention to the
system data type pg_lsn?

   
+   WAL records are appended to the WAL
+   logs as each new record is written. The insert position is described by
+   a Log Sequence Number (LSN) that is a byte offset into
+   the logs, increasing monotonically with each new record. Two
+   LSNs can be compared to calculate the volume of
+   WAL data that separates them, so are used to measure
+   progress of WAL during replication and recovery.
+  
Here we could for example append a sentence like "The system data type
pg_lsn is an internal
representation of the LSN".
-- 
Michael


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

Re: [HACKERS] Parallel build with MSVC

[Michael Paquier]

On Mon, Sep 5, 2016 at 9:18 AM, Noah Misch  wrote:
> On Wed, Apr 27, 2016 at 08:15:05AM +, Christian Ullrich wrote:
>> * From: Michael Paquier [mailto:michael.paqu...@gmail.com]
>>
>> > On Wed, Apr 27, 2016 at 4:06 PM, Christian Ullrich 
>> > wrote:
>>
>> > > * From: Michael Paquier [mailto:michael.paqu...@gmail.com]
>>
>> > >> vcbuild also supports /m. Wouldn't it make sense to have a environment
>> > >> variable flag for it as well? vcbuild has been replaced by msbuild in
>> > >> VS2010 but I would think that in back-branches this would have value
>> > >> for users still compiling with VS2008 or older, and those are still
>> > >> supported things.
>> > >
>> > > Good point, but I have no installation of 2008 around, so I cannot test
>> > > it. Perhaps there is someone around who could do that (just add the
>> > > $msbflags as the first argument to vcbuild)?
>> >
>> > I forgot that I have actually one! Bumping my Win7 VM CPU from 1 to 2,
>> > and using VS2008 command prompt with vcbuild /m I am not seeing
>> > issues. Moving symbols.out would be the main issue, but I am not
>> > noticing problems related to that.
>>
>> OK then, hopefully last round attached.
>
> Every vcbuild and msbuild invocation ought to recognize this variable, so
> please update the two places involving ecpg_regression.proj.  Apart from that,
> the patch looks good.

Good catch. I did not notice those during my lookups of those patches.
Not my intention to bump into Christian's feet, but here are updated
patches.

Actually, is that worth adding for clean.bat? I doubt that many people
would care if MSBFLAGS is not supported in it. The cleanup script is
not the bottleneck, the build script is. I added it in the patch 0001
attached but I doubt that's worth it to be honest.
-- 
Michael
From aaf028d2fab130d908a4efcd3c6316b4e2c3fdd7 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 5 Sep 2016 14:36:31 +0900
Subject: [PATCH 1/2] Support passing arbitrary arguments to MSBuild/VCBuild.

This is particularly useful to pass /m, to perform a parallel build.
---
 doc/src/sgml/install-windows.sgml | 11 ++-
 src/tools/msvc/build.pl   |  7 ---
 src/tools/msvc/clean.bat  |  2 +-
 src/tools/msvc/vcregress.pl   |  3 ++-
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 8cd189c..f656c66 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -145,6 +145,14 @@ $ENV{PATH}=$ENV{PATH} . ';c:\some\where\bison\bin';
 
  
 
+ 
+  To pass additional command line arguments to the Visual Studio build
+  command (msbuild or vcbuild):
+
+$ENV{MSBFLAGS}="/m";
+
+ 
+
  
   Requirements
   
@@ -401,7 +409,8 @@ $ENV{CONFIG}="Debug";
all generated files. You can also run it with the
dist parameter, in which case it will behave like
make distclean and remove the flex/bison output files
-   as well.
+   as well. Additional flags can be passed to this script for commands of
+   msbuild and vcbuild using the environment variable MSBFLAGS.
   
 
   
diff --git a/src/tools/msvc/build.pl b/src/tools/msvc/build.pl
index 007e3c7..5273977 100644
--- a/src/tools/msvc/build.pl
+++ b/src/tools/msvc/build.pl
@@ -38,6 +38,7 @@ my $vcver = Mkvcbuild::mkvcbuild($config);
 # check what sort of build we are doing
 
 my $bconf = $ENV{CONFIG} || "Release";
+my $msbflags  = $ENV{MSBFLAGS} || "";
 my $buildwhat = $ARGV[1] || "";
 if (uc($ARGV[0]) eq 'DEBUG')
 {
@@ -53,16 +54,16 @@ elsif (uc($ARGV[0]) ne "RELEASE")
 if ($buildwhat and $vcver >= 10.00)
 {
 	system(
-		"msbuild $buildwhat.vcxproj /verbosity:normal /p:Configuration=$bconf"
+		"msbuild $buildwhat.vcxproj $msbflags /verbosity:normal /p:Configuration=$bconf"
 	);
 }
 elsif ($buildwhat)
 {
-	system("vcbuild $buildwhat.vcproj $bconf");
+	system("vcbuild $msbflags $buildwhat.vcproj $bconf");
 }
 else
 {
-	system("msbuild pgsql.sln /verbosity:normal /p:Configuration=$bconf");
+	system("msbuild pgsql.sln $msbflags /verbosity:normal /p:Configuration=$bconf");
 }
 
 # report status
diff --git a/src/tools/msvc/clean.bat b/src/tools/msvc/clean.bat
index 469b8a2..e21e37f 100755
--- a/src/tools/msvc/clean.bat
+++ b/src/tools/msvc/clean.bat
@@ -107,6 +107,6 @@ REM for /r %%f in (*.sql) do if exist %%f.in del %%f
 cd %D%
 
 REM Clean up ecpg regression test files
-msbuild /NoLogo ecpg_regression.proj /t:clean /v:q
+msbuild %MSBFLAGS% /NoLogo ecpg_regression.proj /t:clean /v:q
 
 goto :eof
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index b4f9464..bcf2267 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -138,8 +138,9 @@ sub check
 
 sub ecpgcheck
 {
+	my $msbflags  = $ENV{MSBFLAGS} || "";
 	chdir $startdir;
-	system("msbuild ecpg_regression.proj /p:config=$Config");
+	system("msbuild ecpg_regression.proj $msbflags /p:config=$Config");
 	my $status = $? >> 8;
 	exit $status if $status;
 	InstallTemp();
-- 

[HACKERS] [PATCH] Send catalog_xmin separately in hot standby feedback

[Craig Ringer]

Hi all

Currently hot standby feedback sends GetOldestXmin()'s result to the
upstream as the required xmin. GetOldestXmin() returns a slot's
catalog_xmin if that's the lowest xmin on the system.

That's fine so long as we don't do logical decoding on standbys, but
if we start allowing logical slots on standbys it'll cause the master
to retain too much bloat since it'll pin the master's xmin (not
catalog_xmin) down based on the catalog_xmin of any slots on the
downstream.

To fix that, add new fields to the hot standby feedback protocol
message to carry a separate catalog_xmin.

This doesn't need any special care for backward compatibility because
the only thing that has any business sending hot standby feedback is a
physical standby and they're required to be the same major version as
the master. If someone tries to connect with a standby of the wrong
version they'll fail long before this, and even if they didn't they'd
just get an error saying there's not enough data in the message.
pg_basebackup, pg_recvlogical and pg_receivexlog don't send hot
standby feedback messages.

I'm posting this now because Petr was interested in it for his work on
logical replication. I'll be following it a subsequent patch to allow
logical slot creation on physical replicas if they're using a slot to
talk to the master and have hot_standby_feedback enabled, just so you
know the direction this is going in.

Passes 'make check', src/bin/pg_basebackup and src/test/recovery TAP
tests. I haven't added specific tests for this functionality since
there isn't (yet) a way to set catalog_xmin separately on a physical
standby without a dedicated test module.

The logical decoding timeline following patch[1] is also relevant for
this, since it is required for logical decoding on standby to survive
promotion.

Next steps will be:


* Expose information about whether or not a slot is in use from walreceiver.c

* Allow logical slots to be created on replicas if
hot_standby_feedback is enabled and a logical slot is in use. Return
null as the exported snapshot ID when creating over the walsender
protocol, since we can't export a snapshot on a standby due to the
need to allocate an xid. (That can be addressed separately).

* Now that recovery tests are possible, write the recovery test suite
for logical decoding on standby

* Auto-drop replication slots when dropping a database in dbase_redo

* Add a safety mechanism to stop users disabling hs feedback on the
replica or stopping using a physical slot to the upstream while
logical slots exist on the replica. Or mark such logical slots as
unusable using a new persistent field on the slot. Not trivial because
we must allow crash recovery without a slot to upstream (obviously),
and should preferably also allow fallback to archive recovery when
server with slot is temporarily unreachable. Must also consider
handling of physical slot with catalog_xmin set from cascading
physical replica with logical slots on it.

* Extend the logical replication patch to add support for following
physical failover using this functionality, likely in 11.0.

[1] https://commitfest.postgresql.org/10/779/


I'll add this to the next CF, but I realise the inability to test it
standalone may mean it can only be committed as part of a series along
with full support for logical decoding from standby.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 041682447587190e1d5b8ebea437f1bb33c33a84 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Wed, 13 Jul 2016 15:23:08 +0800
Subject: [PATCH] Send catalog_xmin in hot standby feedback

Add catalog_xmin to the to the hot standby feedback protocol so a read replica
that has logical slots can use its physical slot to the master to hold down the
master's catalog_xmin. This lets a replica prevent vacuuming of catalog tuples
still required by the replica's logical slots.

This functionality is required, but not sufficient, to allow logical decoding
to work on a replia. It is also useful for handling physical failover of
database instances that serve as upstreams for logical decoding clients.
---
 contrib/pg_visibility/pg_visibility.c |  4 +-
 contrib/pgstattuple/pgstatapprox.c|  2 +-
 doc/src/sgml/protocol.sgml| 33 ++--
 src/backend/access/transam/xlog.c | 36 -
 src/backend/catalog/index.c   |  2 +-
 src/backend/commands/analyze.c|  2 +-
 src/backend/commands/vacuum.c |  4 +-
 src/backend/replication/logical/logical.c | 20 +--
 src/backend/replication/walreceiver.c | 54 ---
 src/backend/replication/walsender.c   | 90 ++-
 src/backend/storage/ipc/procarray.c   | 51 +-
 src/include/access/xlog.h |  1 +
 src/include/replication/slot.h| 30 +--
 src/include/storage/procarray.h   |  2 +-
 14 files changed,

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[Victor Wagner]

On Mon, 5 Sep 2016 07:59:02 +0530
Mithun Cy  wrote:

> On Aug 31, 2016 1:44 PM, "Victor Wagner"  wrote:
> > Thanks, I've added this to 7-th (yet unpublished here) version of my
> > patch.
> Hi victor, just wanted know what your plan for your patch 07. Would
> you like to submit it to the community. I have just signed up as a
> reviewer for your patch.

As community shows an interest with this patch, and it has been
included in the current commitfest, I've to submit it.

This version of patch includes

1. Most important - a tap test suite (really small enough and has a
room for improvements).

2. Compatibility with older versions - now readonly check is skipped if
only one hostname is specified in the connect string

3. pg_host and pg_port variables now show host name and port library
was connected to.

4. Your fix with compilation of ecpg tests.

Patch is attached.



-- 
   Victor Wagner 
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index f22e3da..835061d 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -792,7 +792,7 @@ host=localhost port=5432 dbname=mydb connect_timeout=10

The general form for a connection URI is:
 
-postgresql://[user[:password]@][netloc][:port][/dbname][?param1=value1&...]
+postgresql://[user[:password]@][netloc][:port][,netloc[:port]...][/dbname][?param1=value1&...]
 

 
@@ -809,6 +809,7 @@ postgresql://localhost/mydb
 postgresql://user@localhost
 postgresql://user:secret@localhost
 postgresql://other@localhost/otherdb?connect_timeout=10&application_name=myapp
+postgresql://node1,node2:5433,node3:4432,node4/mydb?hostorder=random&readonly=1
 
 Components of the hierarchical part of the URI can also
 be given as parameters.  For example:
@@ -831,7 +832,9 @@ postgresql:///mydb?host=localhost&port=5433

 For improved compatibility with JDBC connection URIs,
 instances of parameter ssl=true are translated into
-sslmode=require.
+sslmode=require and
+loadBalanceHosts=true  into
+hostorder=random.

 

@@ -841,6 +844,10 @@ postgresql:///mydb?host=localhost&port=5433
 postgresql://[2001:db8::1234]/database
 

+   
+   There can be serveral host specifications, optionally accompanied
+   with port, separated by comma.
+   
 

 The host component is interpreted as described for the parameter PostgreSQL was built). On machines without
 Unix-domain sockets, the default is to connect to localhost.

+   
+   There can be more than one host parameter in
+   the connect string. In this case these hosts would be considered
+   alternate entries into same database and if connect to first one
+   fails, library would try to connect second etc. This can be used
+   for high availability cluster or for load balancing. See
+parameter.
+   
+   
+   Network host name can be accompanied with port number, separated by
+   colon. If so, this port number is used only when connected to
+   this host. If there is no port number, port specified in the
+parameter would be used.
+   
   
  
 
@@ -942,8 +963,44 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname


   
-
+
+  
+  hostorder
+  
+  
+  Specifies how to choose host from list of alternate hosts,
+  specified in the  parameter.
+  
+  
+  If value of this argument is sequential (the
+  default) library connects to the hosts in order they specified,
+  and tries to connect second one only if connection to the first
+  fails.
+  
+  
+  If value is random host to connect is randomly
+  picked from the list. It allows to balance load between several
+  cluster nodes. However, currently PostgreSQL doesn't support
+  multimaster clusters. So, without use of third-party products,
+  only read-only connections can take advantage from the
+  load-balancing. See 
+  
+  
+  
+  
+  readonly
+  
+  
+  If this parameter is 0 (the default), upon successful connection
+  library checks if host is in recovery state, and if it is so,
+  tries next host in the connect string. If this parameter is
+  non-zero, connection to warm standby nodes are considered
+  successful.
+  
+  
+  
   
+
port


@@ -985,7 +1042,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
  
   connect_timeout
   
@@ -996,7 +1052,27 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
   
  
-
+ 
+ falover_timeout
+ 
+ 
+ Maximum time to cycilically retry all the hosts in commect string.
+ (as decimal integer number of seconds). If not specified, then
+ hosts are tried just once.
+ 
+ 
+ If we have replicating cluster, and master node fails, it might
+ take some time to promote one of standby nodes

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Amit Kapila]

On Mon, Sep 5, 2016 at 3:18 AM, Tomas Vondra
 wrote:
> Hi,
>
> This thread started a year ago, different people contributed various
> patches, some of which already got committed. Can someone please post a
> summary of this thread, so that it's a bit more clear what needs
> review/testing, what are the main open questions and so on?
>

Okay, let me try to summarize this thread.  This thread started off to
ameliorate the CLOGControlLock contention with a patch to increase the
clog buffers to 128 (which got committed in 9.6).   Then the second
patch was developed to use Group mode to further reduce the
CLOGControlLock contention, latest version of which is upthread [1] (I
have checked that version still gets applied).  Then Andres suggested
to compare the Group lock mode approach with an alternative (more
granular) locking model approach for which he has posted patches
upthread [2].  There are three patches on that link, the patches of
interest are 0001-Improve-64bit-atomics-support and
0003-Use-a-much-more-granular-locking-model-for-the-clog-.  I have
checked that second one of those doesn't get applied, so I have
rebased it and attached it with this mail.  In the more granular
locking approach, actually, you can comment USE_CONTENT_LOCK to make
it use atomic operations (I could not compile it by disabling
USE_CONTENT_LOCK on my windows box, you can try by commenting that as
well, if it works for you).  So, in short we have to compare three
approaches here.

1) Group mode to reduce CLOGControlLock contention
2) Use granular locking model
3) Use atomic operations

For approach-1, you can use patch [1].  For approach-2, you can use
0001-Improve-64bit-atomics-support patch[2] and the patch attached
with this mail.  For approach-3, you can use
0001-Improve-64bit-atomics-support patch[2] and the patch attached
with this mail by commenting USE_CONTENT_LOCK.  If the third doesn't
work for you then for now we can compare approach-1 and approach-2.

I have done some testing of these patches for read-write pgbench
workload and doesn't find big difference.  Now the interesting test
case could be to use few sub-transactions (may be 4-8) for each
transaction as with that we can see more contention for
CLOGControlLock.

Few points to note for performance testing, one should use --unlogged
tables, else the WAL writing and WALWriteLock contention masks the
impact of this patch.  The impact of this patch is visible at
higher-client counts (say at 64~128).

> I'm interested in doing some tests on the hardware I have available, but
> I'm not willing spending my time untangling the discussion.
>

Thanks for showing the interest and let me know if something is still
un-clear or you need more information to proceed.


[1] - 
https://www.postgresql.org/message-id/CAA4eK1%2B8gQTyGSZLe1Rb7jeM1Beh4FqA4VNjtpZcmvwizDQ0hw%40mail.gmail.com
[2] - 
https://www.postgresql.org/message-id/20160330230914.GH13305%40awork2.anarazel.de

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


use-granular-locking-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] PassDownLimitBound for ForeignScan/CustomScan

[Kouhei Kaigai]

> On Mon, Aug 29, 2016 at 7:25 AM, Kouhei Kaigai  wrote:
> 
> 
>   Hello,
> 
>   The attached patch adds an optional callback to support special 
> optimization
>   if ForeignScan/CustomScan are located under the Limit node in plan-tree.
> 
>   Our sort node wisely switches the behavior when we can preliminary know
>   exact number of rows to be produced, because all the Sort node has to
>   return is the top-k rows when it is located under the Limit node.
>   It is much lightweight workloads than sorting of entire input rows when
>   nrows is not small.
> 
>   In my case, this information is very useful because GPU can complete its
>   sorting operations mostly on L1-grade memory if we can preliminary know
>   the top-k value is enough small and fits to size of the fast memory.
> 
>   Probably, it is also valuable for Fujita-san's case because this 
> information
>   allows to attach "LIMIT k" clause on the remote query of postgres_fdw.
>   It will reduce amount of the network traffic and remote CPU consumption
>   once we got support of sort pushdown.
> 
> 
> 
>   One thing we need to pay attention is cost estimation on the planner 
> stage.
>   In the existing code, only create_ordered_paths() and
> create_merge_append_path()
>   considers the limit clause for cost estimation of sorting. They use the
>   'limit_tuples' of PlannerInfo; we can reference the structure when 
> extension
>   adds ForeignPath/CustomPath, so I think we don't need a special 
> enhancement
>   on the planner stage.
>
Thanks for your comments.

> I believe this hook is gets called at execution time.
> So to push LIMIT clause like you said above we should use "limit_tuples" at 
> the time
> of planning and then use this hook to optimize at runtime, right?
>
Yes. For more correctness, a valid "limit_tuples" of PlannerInfo is set only 
when
LIMIT clause takes constant values; it is true for most of use case.
Then, the hook I added shall be called at execution time for more exact 
optimization.

If FDW/CSP cannot accept uncertain number of rows to generate on planning time,
it is not a duty to provide its own path which is optimized for small number of
LIMIT clause.

> Apart from that, attached patch applies cleanly on latest sources and found 
> no issues
> with make or with regressions.
> 
> However this patch is an infrastructure for any possible optimization when
> foreign/customscan is under LIMIT.
> 
> So look good to me.
> 
> I quickly tried adding a hook support in postgres_fdw, and it gets called 
> correctly
> when we have foreignscan with LIMIT (limit being evaluated on local server).
> 
> So code wise no issue. Also add this hook details in documentation.
>
OK, I'll try to write up some detailed documentation stuff; not only API 
specification.

Best regards,

> 
> Thanks
> 
> 
> 
>   Thanks,
>   --
>   NEC Business Creation Division / PG-Strom Project
>   KaiGai Kohei 
> 
> 
> 
>   --
>   Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>   To make changes to your subscription:
>   http://www.postgresql.org/mailpref/pgsql-hackers
> 
> 
> 
> 
> 
> 
> 
> --
> 
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> 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

[HACKERS] Re: [bug fix] Cascading standby cannot catch up and get stuck emitting the same message repeatedly

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,
> Our customer hit a problem of cascading replication, and we found the cause.
> They are using the latest PostgreSQL 9.2.18.  The bug seems to have been
> fixed in 9.4 and higher during the big modification of xlog.c, but it's
> not reflected in older releases.
> 
> The attached patch is for 9.2.18.  This just borrows the idea from 9.4 and
> higher.
> 
> But we haven't been able to reproduce the problem.  Could you review the
> patch and help to test it?  I would very much appreciate it if you could
> figure out how to reproduce the problem easily.

We could successfully reproduce the problem and confirm that the patch fixes 
it.  Please use the attached script to reproduce the problem.  Place it in an 
empty directory and just run "./test.sh" with no argument.  It creates three 
database clusters (primary, standby, and cascading standby) in the current 
directory.

Could you review the patch and commit it for the next release?  If you think I 
should register the patch with the CommitFest even if the problem occurs in 9.2 
and 9.3, please say so.  I'll do so if there's no comment.

Regards
Takayuki Tsunakawa




test.sh
Description: test.sh

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

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Michael Paquier]

On Mon, Sep 5, 2016 at 9:32 AM, Andreas Karlsson  wrote:
> On 09/05/2016 02:23 AM, Tom Lane wrote:
>>
>> Judging by the number of people who have popped up recently with their
>> own OpenSSL 1.1 patches, I think there is going to be a lot of demand for
>> back-patching some sort of 1.1 support into our back branches.  All this
>> talk of refactoring does not sound very back-patchable.  Should we be
>> thinking of what we can extract that is back-patchable?
>
> My idea is that the first of my four patches contains the minimum changes
> needed to add support for 1.1 and tries to do as little refactoring as
> possible while the other patches refactor things. I am not sure about if
> anything of the other patches should be backpatched.

>From what I can see of the 4 patches proposed, those are not that much
invasive, so a backpatch of those is really doable. But yes let's keep
the refactoring only for HEAD. That's definitely the safest approach.
-- 
Michael


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

Re: [HACKERS] Logical decoding slots can go backwards when used from SQL, docs are wrong

[Craig Ringer]

On 5 September 2016 at 10:41, Craig Ringer  wrote:
> On 2 September 2016 at 17:49, Craig Ringer  wrote:
>
>> So the main change becomes the one-liner in my prior mail.
>
> Per feedback from Simon, updated with a new test in src/test/recovery .

... attached this time.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From eb7e141195ddecd2e58b52167f83d83130909201 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Mon, 2 May 2016 19:50:22 +0800
Subject: [PATCH] Dirty replication slots when accessed via sql interface

When pg_logical_slot_get_changes(...) sets confirmed_flush_lsn to the point at
which replay stopped, it doesn't dirty the replication slot.  So if the replay
didn't cause restart_lsn or catalog_xmin to change as well, this change will
not get written out to disk. Even on a clean shutdown.

If Pg crashes or restarts, a subsequent pg_logical_slot_get_changes(...) call
will see the same changes already replayed since it uses the slot's
confirmed_flush_lsn as the start point for fetching changes. The caller can't
specify a start LSN when using the SQL interface.

Mark the slot as dirty after reading changes using the SQL interface so that
users won't see repeated changes after a clean shutdown. Repeated changes still
occur when using the walsender interface or after an unclean shutdown.
---
 src/backend/replication/logical/logicalfuncs.c | 15 ++
 src/test/recovery/Makefile |  2 ++
 src/test/recovery/t/006_logical_decoding.pl| 40 ++
 3 files changed, 57 insertions(+)
 create mode 100644 src/test/recovery/t/006_logical_decoding.pl

diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 4e4c8cd..9c7be2d 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -321,7 +321,22 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
 		 * business..)
 		 */
 		if (ctx->reader->EndRecPtr != InvalidXLogRecPtr && confirm)
+		{
 			LogicalConfirmReceivedLocation(ctx->reader->EndRecPtr);
+			/*
+			 * If only the confirmed_flush_lsn has changed the slot won't get
+			 * marked as dirty by the above. Callers on the walsender interface
+			 * are expected to keep track of their own progress and don't need
+			 * it written out. But SQL-interface users cannot specify their own
+			 * start positions and it's harder for them to keep track of their
+			 * progress, so we should make more of an effort to save it for them.
+			 *
+			 * Dirty the slot so it's written out at the next checkpoint. We'll
+			 * still lose its position on crash, as documented, but it's better
+			 * than always losing the position even on clean restart.
+			 */
+			ReplicationSlotMarkDirty();
+		}
 
 		/* free context, call shutdown callback */
 		FreeDecodingContext(ctx);
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index 9290719..a847952 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -18,3 +18,5 @@ check:
 
 clean distclean maintainer-clean:
 	rm -rf tmp_check
+
+EXTRA_INSTALL = contrib/test_decoding
diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
new file mode 100644
index 000..4a782b8
--- /dev/null
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -0,0 +1,40 @@
+# Testing of logical decoding using SQL interface and/or pg_recvlogical
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 2;
+
+# Initialize master node
+my $node_master = get_new_node('master');
+$node_master->init(allows_streaming => 1);
+$node_master->append_conf(
+		'postgresql.conf', qq(
+max_replication_slots = 4
+wal_level = logical
+));
+$node_master->start;
+my $backup_name = 'master_backup';
+
+$node_master->safe_psql('postgres', qq[CREATE TABLE decoding_test(x integer, y text);]);
+
+$node_master->safe_psql('postgres', qq[SELECT pg_create_logical_replication_slot('test_slot', 'test_decoding');]);
+
+$node_master->safe_psql('postgres', qq[INSERT INTO decoding_test(x,y) SELECT s, s::text FROM generate_series(1,10) s;]);
+
+# Basic decoding works
+my($result) = $node_master->safe_psql('postgres', qq[SELECT pg_logical_slot_get_changes('test_slot', NULL, NULL);]);
+is(scalar(split /^/m, $result), 12, 'Decoding produced 12 rows inc BEGIN/COMMIT');
+
+# If we immediately crash the server we might lose the progress we just made
+# and replay the same changes again. But a clean shutdown should never repeat
+# the same changes when we use the SQL decoding interface.
+$node_master->restart('fast');
+
+# There are no new writes, so the result should be empty.
+$result = $node_master->safe_psql('postgres', qq[SELECT pg_logical_slot_get_changes('test_slot', NULL, NULL);]);
+chomp($result);
+is($result, '', 'Decoding after fast restart repeats no rows rows');
+

Re: [HACKERS] Logical decoding slots can go backwards when used from SQL, docs are wrong

[Craig Ringer]

On 2 September 2016 at 17:49, Craig Ringer  wrote:

> So the main change becomes the one-liner in my prior mail.

Per feedback from Simon, updated with a new test in src/test/recovery .

If you revert the change to
src/backend/replication/logical/logicalfuncs.c then the test will
start failing.

I'd quite like to backpatch the fix since it's simple and safe, but I
don't feel that it's hugely important to do so. This is an annoyance
not a serious data risking bug.

My only concern here is that we still lose position on crash. So
SQL-interface callers should still be able to cope with it. But they
won't see it happen if they're only testing with normal shutdowns,
host reboots, etc. In practice users aren't likely to notice this
anyway, though, since most people don't restart the server all the
time. I think it's better than what we have.

This issue could be eliminated completely by calling
ReplicationSlotSave() after ReplicationSlotMarkDirty(). This would
force an immediate flush after a non-peeked SQL interface decoding
call. It means more fsync()ing, though, and the SQL interface can only
be used by polling so that could be a significant performance hit.
We'd want to only flush when the position changed, but even then it'll
mean a sync every time anything gets returned.

The better alternative is to add a variant on
pg_logical_slot_get_changes(...) etc that accepts a start LSN. But
it's not convenient or easy for SQL callers to extract the last commit
LSN received from the last call to pass it to the next one, so this
isn't simple, and is probably best tackled as part of the SQL
interface API change Petr and Andres discussed in this thread.

I'm inclined to just dirty the slot for now, then provide a better
solution by adding the peek/confirm interface discussed upthread down
the track.

Andres, Petr, got an opinion here?

-- 
 Craig Ringer   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] Patch: Implement failover on libpq connect level.

[Mithun Cy]

On Aug 31, 2016 1:44 PM, "Victor Wagner"  wrote:
> Thanks, I've added this to 7-th (yet unpublished here) version of my
> patch.
Hi victor, just wanted know what your plan for your patch 07. Would you
like to submit it to the community. I have just signed up as a reviewer for
your patch.

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Andres Freund]

On September 4, 2016 6:33:30 PM PDT, Tom Lane  wrote:
>Andres Freund  writes:
>> On 2016-09-04 21:09:41 -0400, Tom Lane wrote:
>>> Hm, couldn't we do that automatically?  At least in the case where
>only
>>> one base-version script is available?
>
>> You mean determining the baseversion? Hm, yes, that might work. But
>I'm
>> not sure how much we can rely on no earlier scripts being already
>> installed or such. Seems like a problem you'd possibly only encounter
>in
>> the field or dirty development environments.
>
>Actually, why would we care?  Pick one, with some tiebreaker rules
>(shortest update path, for starters). 

Fair point.

> I think nearly all of the
>infrastructure for this is already there in extension.c.

Yes, it doesn't sound very hard...

Andres

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


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

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Tom Lane]

Andres Freund  writes:
> On 2016-09-04 21:09:41 -0400, Tom Lane wrote:
>> Hm, couldn't we do that automatically?  At least in the case where only
>> one base-version script is available?

> You mean determining the baseversion? Hm, yes, that might work. But I'm
> not sure how much we can rely on no earlier scripts being already
> installed or such. Seems like a problem you'd possibly only encounter in
> the field or dirty development environments.

Actually, why would we care?  Pick one, with some tiebreaker rules
(shortest update path, for starters).  I think nearly all of the
infrastructure for this is already there in extension.c.

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] Remove superuser() checks from pgstattuple

[Andres Freund]

On 2016-09-04 21:09:41 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2016-09-04 11:55:06 -0400, Tom Lane wrote:
> >> It is becoming clear that the current extension update mechanism is kind
> >> of brute-force for this sort of change.  I have no ideas offhand about a
> >> better way to do it, but like Peter, I was dismayed by the amount of pure
> >> overhead involved in the PARALLEL SAFE updates.
> 
> > Agreed. I think one way, which a few extensions are taking, is to have a
> > base version and then incremental version upgrades. Currently CREATE
> > EXTENSION doesn't natively support that, so you have to concatenate the
> > upgrade scripts.  I think it'd be great if we could add a 'baseversion'
> > property to the extension control file. When you create a new extension,
> > it'll start with the base version and then use the existing code to find
> > a path to upgrade to the target version.   That also makes it a lot
> > easier to actually properly test extension upgrade paths, something
> > we've not really been good at.
> 
> Hm, couldn't we do that automatically?  At least in the case where only
> one base-version script is available?

You mean determining the baseversion? Hm, yes, that might work. But I'm
not sure how much we can rely on no earlier scripts being already
installed or such. Seems like a problem you'd possibly only encounter in
the field or dirty development environments.

Andres


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

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Tom Lane]

Andres Freund  writes:
> On 2016-09-04 11:55:06 -0400, Tom Lane wrote:
>> It is becoming clear that the current extension update mechanism is kind
>> of brute-force for this sort of change.  I have no ideas offhand about a
>> better way to do it, but like Peter, I was dismayed by the amount of pure
>> overhead involved in the PARALLEL SAFE updates.

> Agreed. I think one way, which a few extensions are taking, is to have a
> base version and then incremental version upgrades. Currently CREATE
> EXTENSION doesn't natively support that, so you have to concatenate the
> upgrade scripts.  I think it'd be great if we could add a 'baseversion'
> property to the extension control file. When you create a new extension,
> it'll start with the base version and then use the existing code to find
> a path to upgrade to the target version.   That also makes it a lot
> easier to actually properly test extension upgrade paths, something
> we've not really been good at.

Hm, couldn't we do that automatically?  At least in the case where only
one base-version script is available?

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] [PATCH] COPY vs \copy HINT

[Craig Ringer]

On 5 September 2016 at 09:05, Craig Ringer  wrote:

>I've attached an update that does so and
> warns on EACCES too.

... this time, with required parens.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From d4455da00411daae846eadbd7f7d9bae2fb61b49 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 12 Aug 2016 15:42:12 +0800
Subject: [PATCH] Emit a HINT when COPY can't find a file

Users often get confused between COPY and \copy and try to use client-side
paths with COPY. The server of course cannot find the file.

Emit a HINT in the most common cases to help users out.

Craig Ringer, review by Tom Lane and Christoph Berg
---
 src/backend/commands/copy.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 5947e72..8d5cb4f 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1751,6 +1751,7 @@ BeginCopyTo(Relation rel,
 		{
 			mode_t		oumask; /* Pre-existing umask value */
 			struct stat st;
+			int			save_errno;
 
 			/*
 			 * Prevent write to relative path ... too easy to shoot oneself in
@@ -1759,16 +1760,21 @@ BeginCopyTo(Relation rel,
 			if (!is_absolute_path(filename))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_NAME),
-	  errmsg("relative path not allowed for COPY to file")));
+		 errmsg("relative path not allowed for COPY to file")));
 
 			oumask = umask(S_IWGRP | S_IWOTH);
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_W);
+			save_errno = errno;
 			umask(oumask);
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for writing: %m",
-cstate->filename)));
+cstate->filename),
+		 ((save_errno == ENOENT || save_errno == EACCES) ?
+		  errhint("COPY copies to a file on the PostgreSQL server, not on the client. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), &st))
 ereport(ERROR,
@@ -2786,13 +2792,21 @@ BeginCopyFrom(Relation rel,
 		else
 		{
 			struct stat st;
+			int			save_errno;
 
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_R);
+
+			/* in case something in ereport changes errno: */
+			save_errno = errno;
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for reading: %m",
-cstate->filename)));
+cstate->filename),
+		 ((save_errno == ENOENT || save_errno == EACCES) ?
+		  errhint("COPY copies from a file on the PostgreSQL server, not on the client. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), &st))
 ereport(ERROR,
-- 
2.5.5


-- 
Sent 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] COPY vs \copy HINT

[Craig Ringer]

On 4 September 2016 at 23:33, Tom Lane  wrote:

> So my consciousness was raised just now by an example of exactly this
> scenario over in pgsql-novice.  What I forgot was that the client may
> in fact be on the same machine as the server, in which case EACCES
> is pretty much exactly what you'd expect.

Yep. Also in cases with common paths, like /root or whatever.

> So we probably do want to
> hint for that case, but the hint wording I previously suggested no
> longer seems like le mot juste ... it needs to cover the idea that
> the client and server are different processes on the same machine.

Yeah, may be. *We* know that in this case "client" and "server" still
applies since client and server can be on the same host, but the
people who needs this hint may not understand that. Though there's
only so much we can do or should try to do in a HINT.

I think the most important bit is pointing them at \copy, so it's still useful.

To cover the same-host case we could try something like:

   COPY runs on the PostgreSQL server, using the PostgreSQL server's
directories and permissions, it doesn't run on the client.

... but I think that's actually less helpful for the users who'll need this.

We could say "COPY runs as the PostgreSQL server" but that's the kind
of hint that mostly helps people who already understand it and don't
actually the hint.

(BTW, whoever came up with "EACCES" needs to go spend time with the
creat() system call somewhere dark and smelly).

> I don't suppose there's any easy way for COPY to distinguish local
> from remote connections

Not that I see, since "local" can be unix socket or tcp to localhost.
Not cleanly anyway.

I don't think it matters. Many hosts have enough paths in common that
in practice the hint on EACCES will be useful anyway. It'd be nice to
work in something about running with the permissions of the PostgreSQL
server, but I don't see a way to do that without making it all more
complex.

I don't think it's worth tons of time anyway. This will be better than
what we have, lets do it.


I'm fairly happy with the wording you came up with:

"COPY copies to a file on the PostgreSQL server, not on the client. "
"You may want a client-side facility such as psql's \\copy."

and am inclined to suggest going ahead with the existing wording. I
agree that removing the part for "relative path not allowed for COPY
to file" is reasonable, so I've attached an update that does so and
warns on EACCES too.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From a30266cdbf1febd033ad2ba127341e1da4677dae Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 12 Aug 2016 15:42:12 +0800
Subject: [PATCH] Emit a HINT when COPY can't find a file

Users often get confused between COPY and \copy and try to use client-side
paths with COPY. The server of course cannot find the file.

Emit a HINT in the most common cases to help users out.

Craig Ringer, review by Tom Lane and Christoph Berg
---
 src/backend/commands/copy.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 5947e72..341c058 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1751,6 +1751,7 @@ BeginCopyTo(Relation rel,
 		{
 			mode_t		oumask; /* Pre-existing umask value */
 			struct stat st;
+			int			save_errno;
 
 			/*
 			 * Prevent write to relative path ... too easy to shoot oneself in
@@ -1759,16 +1760,21 @@ BeginCopyTo(Relation rel,
 			if (!is_absolute_path(filename))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_NAME),
-	  errmsg("relative path not allowed for COPY to file")));
+		 errmsg("relative path not allowed for COPY to file")));
 
 			oumask = umask(S_IWGRP | S_IWOTH);
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_W);
+			save_errno = errno;
 			umask(oumask);
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for writing: %m",
-cstate->filename)));
+cstate->filename),
+		 (save_errno == ENOENT || save_errno == EACCES ?
+		  errhint("COPY copies to a file on the PostgreSQL server, not on the client. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), &st))
 ereport(ERROR,
@@ -2786,13 +2792,21 @@ BeginCopyFrom(Relation rel,
 		else
 		{
 			struct stat st;
+			int			save_errno;
 
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_R);
+
+			/* in case something in ereport changes errno: */
+			save_errno = errno;
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for reading: %m",
-cstate->filename)));
+cstate->filename),
+		 (save_errno == ENOENT || save_errno == EACCES ?
+			

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Andres Freund]

On 2016-09-04 11:55:06 -0400, Tom Lane wrote:
> [ warning, thread hijack ahead ]
> 
> Stephen Frost  writes:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> I think this is a good change to pursue, and we'll likely want to do
> >> more similar changes in contrib.  But I'm worried that what is logically
> >> a 10-line change will end up a 20 KiB patch every time.
> 
> > We also created a new version to add the PARALLEL SAFE markings to the
> > functions.  In general, I believe it's better to use a new version when
> > we're making these kinds of changes.
> 
> It is becoming clear that the current extension update mechanism is kind
> of brute-force for this sort of change.  I have no ideas offhand about a
> better way to do it, but like Peter, I was dismayed by the amount of pure
> overhead involved in the PARALLEL SAFE updates.

Agreed. I think one way, which a few extensions are taking, is to have a
base version and then incremental version upgrades. Currently CREATE
EXTENSION doesn't natively support that, so you have to concatenate the
upgrade scripts.  I think it'd be great if we could add a 'baseversion'
property to the extension control file. When you create a new extension,
it'll start with the base version and then use the existing code to find
a path to upgrade to the target version.   That also makes it a lot
easier to actually properly test extension upgrade paths, something
we've not really been good at.


> It's not only development overhead, either: users have to remember to
> run around and issue ALTER EXTENSION UPDATE for every extension they
> have, in every database they have it installed in.  Anyone want to
> lay a side bet on how many users will actually do that?  Given that
> the release notes don't currently suggest doing so, I'd be willing
> to put money on "none at all" :-(

Some certainly are, but I'm afraid that you're right that it's not too
many.  If we don't make pg_upgrade upgrade all extensions, we should at
least have it generate a script doing so.


Greetings,

Andres Freund


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

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Tom Lane]

Andres Freund  writes:
> On 2016-04-05 11:38:27 -0300, Alvaro Herrera wrote:
>> The current arrangement looks bizantine to me, for no reason.  If we
>> think that one of the two branches might do something additional to the
>> list deletion, surely that will be in a separate stanza with its own
>> comment; and if we ever want to remove the list deletion from one of the
>> two cases (something that strikes me as unlikely) then we will need to
>> fix the comment, too.

> You realize it's two different lists they're deleted in the different
> branches?

I looked at this and can see some of the argument on both sides, but
if it's setting off static-analyzer warnings for some people, that
seems like a sufficient reason to change it.  We certainly make more
significant changes than this in order to silence warnings.

I rewrote the comment a bit more and pushed it.

regards, tom lane


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

Re: [HACKERS] Parallel build with MSVC

[Noah Misch]

On Sun, Sep 04, 2016 at 08:26:12PM -0400, Tom Lane wrote:
> Noah Misch  writes:
> > I was tempted to back-patch this.  The risk to back branch users seems
> > negligible, and it would be convenient for me as a person who builds all
> > branches.  That reason is not good enough, so I plan not to back-patch.  I
> > feel like I might be missing a stronger reason to back-patch.
> 
> Hm, wouldn't it help reduce the cycle time for Windows buildfarm members?
> That might still not be adequate reason, but it's an advantage beyond
> time-saving for individual developers.

Yes; multi-core Windows buildfarm members could configure MSBFLAGS=/m to
finish more quickly.


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

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Andreas Karlsson]

On 09/05/2016 02:23 AM, Tom Lane wrote:

Judging by the number of people who have popped up recently with their
own OpenSSL 1.1 patches, I think there is going to be a lot of demand for
back-patching some sort of 1.1 support into our back branches.  All this
talk of refactoring does not sound very back-patchable.  Should we be
thinking of what we can extract that is back-patchable?


My idea is that the first of my four patches contains the minimum 
changes needed to add support for 1.1 and tries to do as little 
refactoring as possible while the other patches refactor things. I am 
not sure about if anything of the other patches should be backpatched.


Andreas


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

Re: [HACKERS] Parallel build with MSVC

[Tom Lane]

Noah Misch  writes:
> I was tempted to back-patch this.  The risk to back branch users seems
> negligible, and it would be convenient for me as a person who builds all
> branches.  That reason is not good enough, so I plan not to back-patch.  I
> feel like I might be missing a stronger reason to back-patch.

Hm, wouldn't it help reduce the cycle time for Windows buildfarm members?
That might still not be adequate reason, but it's an advantage beyond
time-saving for individual developers.

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] OpenSSL 1.1 breaks configure and more

[Tom Lane]

Andreas Karlsson  writes:
> On 08/30/2016 08:42 AM, Heikki Linnakangas wrote:
>> PS. I just remembered that I've wanted to refactor the pgcrypto calls
>> for symmetric encryption to use the newer EVP API for some time, and
>> even posted a patch for that
>> (https://www.postgresql.org/message-id/561274f1.1030...@iki.fi). I
>> dropped the ball back then, but I think I'll go ahead and do that now,
>> once we get these other OpenSSL changes in.

> Nice!

Judging by the number of people who have popped up recently with their
own OpenSSL 1.1 patches, I think there is going to be a lot of demand for
back-patching some sort of 1.1 support into our back branches.  All this
talk of refactoring does not sound very back-patchable.  Should we be
thinking of what we can extract that is back-patchable?

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] Parallel build with MSVC

[Noah Misch]

On Wed, Apr 27, 2016 at 08:15:05AM +, Christian Ullrich wrote:
> * From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> 
> > On Wed, Apr 27, 2016 at 4:06 PM, Christian Ullrich 
> > wrote:
> 
> > > * From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> 
> > >> vcbuild also supports /m. Wouldn't it make sense to have a environment
> > >> variable flag for it as well? vcbuild has been replaced by msbuild in
> > >> VS2010 but I would think that in back-branches this would have value
> > >> for users still compiling with VS2008 or older, and those are still
> > >> supported things.
> > >
> > > Good point, but I have no installation of 2008 around, so I cannot test
> > > it. Perhaps there is someone around who could do that (just add the
> > > $msbflags as the first argument to vcbuild)?
> > 
> > I forgot that I have actually one! Bumping my Win7 VM CPU from 1 to 2,
> > and using VS2008 command prompt with vcbuild /m I am not seeing
> > issues. Moving symbols.out would be the main issue, but I am not
> > noticing problems related to that.
> 
> OK then, hopefully last round attached.

Every vcbuild and msbuild invocation ought to recognize this variable, so
please update the two places involving ecpg_regression.proj.  Apart from that,
the patch looks good.

I was tempted to back-patch this.  The risk to back branch users seems
negligible, and it would be convenient for me as a person who builds all
branches.  That reason is not good enough, so I plan not to back-patch.  I
feel like I might be missing a stronger reason to back-patch.


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

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Andreas Karlsson]

On 08/30/2016 08:42 AM, Heikki Linnakangas wrote:

There's the ResourceOwner mechanism, see src/backend/utils/resowner/.
That would be the proper way to do this. Call
RegisterResourceReleaseCallback() when the context is allocated, and
have the callback free it. One pitfall to watch out for is that
RegisterResourceReleaseCallback() itself calls palloc(), and can error
out, so you have to do things in such an order that you don't leak in
that case either.

Want to take a stab at that?

Another approach is put each allocated context in a list or array in a
global variable, and to register a callback to be called at
end-of-(sub)transaction, which closes all the contexts. But the resource
owner mechanism is probably easier.

There's also PG_TRY-CATCH, that you could maybe use in the callers of
px_find_digest(), to make sure they call px_free_digest() even on error.
But that also seems difficult to use with the pgp_encrypt() pipeline.


Sure, I have attached a patch where I try to use it.


PS. I just remembered that I've wanted to refactor the pgcrypto calls
for symmetric encryption to use the newer EVP API for some time, and
even posted a patch for that
(https://www.postgresql.org/message-id/561274f1.1030...@iki.fi). I
dropped the ball back then, but I think I'll go ahead and do that now,
once we get these other OpenSSL changes in.


Nice!

Andreas
>From 0afeb5e075028339f28c2c2d7b407c1bcc85c4be Mon Sep 17 00:00:00 2001
From: Andreas Karlsson 
Date: Sun, 4 Sep 2016 18:44:49 +0200
Subject: [PATCH] Add callback for always freeing digest context

---
 contrib/pgcrypto/openssl.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/contrib/pgcrypto/openssl.c b/contrib/pgcrypto/openssl.c
index 6586d32..68a6956 100644
--- a/contrib/pgcrypto/openssl.c
+++ b/contrib/pgcrypto/openssl.c
@@ -32,6 +32,7 @@
 #include "postgres.h"
 
 #include "px.h"
+#include "utils/resowner.h"
 
 #include 
 #include 
@@ -98,16 +99,30 @@ digest_finish(PX_MD *h, uint8 *dst)
 }
 
 static void
+digest_ctx_free_callback(ResourceReleasePhase phase,
+		 bool isCommit,
+		 bool isTopLevel,
+		 void *arg)
+{
+	OSSLDigest *digest = arg;
+
+	EVP_MD_CTX_destroy(digest->ctx);
+	UnregisterResourceReleaseCallback(digest_ctx_free_callback, digest);
+}
+
+static void
 digest_free(PX_MD *h)
 {
 	OSSLDigest *digest = (OSSLDigest *) h->p.ptr;
 
 	EVP_MD_CTX_destroy(digest->ctx);
+	UnregisterResourceReleaseCallback(digest_ctx_free_callback, digest);
 
 	px_free(digest);
 	px_free(h);
 }
 
+
 static int	px_openssl_initialized = 0;
 
 /* PUBLIC functions */
@@ -131,6 +146,9 @@ px_find_digest(const char *name, PX_MD **res)
 
 	digest = px_alloc(sizeof(*digest));
 	digest->algo = md;
+	digest->ctx = NULL;
+
+	RegisterResourceReleaseCallback(digest_ctx_free_callback, digest);
 
 	digest->ctx = EVP_MD_CTX_create();
 	if (digest->ctx == NULL)
@@ -139,6 +157,7 @@ px_find_digest(const char *name, PX_MD **res)
 	EVP_MD_CTX_init(digest->ctx);
 	if (EVP_DigestInit_ex(digest->ctx, digest->algo, NULL) == 0) {
 		EVP_MD_CTX_destroy(digest->ctx);
+		UnregisterResourceReleaseCallback(digest_ctx_free_callback, digest);
 		return -1;
 	}
 
-- 
2.9.3


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

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> [ warning, thread hijack ahead ]

quite.

> Stephen Frost  writes:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> I think this is a good change to pursue, and we'll likely want to do
> >> more similar changes in contrib.  But I'm worried that what is logically
> >> a 10-line change will end up a 20 KiB patch every time.
> 
> > We also created a new version to add the PARALLEL SAFE markings to the
> > functions.  In general, I believe it's better to use a new version when
> > we're making these kinds of changes.
> 
> It is becoming clear that the current extension update mechanism is kind
> of brute-force for this sort of change.  I have no ideas offhand about a
> better way to do it, but like Peter, I was dismayed by the amount of pure
> overhead involved in the PARALLEL SAFE updates.

To make the patches smaller, it seems that we'd need a way to avoid the
removal and re-addition of the entire install script and a way to avoid
having to add a new upgrade script.

Perhaps if the versioned install script was actually a non-versioned
install script in the source tree, and the Makefile simply installed it
into the correct place, then we could eliminate that part.  (All very
hand-wavy, I've not looked at what it'd take to do.)

I don't have any great answers for the upgrade script off-hand.  We
could come up with a way for one file to handle multiple upgrade
options, but that doesn't really make the patch any smaller.

> It's not only development overhead, either: users have to remember to
> run around and issue ALTER EXTENSION UPDATE for every extension they
> have, in every database they have it installed in.  Anyone want to
> lay a side bet on how many users will actually do that?  Given that
> the release notes don't currently suggest doing so, I'd be willing
> to put money on "none at all" :-(

I agree, that's also an issue.

> I wonder whether pg_upgrade ought to be changed to attempt upgrading
> every extension after it's completed the basic migration.  Or at
> least offer a script containing all the needed commands.

I like the idea of having an option and documentation to go along with
it.  Hopefully that will help administrators at least realize that it's
something they'll want to look at doing.

> Anyway, it's not this particular patch's job to do better, but we
> oughta think about better ways to do it.

Agreed.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Change error code for hstore syntax error

[Marko Tiikkaja]

Hi Sherrylyn,

On 2016-05-09 19:42, Sherrylyn Branchaw wrote:

I'm attaching a revised patch; please let me know if there are any other
issues before I submit to the commitfest.


I think this is mostly good, but these two should be changed:

  errmsg("unexpected end of string: \"%s\"", state->begin)
  errmsg("syntax error at position %d: \"%s\"", ...)

Right now, aside from the error code, these two look like they're 
reporting about an error in the SQL statement itself, and not in an 
input value for a type.  I think they should look more like this:


  errmsg("invalid input syntax for type hstore: \"%s\"", string),
  errdetail("Unexpected end of input.")

If possible, it might also make sense to provide more information than 
"unexpected end of string".  For example: what character were you 
expecting to find, or what were you scanning?  I didn't look too closely 
what exactly could be done here.  I'll leave that part to you.



.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] \timing interval

[Jim Nasby]

On 9/3/16 2:35 PM, Tom Lane wrote:

I pushed the patch using this:

Time: 176460001.200 ms (2 d 01:01:00.001)

and all else as before.


I'd find this useful in the final output of EXPLAIN ANALYZE as well; any 
objections to adding it?

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

[HACKERS] Minor fix to comments

[Jim Nasby]

I noticed some imbalanced '-'s in execnodes.h. Though, ISTM newer code 
doesn't use -'s in comments anymore, so maybe it'd be better to just 
ditch them?

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index a4ea1b9..e28477d 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1791,7 +1791,7 @@ typedef struct SortState
 
 /* -
  * GroupState information
- * -
+ * -
  */
 typedef struct GroupState
 {
@@ -1810,7 +1810,7 @@ typedef struct GroupState
  * input group during evaluation of an Agg node's output tuple(s).  We
  * create a second ExprContext, tmpcontext, in which to evaluate input
  * expressions and run the aggregate transition functions.
- * -
+ * -
  */
 /* these structs are private in nodeAgg.c: */
 typedef struct AggStatePerAggData *AggStatePerAgg;

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

Re: [HACKERS] pg_sequence catalog

[Greg Stark]

On Wed, Aug 31, 2016 at 3:01 PM, Tom Lane  wrote:
>
> Uh, not as subtly as all that, because "select * from sequence" will
> now return a different set of columns, which will flat out break a
> lot of clients that use that method to get sequence properties.

So? Clients expect changes like this between major releases surely.
Subtle changes that cause silent breakage for end-users seems scarier
than unsubtle breakage that tool authors can fix.

On Wed, Aug 31, 2016 at 7:30 PM, Andres Freund  wrote:
> Isn't that *precisely* when it's going to matter? If you have 5 active
> tables & sequences where the latter previously used independent locks,
> and they'd now be contending on a single lock.  If you have hundreds of
> thousands of active sequences, I doubt individual page locks would
> become a point of contention.

I think even two sequences could be a point of contention if you, for
example, are using COPY to load data into two otherwise completely
independent tables in two separate processes.

But that just means the row needs to be padded out to a cache line,
no? Or was the concern about things like trying to pin the same page,
parse the same page header, follow nearby line pointers...? I'm not
sure how effective all that caching is today but it doesn't seem
impossible to imagine getting that all optimized away.

-- 
greg


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

Re: [HACKERS] Obsolete TODO item "-Wcast-align" ?

[Christian Convey]

On Sun, Sep 4, 2016 at 5:56 PM, Tom Lane  wrote:
> Christian Convey  writes:
>> I chose this item from the TODO page: "[E] Remove warnings created by
>> -Wcast-align".  It didn't have a check-mark after the "[E]", which I
>> took to mean it's an outstanding issue.
>> However, I'm starting to wonder if it's already been fixed:
>
> No, but you only see it on some platforms/compilers.  On my OS X laptop
> (clang-based not gcc-based compiler), turning that on generates just a
> hair short of 13000 warnings :-(
>
> I think that TODO item is indeed obsolete, but more in the direction
> of "we're never gonna do that".  There are too many places where we
> do need to cast up from generic pointer to specific structure pointer,
> and there doesn't seem to be any practical way to tell a compiler which
> instances are useful to warn about.

Thanks for the response.  I'm unclear about how the TODO list is
curated.  Is there someone whose attention I should direct to this
thread?

- Christian


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

Re: [HACKERS] Obsolete TODO item "-Wcast-align" ?

[Tom Lane]

Christian Convey  writes:
> I chose this item from the TODO page: "[E] Remove warnings created by
> -Wcast-align".  It didn't have a check-mark after the "[E]", which I
> took to mean it's an outstanding issue.
> However, I'm starting to wonder if it's already been fixed:

No, but you only see it on some platforms/compilers.  On my OS X laptop
(clang-based not gcc-based compiler), turning that on generates just a
hair short of 13000 warnings :-(

I think that TODO item is indeed obsolete, but more in the direction
of "we're never gonna do that".  There are too many places where we
do need to cast up from generic pointer to specific structure pointer,
and there doesn't seem to be any practical way to tell a compiler which
instances are useful to warn about.

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] Speed up Clog Access by increasing CLOG buffers

[Tomas Vondra]

Hi,

This thread started a year ago, different people contributed various
patches, some of which already got committed. Can someone please post a
summary of this thread, so that it's a bit more clear what needs
review/testing, what are the main open questions and so on?

I'm interested in doing some tests on the hardware I have available, but
I'm not willing spending my time untangling the discussion.

thanks

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Obsolete TODO item "-Wcast-align" ?

[Christian Convey]

Hi guys,

I'm trying to get my feet wet in PG development, but I ran into a snag
with the TODO item I picked.  Could someone please tell me if I'm
making an obvious error?

I chose this item from the TODO page: "[E] Remove warnings created by
-Wcast-align".  It didn't have a check-mark after the "[E]", which I
took to mean it's an outstanding issue.

However, I'm starting to wonder if it's already been fixed:

(1) I wasn't able to reproduce it.  I built PG using:
 * PG source commit 6591f4...  (a recent master/HEAD)
 * Ubuntu's patched version of GCC 4.8.4
 * "-Wcast-align"
but I didn't see anything in the compiler output indicating that the
warning got triggered.

(2) It looks like someone else tried to tackle this TODO item about
two years ago, but he also was unable to reproduce it:
https://www.postgresql.org/message-id/flat/CALgh3eseTDT9RspDi0AfhE0j0A3e%2B5XiS3ykQyhK7Z5RgPQTLA%40mail.gmail.com#calgh3esetdt9rspdi0afhe0j0a3e+5xis3ykqyhk7z5rgpq...@mail.gmail.com

I didn't find any other discussions about that line-item via Google,
or by looking at the official/unofficial TODO-detail pages.

Thanks very much,
Christian Convey


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

Re: [HACKERS] Better locale-specific-character-class handling for regexps

[Tom Lane]

I wrote:
> I got tired of hearing complaints about the issue described in
> this thread:
> https://www.postgresql.org/message-id/flat/24241.1329347196%40sss.pgh.pa.us
> Here's a proposed fix.  I've not done extensive performance testing,
> but it seems to be as fast or faster than the old code in cases where
> there are not too many "large" characters in the input.  And, more
> to the point, it gets the right answer for such large characters.

I've now done some performance testing on this patch.  I ran the
attached scripts in cassert-off builds, using en_US.utf8 locale on
a RHEL6 box.  Each number is the best of 3 runs; all times in ms.

The first two test cases are meant to exercise regex compile speed; they
provide relatively short input strings, and each call provides a slightly
different pattern so as to defeat the compiled-pattern cache in
adt/regexp.c.

HEADpatched

compilespeed.sql61959.793   32936.773
compilespeed09.sql  6411.9575903.211

compilespeed.sql exercises compiling '\w' which of course is locale
dependent, while compilespeed09.sql compiles '[0-9]' which is not.
Since the patch sets MAX_SIMPLE_CHR to 0x7FF which is the same as
regc_pg_locale.c's old cutoff for how many characters to examine,
we must be doing about the same number of compile-time iswalnum()
probes in both old and new code.  The compile speedup must come entirely
from the fact that the new code uses a simple array for the low-char-codes
colormap, whereas the old code has a complicated multilevel colormap that
is more expensive to update.  Then the bigger win for compilespeed.sql
must be due to the fact that \w will require changing the colors of more
characters in the colormap.  I was not expecting to see such a big win,
but I'll take it ;-)

The remaining cases hold the pattern fixed (so it's not recompiled
each time) and are meant to measure the per-character scan speed
while executing a compiled regexp.

HEADpatched ratio of per-char times

runspeedlo.sql  12951.588   12053.533   0.91
runspeedhi.sql  10410.140 * 36678.876   2.72 (estimated)
runspeednotlo.sql   12999.629   12130.439   0.91
runspeednothi.sql   15678.125   20053.861   1.36
dummy.sql   3444.6963437.984(to measure overhead)

* gives wrong answer

In the runspeedhi.sql case, HEAD thinks the first character doesn't match
the pattern, so it's not scanning the whole input, which makes that
measurement not useful.  We can assume though that the scan rate would
have been about the same as runspeednothi.sql, and the estimated per-char
ratio shown in the table is computed on that basis.

So the patched code's scan speed is about 9% better than HEAD for data
characters below MAX_SIMPLE_CHR, presumably because of the simpler color
map structure.  But it's noticeably worse for data chars above that, and
much worse if the pattern forces runtime iswalnum() tests.  That's what
I was expecting.

It's worth noticing that comparing runspeednotlo.sql and runspeednothi.sql
shows that HEAD is 28% worse per-character on the latter.  Those cases are
exactly the same so far as HEAD's regex engine is concerned, so I think
that that differential must be blamable entirely on the fact that we're
converting 2-byte UTF8 sequences into pg_wchars in the first case and
3-byte UTF8 sequences in the second case.  The patched code is also paying
that cost of course, so you should take it into account when comparing
numbers in different rows of this table.  And it's also an indication
that really, this code is pretty frickin fast --- the total time per
character is only circa 4x the extra logic to pick up and shift in
one more byte!

Overall I'm pretty happy with these results.  I think that most cases
will be as fast or faster than before, assuming that most data characters
are below MAX_SIMPLE_CHR for most use-cases.  In the worst cases, the
per-character cost can be several times what it was before, but those
are exactly the same cases where *we gave the wrong answer* before, so
it's hard to complain too much.

regards, tom lane

\timing on
do $$
begin
  for i in 1..10 loop
perform 'aqaqaqaqaqaqaqaqaqaqaqaqaq' ~ ('\w\w\w\w\w\w\w\w\w\w\w\w\w\w' || 
i);
  end loop;
end$$;
\timing on
do $$
begin
  for i in 1..10 loop
perform 'a' ~ 
('[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]' || i);
  end loop;
end$$;
\timing on
do $$
declare
  txt text := repeat(chr(2037), 1000);
  pat text := '^\w*$';
begin
  raise notice 'result of txt ~ pat = %', txt ~ pat;
  for i in 1..100 loop
perform txt ~ pat;
  end loop;
end$$;
\timing on
do $$
declare
  txt text := repeat(chr(2310), 1000);
  pat text := '^\w*$';
begin
  raise notice 'result of txt ~ pat = %', txt ~ pat;
  for i in 1..100 loop
perform txt ~ pat;
  end loop;
end$$;
\timing on
do $$
de

Re: [HACKERS] [PATCH] Alter or rename enum value

[Emre Hasegeli]

> I started looking at this patch.  I'm kind of unhappy with having *both*
> IF EXISTS and IF NOT EXISTS options on the statement, especially since
> the locations of those phrases in the syntax seem to have been chosen
> with a dartboard.  This feels way more confusing than it is useful.
> Is there really a strong use-case for either option?  I note that
> ALTER TABLE RENAME COLUMN, which is probably used a thousand times
> more often than this will be, has so far not grown either kind of option,
> which sure makes me think that this proposal is getting ahead of itself.

I think they can be useful.  I am writing a lot of migration scripts
for small projects.  It really helps to be able to run parts of them
again.  ALTER TYPE ... ADD VALUE already have IF NOT EXISTS option.  I
don't think we would lose anything to support both of them in here.

The syntax ALTER TYPE ... RENAME VALUE [ IF EXISTS ] ... TO ... [ IF
NOT EXISTS ] looks self-explaining to me.  I haven't confused when I
first saw.  IF EXISTS applying to the old value, IF NOT EXISTS
applying to the new value, are the only reasonable semantics one might
expect from renaming things.  Anybody is interpreting it wrong? or can
think of another syntax?


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

Re: [HACKERS] Better locale-specific-character-class handling for regexps

[Tom Lane]

Heikki Linnakangas  writes:
> On 08/23/2016 03:54 AM, Tom Lane wrote:
>> ! the color map for characters above MAX_SIMPLE_CHR is really a 2-D array,
>> ! whose rows correspond to character ranges that are explicitly mentioned
>> ! in the input, and whose columns correspond to sets of relevant locale
>> ! character classes.

> I think that last sentence should say "whose rows correspond to 
> character ranges that are explicitly mentioned in the *regex pattern*", 
> rather than "in the input".

OK.  The pattern is the input under consideration by regcomp.c, but I see
your point that someone might be confused.

> An example would be very helpful here.

I'll see what I can do.

> I can easily come up with examples for 
> character classes, but not for "high" character-ranges. The best I could 
> come up with is to check if a characters belongs to some special group 
> of unicode characters, like U&'[\+01D100-\+01D1FF]' to check for musical 
> symbol characters.

Right, or someone might write the equivalent of [A-Z] in the Klingon
alphabet, or whatever --- it wouldn't necessarily have to use backslash
notation, it could be literal characters with large codes.

> In practice, I guess you will only see single 
> characters in the colormaprange array, although we must of course cope 
> with ranges too.

I suspect that's true; I'm not sure that there are many places in the
upper reaches of Unicode space where ranges of consecutive character
codes form natural units to search for.

>> +   /* this relies on WHITE being zero: */
>> +   memset(cm->locolormap, WHITE,
>> +  (MAX_SIMPLE_CHR - CHR_MIN + 1) * sizeof(color));

> Is the comment correct? I don't see why this wouldn't work with "WHITE 
> != 0".

The array entries are shorts not chars, so if WHITE were say 2, memset
would fill the array with 0x0202 rather than 2.  I suppose if WHITE were
say 0x0303 then it would accidentally work, but I don't think the comment
needs to get into that.

> Perhaps "backwards" would be clearer than "downwards"?

OK.

> +1 for this patch in general. Some regression test cases would be nice.

I'm not sure how to write such tests without introducing insurmountable
platform dependencies --- particularly on platforms with weak support for
UTF8 locales, such as OS X.  All the interesting cases require knowing
what iswalpha() etc will return for some high character codes.

What I did to test it during development was to set MAX_SIMPLE_CHR to
something in the ASCII range, so that the high-character-code paths could
be tested without making any assumptions about locale classifications for
non-ASCII characters.  I'm not sure that's a helpful idea for regression
testing purposes, though.

I guess I could follow the lead of collate.linux.utf8.sql and produce
a test that's only promised to pass on one platform with one encoding,
but I'm not terribly excited by that.  AFAIK that test file does not
get run at all in the buildfarm or in the wild.

Thanks for reviewing!

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: Transactional enum additions - was Re: [HACKERS] Alter or rename enum value

[Tom Lane]

Emre Hasegeli  writes:
>> +   /*
>> +* If the row is hinted as committed, it's surely safe.  This provides a
>> +* fast path for all normal use-cases.
>> +*/
>> +   if (HeapTupleHeaderXminCommitted(enumval_tup->t_data))
>> +   return;
>> +
>> +   /*
>> +* Usually, a row would get hinted as committed when it's read or loaded
>> +* into syscache; but just in case not, let's check the xmin directly.
>> +*/
>> +   xmin = HeapTupleHeaderGetXmin(enumval_tup->t_data);
>> +   if (!TransactionIdIsInProgress(xmin) &&
>> +   TransactionIdDidCommit(xmin))
>> +   return;

> This looks like transaction internal logic inside enum.c to me.  Maybe
> it is because I am not much familiar with the internals.  I couldn't
> find a similar pattern anywhere else, but it might still be useful to
> invent something like HeapTupleHeaderXminReallyCommitted().

I wondered about that too, but there are no other roughly similar cases
that I could find, and abstracting from a single use-case is perilous ---
especially when there's no compelling reason to think there will ever be
any other similar use-cases.  I'd originally wondered whether we could
replace this logic with a call to something in tqual.c, but none of the
available functions there produce the required behavior either.

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] Make better use of existing enums in plpgsql

[Pavel Stehule]

Hi

2016-08-19 16:56 GMT+02:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> plpgsql.h defines a number of enums, but most of the code passes them
> around as ints.  The attached patch updates structs and function
> prototypes to take enum types instead.  This clarifies the struct
> definitions in plpgsql.h in particular.
>

I am sending review of this trivial patch.

1. No problems with patching, compiling
2. all regress tests passed
3. There are not any visible change, so tests, docs are not necessary
4. Using enum instead int is generally good idea

I will mark this patch as ready for commiters.


>
> I didn't deal with the PLPGSQL_RC_* symbols, since they are only used in
> pl_exec.c (could be moved there?), and it would bloat this patch.
>

It is good idea. PLPGSQL_RC_* should be private in pl_exec.c

Regards

Pavel


>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, 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] Remove superuser() checks from pgstattuple

[Tom Lane]

[ warning, thread hijack ahead ]

Stephen Frost  writes:
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> I think this is a good change to pursue, and we'll likely want to do
>> more similar changes in contrib.  But I'm worried that what is logically
>> a 10-line change will end up a 20 KiB patch every time.

> We also created a new version to add the PARALLEL SAFE markings to the
> functions.  In general, I believe it's better to use a new version when
> we're making these kinds of changes.

It is becoming clear that the current extension update mechanism is kind
of brute-force for this sort of change.  I have no ideas offhand about a
better way to do it, but like Peter, I was dismayed by the amount of pure
overhead involved in the PARALLEL SAFE updates.

It's not only development overhead, either: users have to remember to
run around and issue ALTER EXTENSION UPDATE for every extension they
have, in every database they have it installed in.  Anyone want to
lay a side bet on how many users will actually do that?  Given that
the release notes don't currently suggest doing so, I'd be willing
to put money on "none at all" :-(

I wonder whether pg_upgrade ought to be changed to attempt upgrading
every extension after it's completed the basic migration.  Or at
least offer a script containing all the needed commands.

Anyway, it's not this particular patch's job to do better, but we
oughta think about better ways to do it.

regards, tom lane


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

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Stephen Frost]

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 8/23/16 5:22 PM, Stephen Frost wrote:
> > Now that we track initial privileges on extension objects and changes to
> > those permissions, we can drop the superuser() checks from the various
> > functions which are part of the pgstattuple extension.
> > 
> > Since a pg_upgrade will preserve the version of the extension which
> > existed prior to the upgrade, we can't simply modify the existing
> > functions but instead need to create new functions which remove the
> > checks and update the SQL-level functions to use the new functions
> 
> I think this is a good change to pursue, and we'll likely want to do
> more similar changes in contrib.  But I'm worried that what is logically
> a 10-line change will end up a 20 KiB patch every time.

This is primairly due to how we handle new versions of an extension.
Any change to an extension is going to involve a new upgrade script and
the removal of the prior version install script and addition of the new
version install scripts.

> Have we explored other options for addressing the upgrade problems?

We did discuss the upgrade issue and Noah proposed the current approach,
which appears to be the best option.

> Maybe the function could check that non-default privileges have been
> granted?

Simply changing the function to behave differently depending on what
privileges have or havn't been granted doesn't seem like a very good
idea.

Consider an existing installation where the admin tried to grant access
to one of these functions:

GRANT EXECUTE ON pgstattuple_func() TO bob;

This would result in a GRANT to bob explicitly, and the GRANT to public
would still be there also.

Then an upgrade of PG, without upgrading the extension, would lead to
any user being able to execute the function.  An upgrade of the
extension would revoke the GRANT to PUBLIC and, further, would
hopefuflly cause the admin to consider checking the documentation about
the upgrade (which needs to be added; I'll do that).

We also created a new version to add the PARALLEL SAFE markings to the
functions.  In general, I believe it's better to use a new version when
we're making these kinds of changes.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Tom Lane]

I wrote:
> Craig Ringer  writes:
>> I thought about that but figured it didn't really matter too much,
>> when thinking about examples like
>> # COPY batch_demo FROM '/root/secret.csv' WITH (FORMAT CSV);
>> ERROR:  could not open file "/root/secret.csv" for reading: Permission denied
>> or whatever, where the user doesn't understand why they can't read the
>> file given that their local client has permission to do so.

> Meh.  Hinting in this case could be helpful only if the user in fact had
> identical directory structures on client and server and a data file in the
> right place on both.

So my consciousness was raised just now by an example of exactly this
scenario over in pgsql-novice.  What I forgot was that the client may
in fact be on the same machine as the server, in which case EACCES
is pretty much exactly what you'd expect.  So we probably do want to
hint for that case, but the hint wording I previously suggested no
longer seems like le mot juste ... it needs to cover the idea that
the client and server are different processes on the same machine.

I don't suppose there's any easy way for COPY to distinguish local
from remote connections --- if it could, that might influence both
the errnos to hint for and the phrasing of the hint.

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] Add support for restrictive RLS policies

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost  wrote:
> > As outlined in the commit message, this adds support for restrictive RLS
> > policies.  We've had this in the backend since 9.5, but they were only
> > available via hooks and therefore extensions.  This adds support for
> > them to be configured through regular DDL commands.  These policies are,
> > essentially "AND"d instead of "OR"d.
> >
> > Includes updates to the catalog, grammer, psql, pg_dump, and regression
> > tests.  Documentation will be added soon, but until then, would be great
> > to get feedback on the grammer, catalog and code changes.
> 
> I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
> for one thing.  I think putting the word RESTRICT, or maybe AS
> RESTRICT, somewhere later in the command would be better.

I had been notionally thinking RESTRICTIVE, but ended up just using
RESTRICT since it was already an unreserved keyword.  Of course, that's
not a good reason.

> I also think that it is very strange to have the grammar keyword be
> "restrict" but the internal flag be called "permissive".  It would be
> better to have the sense of those flags match.

Permissive is the default and should just be added to the grammar, so
users can be explicit, if they wish to.

* Thom Brown (t...@linux.com) wrote:
> On 1 September 2016 at 10:02, Robert Haas  wrote:
> > I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
> > for one thing.  I think putting the word RESTRICT, or maybe AS
> > RESTRICT, somewhere later in the command would be better.
> >
> > I also think that it is very strange to have the grammar keyword be
> > "restrict" but the internal flag be called "permissive".  It would be
> > better to have the sense of those flags match.
> >
> > (This is not intended as a full review, just a quick comment.)
> 
> I had proposed this sort of functionality a couple years back:
> https://www.depesz.com/2014/10/02/waiting-for-9-5-row-level-security-policies-rls/#comment-187800
> 
> And I suggested CREATE RESTRICTIVE POLICY, but looking back at that,
> perhaps you're right, and it would be better to add it later in the
> command.

Ah, I had recalled seeing something along those lines somewhere, but
didn't know where, thanks.

Based on Robert's suggestion and using Thom's verbiage, I've tested this
out:

CREATE POLICY pol ON tab AS [PERMISSIVE|RESTRICTIVE] ...

and it appears to work fine with the grammar, etc.

Unless there's other thoughts on this, I'll update the patch to reflect
this grammar in a couple days.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

[Andreas Karlsson]

On 08/31/2016 11:34 PM, Peter Geoghegan wrote:

On Sun, Nov 22, 2015 at 7:29 PM, Andreas Karlsson  wrote:

Sorry for dropping this patch, but now I have started looking at it again.


Any chance of picking this up again soon, Andreas? I think it's an
important project. I would like to review it.


I do not really have any good ideas for how to fix it for Windows, but 
if anyone would like to discuss solutions I am interested in working on 
this patch again.


The alternatives as I see them now:

1) Serialize the certificates, key, and CRL and write them to the 
backend_var temp file and then deserialize everything in the backends.


Sounds like you would need to write some code for every SSL library to 
support the serialization and deserialization, which I am not a fan of 
doing just for one platform since I worry about little used code paths. 
Additionally this would mean that we write a copy of the private key to 
potentially another file system than the one where the private key is 
stored, this sounds like a bad idea from a security point of view.


2) Copy all the SSL related files into the data directory at SIGHUP, 
before loading them. While this does not require any serialization of 
certificates it still has the problem of writing private keys to disk.


3) Leave my patch as it is now. This means the postmaster will reload 
certificates on SIGHUP while the backends will also load them when 
spawning. This means windows will continue to work the same as before my 
patch.


Is there any other way to pass the current set of loaded certificates 
and keys from the postmaster to the backends on Windows? I guess you 
could use a pipe, but if so we should probably send all data on this 
pipe, not just the SSL stuff.


I am leaning towards doing (3) but I know I am biased since it is less 
work and I do not care much for Windows.


Andreas


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

Re: [HACKERS] LSN as a recovery target

[Simon Riggs]

On 4 September 2016 at 14:32, Abhijit Menon-Sen  wrote:
> At 2016-09-04 07:02:01 +0100, si...@2ndquadrant.com wrote:
>>
>> > By the way, what has been committed does not include the patch
>> > adding the parsing context in case of an error as wanted upthread.
>> > Perhaps that's not worth adding now as there is the GUC refactoring
>> > potentially happening for the recovery parameters, so I don't mind
>> > much. Just that's worth mentioning.
>>
>> Hmm, that was unintentional. If something stalls the recovery
>> parameter project, please remind me to commit that as well.
>
> I'm aware of this, and will adjust accordingly in the GUC patch. Thanks
> for the heads up.

I noticed we don't mention what LSN is anywhere, so I'd like to apply
the following doc patch also.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


doc_lsn.v1.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] Better locale-specific-character-class handling for regexps

[Heikki Linnakangas]

On 08/23/2016 03:54 AM, Tom Lane wrote:

! That's still not quite enough, though, because of locale-dependent
! character classes such as [[:alpha:]].  In Unicode locales these classes
! may have thousands of entries that are above MAX_SIMPLE_CHR, and we
! certainly don't want to be searching large colormaprange arrays at runtime.
! Nor do we even want to spend the time to initialize cvec structures that
! exhaustively describe all of those characters.  Our solution is to compute
! exact per-character colors at compile time only up to MAX_SIMPLE_CHR.
! For characters above that, we apply the  or  lookup
! functions at runtime for each locale-dependent character class used in the
! regex pattern, constructing a bitmap that describes which classes the
! runtime character belongs to.  The per-character-range data structure
! mentioned above actually holds, for each range, a separate color entry
! for each possible combination of character class properties.  That is,
! the color map for characters above MAX_SIMPLE_CHR is really a 2-D array,
! whose rows correspond to character ranges that are explicitly mentioned
! in the input, and whose columns correspond to sets of relevant locale
! character classes.


I think that last sentence should say "whose rows correspond to 
character ranges that are explicitly mentioned in the *regex pattern*", 
rather than "in the input".


An example would be very helpful here. I had to read through this many 
times, until I understood it. I can easily come up with examples for 
character classes, but not for "high" character-ranges. The best I could 
come up with is to check if a characters belongs to some special group 
of unicode characters, like U&'[\+01D100-\+01D1FF]' to check for musical 
symbol characters. In practice, I guess you will only see single 
characters in the colormaprange array, although we must of course cope 
with ranges too.



+   /* this relies on WHITE being zero: */
+   memset(cm->locolormap, WHITE,
+  (MAX_SIMPLE_CHR - CHR_MIN + 1) * sizeof(color));
+
+   memset(cm->classbits, 0, sizeof(cm->classbits));
+   cm->numcmranges = 0;
+   cm->cmranges = NULL;
+   cm->maxarrayrows = 4;   /* arbitrary initial allocation */
+   cm->hiarrayrows = 1;
+   cm->hiarraycols = 1;
+   cm->hicolormap = (color *) MALLOC(cm->maxarrayrows * sizeof(color));
+   if (cm->hicolormap == NULL)
+   {
+   CERR(REG_ESPACE);
+   return;
+   }
+   /* initialize the "all other characters" row to WHITE */
+   cm->hicolormap[0] = WHITE;


Is the comment correct? I don't see why this wouldn't work with "WHITE 
!= 0".



!   /* Duplicate existing columns to the right, and increase ref counts */
!   /* Must work downwards in the array because we realloc'd in place */
!   for (r = cm->hiarrayrows - 1; r >= 0; r--)
!   {
!   color  *oldrowptr = &newarray[r * cm->hiarraycols];
!   color  *newrowptr = &newarray[r * cm->hiarraycols * 2];
!   color  *newrowptr2 = newrowptr + cm->hiarraycols;

!   for (c = 0; c < cm->hiarraycols; c++)
!   {
!   color   co = oldrowptr[c];
!
!   newrowptr[c] = newrowptr2[c] = co;
!   cm->cd[co].nuchrs++;
!   }
!   }


Perhaps "backwards" would be clearer than "downwards"? At least in my 
mental model, index 0 is the top row of an array, so "downwards" means 
0, 1, 2. I guess you meant downwards numerically, rather than visually, 
but it took me a moment to process that.


+1 for this patch in general. Some regression test cases would be nice.

- Heikki



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

Re: [HACKERS] LSN as a recovery target

[Abhijit Menon-Sen]

At 2016-09-04 07:02:01 +0100, si...@2ndquadrant.com wrote:
>
> > By the way, what has been committed does not include the patch
> > adding the parsing context in case of an error as wanted upthread.
> > Perhaps that's not worth adding now as there is the GUC refactoring
> > potentially happening for the recovery parameters, so I don't mind
> > much. Just that's worth mentioning.
> 
> Hmm, that was unintentional. If something stalls the recovery
> parameter project, please remind me to commit that as well.

I'm aware of this, and will adjust accordingly in the GUC patch. Thanks
for the heads up.

-- Abhijit


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

Re: [HACKERS] pg_hba_file_settings view patch

[Christoph Berg]

Re: Simon Riggs 2016-09-03 

> pg_hba_file_settings seems a clumsy name. I'd prefer pg_hba_settings,
> since that name could live longer than the concept of pg_hba.conf,
> which seems likely to become part of ALTER SYSTEM in future, so we
> wouldn't really want the word "file" in there.

IMHO "settings" is wrong here. "pg_file_settings" means "settings in
config file (that might not been applied yet)". The contents of
pg_hba.conf are not config settings, but there doesn't appear to be a
standard name for them - 19.1 calls them "records".

Given that the patch seems to report what's on disk, "pg_hba_file"
seems a good name to me. Even if ALTER SYSTEM should become able to
update the file, it'd still be a file. (If it were the actual running
config, I'd go for "pg_hba_rules".)

Christoph


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

Re: [HACKERS] [sqlsmith] Failed assertion in numeric aggregate

[Heikki Linnakangas]

On 09/03/2016 08:59 PM, Tom Lane wrote:

Andreas Seltenreich  writes:

Digging in the coredumps, it looks like set_var_from_num() is invoked on
an uninitialized NumericVar.  Sample gdb session below.


Hm, yeah, looks like numeric_poly_deserialize is missing a required
init_var() step.  Slightly astonishing that this got through even
minimal testing.

For the archives' sake: I could not reproduce this at default settings,
because the aggregate calls don't get parallelized.  But it fails fairly
quickly with
set parallel_tuple_cost = 0;
set parallel_setup_cost = 0;
set min_parallel_relation_size = 0;

Thanks for the report!


Thanks for fixing this so quickly. For the record, I did test 
parallelization at one point, but maybe it was with an older patch 
version or I screwed up something in the tests - otherwise I clearly 
would've hit that.


- Heikki



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

Re: Transactional enum additions - was Re: [HACKERS] Alter or rename enum value

[Emre Hasegeli]

> Got around to looking at this.  Attached is a revised version that I think
> is in committable shape.  The main non-cosmetic change is that the test
> for "type was created in same transaction as new value" now consists of
> comparing the xmins of the pg_type and pg_enum rows, without consulting
> GetCurrentTransactionId().  I did not like the original coding because
> it would pointlessly disallow references to enum values that were created
> in a parent transaction of the current subxact.  This way is still leaving
> some subxact use-cases on the table, as noted in the code comments, but
> it's more flexible than before.

Thank you for picking this up.  The patch looks good to me.  I think
this is a useful to support adding values to the enum created in the
same transaction.

> +   /*
> +* If the row is hinted as committed, it's surely safe.  This provides a
> +* fast path for all normal use-cases.
> +*/
> +   if (HeapTupleHeaderXminCommitted(enumval_tup->t_data))
> +   return;
> +
> +   /*
> +* Usually, a row would get hinted as committed when it's read or loaded
> +* into syscache; but just in case not, let's check the xmin directly.
> +*/
> +   xmin = HeapTupleHeaderGetXmin(enumval_tup->t_data);
> +   if (!TransactionIdIsInProgress(xmin) &&
> +   TransactionIdDidCommit(xmin))
> +   return;

This looks like transaction internal logic inside enum.c to me.  Maybe
it is because I am not much familiar with the internals.  I couldn't
find a similar pattern anywhere else, but it might still be useful to
invent something like HeapTupleHeaderXminReallyCommitted().

One risk about this feature is that the future enum functions would
not check if the value is safe to return.  Maybe we should append a
warning to the file header about this.


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

Re: [HACKERS] IF (NOT) EXISTS in psql-completion

[Pavel Stehule]

Hi

This patch needs rebase.

Regards

Pavel

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

minor update - using DefElem instead own private parser type

Regards

Pavel
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5148095..189d201 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10099,6 +10099,47 @@ SELECT xmlroot(xmlparse(document 'abc'),
 

 
+   
+xmltable
+
+   
+xmltable
+   
+
+
+xmltable(xmlnamespaces(namespace uri AS namespace name|DEFAULT namespace uri , ...) rowexpr PASSING BY REF xml BY REF COLUMNS name type PATH columnexpr DEFAULT expr NOT NULL|NULL , ...)
+
+
+
+  The xmltable produces table based on passed XML value.
+
+
+
+
+
+   
+

 xmlagg
 
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 743e7d6..7abc367 100644
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -189,6 +189,9 @@ static Datum ExecEvalCurrentOfExpr(ExprState *exprstate, ExprContext *econtext,
 static Datum ExecEvalGroupingFuncExpr(GroupingFuncExprState *gstate,
 		 ExprContext *econtext,
 		 bool *isNull, ExprDoneCond *isDone);
+static Datum ExecEvalTableExpr(TableExprState *tstate,
+		ExprContext *econtext,
+		bool *isnull, ExprDoneCond *isDone);
 
 
 /* 
@@ -4500,6 +4503,218 @@ ExecEvalCurrentOfExpr(ExprState *exprstate, ExprContext *econtext,
 	return 0;	/* keep compiler quiet */
 }
 
+/* 
+ *		ExecEvalTableExpr
+ *
+ * 
+ */
+
+#define XMLTABLE_DEFAULT_NAMESPACE			"pgdefxmlns"
+
+static Datum
+execEvalTableExpr(TableExprState *tstate,
+		ExprContext *econtext,
+		bool *isNull, ExprDoneCond *isDone)
+{
+	TupleDesc tupdesc;
+	HeapTuple		tuple;
+	HeapTupleHeader		result;
+	int	i;
+	Datum		*values;
+	bool		*nulls;
+	Datum	value;
+	bool	isnull;
+	xmltype		   *xmlval;
+	text		   *row_path;
+
+	tupdesc = tstate->tupdesc;
+
+	if (tstate->firstRow)
+	{
+		ListCell	*ns;
+
+		/* Evaluate expression first */
+		value = ExecEvalExpr(tstate->expr, econtext, &isnull, NULL);
+		if (isnull)
+		{
+			*isDone = ExprSingleResult;
+			*isNull = true;
+			return (Datum) 0;
+		}
+		xmlval = DatumGetXmlP(value);
+
+		value = ExecEvalExpr(tstate->row_path_expr, econtext, &isnull, NULL);
+		if (isnull)
+ereport(ERROR,
+		(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
+	  errmsg("row query must not be null")));
+		row_path = DatumGetTextP(value);
+
+		Assert(tstate->xmltableCxt == NULL);
+		tstate->xmltableCxt = initXmlTableContext(xmlval,
+	tstate->used_dns ?
+		XMLTABLE_DEFAULT_NAMESPACE : NULL,
+	tstate->ncols,
+			  tstate->in_functions,
+			  tstate->typioparams,
+	econtext->ecxt_per_query_memory);
+
+		foreach(ns, tstate->namespaces)
+		{
+			Node	   *n = (Node *) lfirst(ns);
+			ExprState  *expr;
+			char	   *ns_name;
+			char	   *ns_uri;
+
+			if (IsA(n, NamedArgExpr))
+			{
+NamedArgExpr *na = (NamedArgExpr *) n;
+
+expr = (ExprState *) na->arg;
+ns_name = na->name;
+			}
+			else
+			{
+expr = (ExprState *) n;
+ns_name = NULL;
+			}
+
+			value = ExecEvalExpr((ExprState *) expr, econtext, &isnull, NULL);
+			if (isnull)
+ereport(ERROR,
+		(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
+	  errmsg("namespace uri must not be null")));
+			ns_uri = text_to_cstring(DatumGetTextP(value));
+
+			XmlTableSetNs(tstate->xmltableCxt, ns_name, ns_uri);
+		}
+
+		XmlTableSetRowPath(tstate->xmltableCxt, row_path);
+
+		/* Path caclulation */
+		for (i = 0; i < tstate->ncols; i++)
+		{
+			char *col_path;
+
+			if (tstate->col_path_expr[i] != NULL)
+			{
+value = ExecEvalExpr(tstate->col_path_expr[i], econtext, &isnull, NULL);
+if (isnull)
+	ereport(ERROR,
+			(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
+		  errmsg("column path for column \"%s\" must not be null",
+			  NameStr(tupdesc->attrs[i]->attname;
+col_path = text_to_cstring(DatumGetTextP(value));
+			}
+			else
+col_path = NameStr(tupdesc->attrs[i]->attname);
+
+			XmlTableSetColumnPath(tstate->xmltableCxt, i,
+			tupdesc->attrs[i]->atttypid, col_path);
+		}
+		tstate->firstRow = false;
+	}
+
+	values = tstate->values;
+	nulls = tstate->nulls;
+
+	if (XmlTableFetchRow(tstate->xmltableCxt))
+	{
+		if (tstate->ncols > 0)
+		{
+			for (i = 0; i < tstate->ncols; i++)
+			{
+if (i != tstate->for_ordinality_col - 1)
+{
+	values[i] = XmlTableGetValue(tstate->xmltableCxt, i,
+  tupdesc->attrs[i]->atttypid,
+  tupdesc->attrs[i]->atttypmod,
+			  &isnull);
+	if (isnull && tstate->def_expr[i] != NULL)
+		values[i] = ExecEvalExpr(tstate->def_expr[i], econtext, &isnull, NULL);
+
+	if (isnull && tstate->not_null[i])
+		ereport(ERROR,
+(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
+ errmsg("null is not