Re: Unmatched test and comment in partition_join.sql regression test

[Etsuro Fujita]

On Fri, Dec 13, 2019 at 7:17 PM Etsuro Fujita  wrote:
> I noticed this in the regression test while polishing the PWJ-enhancement 
> patch:
>
> -- partitionwise join can not be applied for a join between list and range
> -- partitioned tables
> EXPLAIN (COSTS OFF)
> SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_n t1 FULL JOIN prt1 t2 ON (t1.c = 
> t2.c);
>
> The test doesn't match the comment which precedes it, because both
> tables are range-partitioned as shown below.

> I think the test should be moved to a more appropriate place,

On second thought I changed my mind; we would not need to move that
test, so I refrained from doing so.

> Attached is a patch for that.  The
> patch fixes another misplaced comment as well.

I pushed an updated version of the patch.

Best regards,
Etsuro Fujita

Re: Unix-domain socket support on Windows

[Peter Eisentraut]

On 2019-12-16 05:39, Andrew Dunstan wrote:

On Wed, Oct 30, 2019 at 10:32 PM Peter Eisentraut
 wrote:


To move this topic a long, I'll submit some preparatory patches in a
committable order.

First is the patch to deal with getpeereid() that was already included
in the previous patch series.  This is just some refactoring that
reduces the difference between Windows and other platforms and prepares
the Unix-domain socket specific code to compile cleanly on Windows.




This looks fairly sane and straightforward. Let's give it an outing on
the buildfarm ASAP so we can keep moving forward on this.


pushed

--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: logical decoding : exceeded maxAllocatedDescs for .spill files

[Amit Khandekar]

On Sat, 14 Dec 2019 at 11:59, Amit Kapila  wrote:
>
> On Thu, Dec 12, 2019 at 9:50 PM Amit Khandekar  wrote:
> >
> > Attached is a v4 patch that also addresses your code comments so far.
> > I have included the test case in 006_logical_decoding.pl. I observed
> > that the test case just adds only about 0.5 to 1 sec time. Please
> > verify on your env also, and also whether the test reproduces the
> > issue without the code changes.
> >
>
> It takes roughly the same time on my machine as well.  I have checked
> on Windows as well, it increases the time from 14 to 16 (17) seconds
> for this test.  I don't think this is any big increase considering the
> timing of other tests and it would be good to have a test for such
> boundary conditions.  I have slightly change the comments in the patch
> and ran pgindent.  Attached, find the patch with a proposed commit
> message.
>
> I have also made minor changes related to below code in patch:
> - else if (readBytes != sizeof(ReorderBufferDiskChange))
> +
> + file->curOffset += readBytes;
> +
> + if (readBytes !=
> sizeof(ReorderBufferDiskChange))
>
> Why the size is added before the error check?
The logic was : even though it's an error that the readBytes does not
match the expected size, the file read is successful so update the vfd
offset as early as possible. In our case, this might not matter much,
but who knows, in the future, in the exception block (say, in
ReorderBufferIterTXNFinish, someone assumes that the file offset is
correct and does something with that, then we will get in trouble,
although I agree that it's very unlikely. But IMO, because we want to
simulate the file offset support in vfd, we should update the file
offset immediately after a file read is known to have succeeded.

>  I think it should be
> after that check, so changed accordingly.  Similarly, I don't see why
> we need to change 'else if' to 'if' in this code, so changed back.
Since for adding the size before the error check I had to remove the
else-if, so to be consistent, I removed the else-if at surrounding
places also.

>
> I think we need to change/tweak the test for back branches as there we
> don't have logical_decoding_work_mem.  Can you please look into that
Yeah, I believe we need to backport up to PG 9.4 where logical
decoding was introduced, so I am first trying out with 9.4 branch.

> and see if you can run perltidy for the test file.
Hmm, I tried perltidy, and it seems to mostly add a space after ( and
a space before ) if there's already; so "('postgres'," is replaced by
"( 'postgres',". And this is going to be inconsistent with
other places. And it replaces tab with spaces. Do you think we should
try perltidy, or have we before been using this tool for the tap tests
?


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: segmentation fault when cassert enabled

[Amit Kapila]

On Fri, Dec 13, 2019 at 12:10 PM vignesh C  wrote:
>
> I have made changes to fix the comment provided. The patch for the
> same is attached. Could not add a test case for this scenario is based
> on timing issue.
> Thoughts?
>

I agree that this is a timing issue.  I also don't see a way to write
a reproducible test for this.  However, I could reproduce it via
debugger consistently by following the below steps.  I have updated a
few comments and commit messages in the attached patch.

Peter E., Petr J or anyone else, do you have comments or objections on
this patch?  If none, then I am planning to commit (and backpatch)
this patch in a few days time.

Test steps to reproduce the issue.
Set up
-
set up master and subscriber nodes.
In code, add a while(true) in apply_handle_update() before a call to
logicalrep_rel_open().  This is to ensure that we can debug the replay
of Update
operation on subscriber.

Master
---
Create table t1(c1 int);
Create publication pub_t1 for table t1;
Alter table t1 replica identity full;


Subscriber
-
Create table t1(c1 int);
CREATE SUBSCRIPTION sub_t1 CONNECTION 'host=localhost port=5432
dbname=postgres' PUBLICATION pub_t1;

Master
--
Insert into t1 values(1);  --this will create LogicalRepRelMap entry
for t1 on subscriber.

Subscriber
--
Select * from t1; -- This should display the data inserted in master.

Master
--
Update t1 set c1 = 2 where c1=1;

Now on the subscriber, attach a debugger and debug logicalrep_rel_open
and stop debugger just before table_open call.

Subscriber
---
Alter table t1 add c2 int;

Now, continue in debugger, it should crash in slot_store_cstrings()
because the rel->attrmap is not updated.


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


0001-Fix-subscriber-invalid-memory-access-on-DDL.amit.1.patch
Description: Binary data

Request to be allotted a project or a feature in pipeline

[Utsav Parmar]

To whom it may concern,

I'm Utsav Parmar, pursuing my B. Tech in Computer Engineering. I like to
work on new technologies and am currently looking for open-source projects
to contribute to.

As it may turn out, I've got a college project in my curriculum this
semester under “Software Development Practice”, and I'd like to work upon a
project and/or a feature in pipeline spanning over 3 months in PostgreSQL
organization as a part of the same college project. My mentor cum professor
has already agreed for the same, given that I get approval from one of the
maintainers. So, if possible, will you please allot me something to work
upon?

Thank you for your time. Looking forward to hearing from you.

Regards,

Utsav Parmar

Improvement to psql's connection defaults

[Tomas Zubiri]

Hello, this week I decided to pursue an error a bit further than
usual, even after having fixed it for myself, I found that I could fix
it for future newcomers, especially those running
containerized distributions.

The problem was that running the command psql without arguments
returned the following
error message:

psql: could not connect to server: No such file or directory
Is the server running locally and accepting
connections on Unix domain socket "/var/run/postgresql/.s.PGSQL.5432"?

Now, I eventually found a way around this by specifying the host with
the following command 'psql -h localhost -p 5432'.

However, the answers I found on google didn't suggest this simple fix
at all, I found a lot of confused users either exposing the sockets
from their containers, or worse, bashing into their containers and
running psql from inside :*(
https://stackoverflow.com/questions/27673563/how-to-get-into-psql-of-a-running-postgres-container/59296176#59296176

I also found this is a common error in postgres docs:
https://www.postgresql.org/docs/9.1/server-start.html
https://www.postgresql.org/docs/10/tutorial-createdb.html


So I wondered, since psql went through the trouble of guessing my unix
socket, it could guess my hostname as well. Indeed I would later find
that the tcp defaults were already implemented on non-unix builds,
additionally psql already has a mechanism to try multiple connections.
So
my humble change is for unix builds to try to connect via unix socket,
and if that fails, to connect via localhost. This would save headaches
for newbies trying to connect for the first time.

Attached you will find my patch. Below you can find the form required
for submitting patches.

Project name: Not sure, psql?
Uniquely identifiable file name, so we can tell the difference between
your v1 and v24:
Running-psql-without-specifying-host-on-unix-systems.patch
What the patch does in a short paragraph: When psql is not supplied a
host or hostname, and connection via default socket fails, psql will
attempt to connect via default tcp, probably localhost.
Whether the patch is for discussion or for application: Application,
but further testing is required.
Which branch the patch is against: master
Whether it compiles and tests successfully, so we know nothing obvious
is broken: Compiles and works successfully in my linux machine,
however I can't test whether this works on non-unix machines, I will
need some help there. I didn't see any automated tests, hopefully I
didn't miss any.
Whether it contains any platform-specific items and if so, has it been
tested on other platforms: Yes, connection via socket is only
available on unix systems. I need help testing on other platforms.
Confirm that the patch includes regression tests to check the new
feature actually works as described.: make check runs successfully,
there seems to be a test called psql_command that confirms that psql
can connect without specifying host. But I didn't add a test for
connecting via tcp.
Include documentation on how to use the new feature, including
examples: The docs already describe the correct behaviour in
/doc/src/sgml/ref/psql-ref.sgml "If you omit the host name psql will
connect via a Unix-domain socket to a server on the local host, or via
TCP/IP to localhost on machines that don't have Unix-domain sockets."
Describe the effect your patch has on performance, if any: OS without
unix socket support still won't try to connect via unix socket so they
will be unaffected. This change should only affect paths where
connection via socket failed and the user would have been shown an
error. One could argue that, some users might suffer a slight
performance hit by not being told that they are connecting via a
subpar method, but this is a sub-tenth of a second latency difference
for local connections I believe. If this is an issue, a warning could
be added.


Thank you for time,
Tomas.
From 6e4d305b6a011df4ea20f606431e50b3462a18c7 Mon Sep 17 00:00:00 2001
From: Tomas Zubiri 
Date: Sun, 15 Dec 2019 02:46:19 -0300
Subject: [PATCH] Running psql without specifying host on unix systems should
 try connecting via tcp if unix socket is unavailable.

---
 src/interfaces/libpq/fe-connect.c | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 66a9128605..5307a99dfc 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -994,7 +994,9 @@ connectOptions2(PGconn *conn)
 	/*
 	 * Allocate memory for details about each host to which we might possibly
 	 * try to connect.  For that, count the number of elements in the hostaddr
-	 * or host options.  If neither is given, assume one host.
+	 * or host options.  If neither is given, assume 2 hosts on systems with
+	 * unix socket support, try to connect via sockets first, if that fails,
+	 *  connect via tcp. On non-unix systems, connect via tcp directly.
 	 */

Re: logical decoding : exceeded maxAllocatedDescs for .spill files

[Amit Kapila]

On Mon, Dec 16, 2019 at 3:26 PM Amit Khandekar  wrote:
>
> On Sat, 14 Dec 2019 at 11:59, Amit Kapila  wrote:
> >
> > I have also made minor changes related to below code in patch:
> > - else if (readBytes != sizeof(ReorderBufferDiskChange))
> > +
> > + file->curOffset += readBytes;
> > +
> > + if (readBytes !=
> > sizeof(ReorderBufferDiskChange))
> >
> > Why the size is added before the error check?
> The logic was : even though it's an error that the readBytes does not
> match the expected size, the file read is successful so update the vfd
> offset as early as possible. In our case, this might not matter much,
> but who knows, in the future, in the exception block (say, in
> ReorderBufferIterTXNFinish, someone assumes that the file offset is
> correct and does something with that, then we will get in trouble,
> although I agree that it's very unlikely.
>

I am not sure if there is any such need, but even if it is there, I
think updating after a *short* read (read less than expected) doesn't
seem like a good idea because there is clearly some problem with the
read call.  Also, in the case below that case where we read the actual
change data, the offset is updated after the check of *short* read.  I
don't see any advantage in such an inconsistency.  I still feel it is
better to update the offset after all error checks.

>
> > and see if you can run perltidy for the test file.
> Hmm, I tried perltidy, and it seems to mostly add a space after ( and
> a space before ) if there's already; so "('postgres'," is replaced by
> "( 'postgres',". And this is going to be inconsistent with
> other places. And it replaces tab with spaces. Do you think we should
> try perltidy, or have we before been using this tool for the tap tests
> ?
>

See text in src/test/perl/README (Note that all tests and test tools
should have perltidy run on them before patches are submitted, using
perltidy - profile=src/tools/pgindent/perltidyrc).  It is recommended
to use perltidy.

Now, if it is making the added code inconsistent with nearby code,
then I suggest to leave it.


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

Re: automating pg_config.h.win32 maintenance

[Peter Eisentraut]

On 2019-12-13 14:56, Tom Lane wrote:

One thing that disturbs me slightly is that the plan seems to be to
not mention variables in this list at all if they're to be undefined
on Windows.  I realize that we've frequently done that by omission in
pg_config.h.win32, but I don't think it's good practice: it encourages
failure to think about how such variables need to be set on Windows.


OK, here is an updated patch set that has all defines in one big Perl 
hash, and also requires that all symbols in pg_config.h.in are accounted 
for.  (The indentation is from pgperltidy.)


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 50e40cc0054309c93a3a8246f2b84c7f681a9d31 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 7 Nov 2019 15:56:22 +0100
Subject: [PATCH v2 1/2] Generate pg_config.h from pg_config.h.in on Windows

---
 src/tools/msvc/Solution.pm | 450 -
 1 file changed, 391 insertions(+), 59 deletions(-)

diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 94cc4b950e..e7b893704a 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -20,7 +20,6 @@ sub _new
projects   => {},
options=> $options,
numver => '',
-   strver => '',
VisualStudioVersion=> undef,
MinimumVisualStudioVersion => undef,
vcver  => undef,
@@ -140,16 +139,22 @@ sub GenerateFiles
 {
my $self = shift;
my $bits = $self->{platform} eq 'Win32' ? 32 : 64;
+   my $package_name;
+   my $package_version;
+   my $package_bugreport;
 
# Parse configure.in to get version numbers
open(my $c, '<', "configure.in")
  || confess("Could not open configure.in for reading\n");
while (<$c>)
{
-   if (/^AC_INIT\(\[PostgreSQL\], \[([^\]]+)\]/)
+   if (/^AC_INIT\(\[([^\]]+)\], \[([^\]]+)\], \[([^\]]+)\]/)
{
-   $self->{strver} = $1;
-   if ($self->{strver} !~ /^(\d+)(?:\.(\d+))?/)
+   $package_name  = $1;
+   $package_version   = $2;
+   $package_bugreport = $3;
+
+   if ($package_version !~ /^(\d+)(?:\.(\d+))?/)
{
confess "Bad format of version: 
$self->{strver}\n";
}
@@ -159,7 +164,10 @@ sub GenerateFiles
}
close($c);
confess "Unable to parse configure.in for all variables!"
- if ($self->{strver} eq '' || $self->{numver} eq '');
+ if ( $package_name eq ''
+   || $package_version eq ''
+   || $self->{numver} eq ''
+   || $package_bugreport eq '');
 
if (IsNewer("src/include/pg_config_os.h", "src/include/port/win32.h"))
{
@@ -167,89 +175,413 @@ sub GenerateFiles
copyFile("src/include/port/win32.h", 
"src/include/pg_config_os.h");
}
 
-   if (IsNewer("src/include/pg_config.h", "src/include/pg_config.h.win32"))
+   if (IsNewer("src/include/pg_config.h", "src/include/pg_config.h.in"))
{
print "Generating pg_config.h...\n";
-   open(my $i, '<', "src/include/pg_config.h.win32")
- || confess "Could not open pg_config.h.win32\n";
-   open(my $o, '>', "src/include/pg_config.h")
- || confess "Could not write to pg_config.h\n";
+
my $extraver = $self->{options}->{extraver};
$extraver = '' unless defined $extraver;
-   while (<$i>)
-   {
-   s{PG_VERSION "[^"]+"}{PG_VERSION 
"$self->{strver}$extraver"};
-   s{PG_VERSION_NUM \d+}{PG_VERSION_NUM $self->{numver}};
-   s{PG_VERSION_STR "[^"]+"}{PG_VERSION_STR "PostgreSQL 
$self->{strver}$extraver, compiled by Visual C++ build " CppAsString2(_MSC_VER) 
", $bits-bit"};
-   print $o $_;
-   }
-   print $o "#define PG_MAJORVERSION \"$self->{majorver}\"\n";
-   print $o "/* defines added by config steps */\n";
-   print $o "#ifndef IGNORE_CONFIGURED_SETTINGS\n";
-   print $o "#define USE_ASSERT_CHECKING 1\n"
- if ($self->{options}->{asserts});
-   print $o "#define USE_LDAP 1\n"   if ($self->{options}->{ldap});
-   print $o "#define HAVE_LIBZ 1\n"  if ($self->{options}->{zlib});
-   print $o "#define ENABLE_NLS 1\n" if ($self->{options}->{nls});
-
-   print $o "#define BLCKSZ ", 1024 * 
$self->{options}->{blocksize},
- "\n";
-   print $o "#define RELSEG_SIZE ",
-

Re: Collation versions on Windows (help wanted, apply within)

[Peter Eisentraut]

On 2019-12-16 01:26, Thomas Munro wrote:

While wondering about that, I noticed the "C.UTF-8" encoding (here on
a glibc system):

postgres=# \pset null 
Null display is "".
postgres=# select collname, collprovider, collencoding, collcollate,
collctype, collversion from pg_collation ;
   collname  | collprovider | collencoding | collcollate | collctype  |
collversion
+--+--+-++-
  default| d|   -1 | || 
  C  | c|   -1 | C   | C  | 
  POSIX  | c|   -1 | POSIX   | POSIX  | 
  ucs_basic  | c|6 | C   | C  | 
  C.UTF-8| c|6 | C.UTF-8 | C.UTF-8| 2.28
  en_NZ.utf8 | c|6 | en_NZ.utf8  | en_NZ.utf8 | 2.28
  en_NZ  | c|6 | en_NZ.utf8  | en_NZ.utf8 | 2.28
(7 rows)

I wonder if we should do something to give that no collversion, since
we know that it's really just another way of spelling "binary sort
please", or if we'd be better off not trying to interpret the names
locale -a spits out.


I think it's worth handling that separately.  If we want to give it an 
air of generality and not just hard-code that one locale name, we could 
strip off the encoding and compare the rest with the well-known encoding 
names.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: segmentation fault when cassert enabled

[Peter Eisentraut]

On 2019-12-16 11:11, Amit Kapila wrote:

I agree that this is a timing issue.  I also don't see a way to write
a reproducible test for this.  However, I could reproduce it via
debugger consistently by following the below steps.  I have updated a
few comments and commit messages in the attached patch.

Peter E., Petr J or anyone else, do you have comments or objections on
this patch?  If none, then I am planning to commit (and backpatch)
this patch in a few days time.


The patch seems fine to me.  Writing a test seems hard.  Let's skip it.

The commit message has a duplicate "building"/"built" in the first sentence.

--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Collation versions on Windows (help wanted, apply within)

[Juan José Santamaría Flecha]

On Mon, Dec 16, 2019 at 1:26 AM Thomas Munro  wrote:

> On Wed, Nov 27, 2019 at 10:38 AM Peter Eisentraut
>
> Would you mind posting the full output of the above query (with 
> showing) on a Windows system after running initdb with the -v2 patch,
> so we can see how the collations look?
>
>
Sure, you can find attached the full output with ICU.

This is a resume to illustrate an issue with locale = 'C':

postgres=#   CREATE COLLATION c_test (locale = 'C');
CREATE COLLATION
postgres=# select collname, collprovider, collencoding, collcollate,
collctype, collversion from pg_collation ;
collname| collprovider | collencoding |   collcollate|
   collctype | collversion
+--+--+--+--+-
 default| d|   -1 |  |
 | 
 C  | c|   -1 | C|
C| 
 POSIX  | c|   -1 | POSIX|
POSIX| 
 ucs_basic  | c|6 | C|
C| 
 und-x-icu  | i|   -1 | und  |
und  | 153.97
[... resumed ...]
 c_test | c|6 | C|
C|
(757 rows)

 Shouldn't it be NULL?

Regards,

Juan José Santamaría Flecha


pg_collation.out
Description: Binary data

Re: Improvement to psql's connection defaults

[Christoph Moench-Tegeder]

## Tomas Zubiri (m...@tomaszubiri.com):

> The problem was that running the command psql without arguments

There's an excellent manpage for psql, which can also be found online:
https://www.postgresql.org/docs/current/app-psql.html

In there you'll find a section "Connecting to a Database", with the
following sentences:
: In order to connect to a database you need to know the name of your
: target database, the host name and port number of the server, and what
: user name you want to connect as.  psql can be told about those parameters
: via command line options, namely -d, -h, -p, and -U respectively.

and
: If you omit the host name, psql will connect via a Unix-domain socket
: to a server on the local host, or via TCP/IP to localhost on machines
: that don't have Unix-domain sockets.

I'm a little confused as to why people don't read the documentation and
turn to the 'net - that's bound to dig up a lot of people who haven't
read the docs, too.

> So
> my humble change is for unix builds to try to connect via unix socket,
> and if that fails, to connect via localhost. This would save headaches
> for newbies trying to connect for the first time.

I'd thing that opens a can of worms:
- authentication options for TCP connections, even on localhost, are
  often different from those for Unix-domain sockets (e.g. while
  using peer authentication for administration purposes might make
  a lot of sense, TCP connections need some credential-based authentication
  so "any rogue process" cannot simply connect to your database).
- Do we have any guarantees that these containers always expose the
  PostgreSQL server on what the host thinks is "localhost:5432"? I'm
  thinking of network namespaces, dedicated container network interfaces
  and all the other shenanigans. And what about the use cases of "more
  than one container" and "database on the host and in a container"?
  My concers is that adding more "magic" into the connection logic
  will result in more confusion instead of less - the distinction
  between the "default case Unix-domain socket" and "TCP" will be lost.

Regards,
Christoph

-- 
Spare Space

Re: Conflict handling for COPY FROM

[Surafel Temesgen]

On Thu, Dec 12, 2019 at 7:51 AM asaba.takan...@fujitsu.com <
asaba.takan...@fujitsu.com> wrote:

> 2. I have a question about copy meta-command.
> When I executed copy meta-command, output wasn't displayed.
> Does it correspond to copy meta-command?
>
>
Fixed

regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d9b7c4d0d4..a0ac5b4ef7 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
 FORCE_NULL ( column_name [, ...] )
 ENCODING 'encoding_name'
+ERROR_LIMIT 'limit_number'
 
  
 
@@ -355,6 +356,26 @@ COPY { table_name [ ( 

 
+   
+ERROR_LIMIT
+
+ 
+  Enables ignoring of errored out rows up to limit_number. If limit_number is set
+  to -1, then all errors will be ignored.
+ 
+
+ 
+  Currently, only unique or exclusion constraint violation
+  and rows formatting errors are ignored. Malformed
+  rows will rise warnings, while constraint violating rows
+  will be returned back to the caller.
+ 
+
+
+   
+

 WHERE
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e17d8c760f..c911b3d0c2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -24,6 +24,7 @@
 #include "access/tableam.h"
 #include "access/xact.h"
 #include "access/xlog.h"
+#include "access/printtup.h"
 #include "catalog/dependency.h"
 #include "catalog/pg_authid.h"
 #include "catalog/pg_type.h"
@@ -48,7 +49,9 @@
 #include "port/pg_bswap.h"
 #include "rewrite/rewriteHandler.h"
 #include "storage/fd.h"
+#include "storage/lmgr.h"
 #include "tcop/tcopprot.h"
+#include "tcop/pquery.h"
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
@@ -154,6 +157,7 @@ typedef struct CopyStateData
 	List	   *convert_select; /* list of column names (can be NIL) */
 	bool	   *convert_select_flags;	/* per-column CSV/TEXT CS flags */
 	Node	   *whereClause;	/* WHERE condition (or NULL) */
+	int			error_limit;	/* total number of error to ignore */
 
 	/* these are just for error messages, see CopyFromErrorCallback */
 	const char *cur_relname;	/* table name for error messages */
@@ -183,6 +187,9 @@ typedef struct CopyStateData
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
 	List	   *range_table;
 	ExprState  *qualexpr;
+	bool		ignore_error;	/* is ignore error specified? */
+	bool		ignore_all_error;	/* is error_limit -1 (ignore all error)
+	 * specified? */
 
 	TransitionCaptureState *transition_capture;
 
@@ -837,7 +844,7 @@ CopyLoadRawBuf(CopyState cstate)
 void
 DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	   int stmt_location, int stmt_len,
-	   uint64 *processed)
+	   uint64 *processed, DestReceiver *dest)
 {
 	CopyState	cstate;
 	bool		is_from = stmt->is_from;
@@ -1068,7 +1075,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 		cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
 			   NULL, stmt->attlist, stmt->options);
 		cstate->whereClause = whereClause;
-		*processed = CopyFrom(cstate);	/* copy from file to database */
+		*processed = CopyFrom(cstate, dest);	/* copy from file to database */
 		EndCopyFrom(cstate);
 	}
 	else
@@ -1290,6 +1297,18 @@ ProcessCopyOptions(ParseState *pstate,
 defel->defname),
 		 parser_errposition(pstate, defel->location)));
 		}
+		else if (strcmp(defel->defname, "error_limit") == 0)
+		{
+			if (cstate->ignore_error)
+ereport(ERROR,
+		(errcode(ERRCODE_SYNTAX_ERROR),
+		 errmsg("conflicting or redundant options"),
+		 parser_errposition(pstate, defel->location)));
+			cstate->error_limit = defGetInt64(defel);
+			cstate->ignore_error = true;
+			if (cstate->error_limit == -1)
+cstate->ignore_all_error = true;
+		}
 		else
 			ereport(ERROR,
 	(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1440,6 +1459,10 @@ ProcessCopyOptions(ParseState *pstate,
 		ereport(ERROR,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
  errmsg("CSV quote character must not appear in the NULL specification")));
+	if (cstate->ignore_error && !cstate->is_copy_from)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ERROR LIMIT only available using COPY FROM")));
 }
 
 /*
@@ -2653,7 +2676,7 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
  * Copy FROM file to relation.
  */
 uint64
-CopyFrom(CopyState cstate)
+CopyFrom(CopyState cstate, DestReceiver *dest)
 {
 	ResultRelInfo *resultRelInfo;
 	ResultRelInfo *target_resultRelInfo;
@@ -2675,6 +2698,7 @@ CopyFrom(CopyState cstate)
 	bool		has_before_insert_row_trig;
 	bool		has_instead_insert_row_trig;
 	bool		leafpart_use_multi_insert = false;
+	Portal		portal = NULL;
 
 	Assert(cstate->rel);
 
@@ -2838,7 +2862,19 @@ CopyFrom(CopyState cstate)
 	/* Verify the named relation is a valid target for INSERT */
 	CheckValidResultRel(resultRelInfo, CMD_INSERT);
 
-	ExecOpenIndices(resultRelInfo, false);
+	

Re: logical replication does not fire per-column triggers

[Peter Eisentraut]

On 2019-12-14 03:13, Euler Taveira wrote:

Using the regression test example, table tab_fk_ref have columns id
and bid. If you add a trigger "BEFORE UPDATE OF bid" into subscriber
that fires on replica, it will always fire even if you are **not**
changed bid in publisher. In logical replication protocol all columns
were changed unless it is a (unchanged) TOAST column (if a column is
part of the PK/REPLICA IDENTITY we can compare both values and figure
out if the value changed, however, we can't ensure that a value
changed for the other columns -- those that are not PK/REPLICA
IDENTITY). It is clear that not firing the trigger is wrong but firing
it when you say that you won't fire it is also wrong. Whichever
behavior we choose, limitation should be documented. I prefer the
behavior that ignores "OF col1" and always fire the trigger (because
we can add a filter inside the function/procedure).


There is a small difference: If the subscriber has extra columns not 
present on the publisher, then a column trigger covering only columns in 
published column set will not fire.


In practice, a column trigger is just an optimization.  The column it is 
triggering on might not have actually changed.  The opposite is worse, 
not firing the trigger when the column actually has changed.



+ /* Populate updatedCols for trigger manager */
Add a comment that explains it is not possible to (always) determine
if a column changed. Hence, "OF col1" syntax will be ignored.


done


+ for (int i = 0; i < remoteslot->tts_tupleDescriptor->natts; i++)
+ {
+ RangeTblEntry *target_rte = list_nth(estate->es_range_table, 0);
+
It should be outside the loop.


fixed


+ if (newtup.changed)
It should be newtup.changed[i].


fixed


You should add a test that exposes "ignore OF col1" such as:

$node_publisher->safe_psql('postgres',
 "UPDATE tab_fk_ref SET id = 6 WHERE id = 1;");


done

New patch attached.

--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From e0b5ad061bed9e5a6fcf889124651f6c9fc329cd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 16 Dec 2019 14:30:42 +0100
Subject: [PATCH v2] Have logical replication subscriber fire column triggers

The logical replication apply worker did not fire per-column update
triggers because the updatedCols bitmap in the RTE was not populated.
This fixes that.
---
 src/backend/replication/logical/worker.c   | 16 +++
 src/test/subscription/t/003_constraints.pl | 32 +++---
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/src/backend/replication/logical/worker.c 
b/src/backend/replication/logical/worker.c
index ced0d191c2..99b2be0835 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -690,6 +690,7 @@ apply_handle_update(StringInfo s)
boolhas_oldtup;
TupleTableSlot *localslot;
TupleTableSlot *remoteslot;
+   RangeTblEntry *target_rte;
boolfound;
MemoryContext oldctx;
 
@@ -720,6 +721,21 @@ apply_handle_update(StringInfo s)
  
&estate->es_tupleTable);
EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1);
 
+   /*
+* Populate updatedCols so that per-column triggers can fire.  This 
could
+* include more columns than were actually changed on the publisher
+* because the logical replication protocol doesn't contain that
+* information.  But it would for example exclude columns that only 
exist
+* on the subscriber, since we are not touching those.
+*/
+   target_rte = list_nth(estate->es_range_table, 0);
+   for (int i = 0; i < remoteslot->tts_tupleDescriptor->natts; i++)
+   {
+   if (newtup.changed[i])
+   target_rte->updatedCols = 
bms_add_member(target_rte->updatedCols,
+   
 i + 1 - FirstLowInvalidHeapAttributeNumber);
+   }
+
PushActiveSnapshot(GetTransactionSnapshot());
ExecOpenIndices(estate->es_result_relation_info, false);
 
diff --git a/src/test/subscription/t/003_constraints.pl 
b/src/test/subscription/t/003_constraints.pl
index 81547f65fa..34ab11e7fe 100644
--- a/src/test/subscription/t/003_constraints.pl
+++ b/src/test/subscription/t/003_constraints.pl
@@ -3,7 +3,7 @@
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 4;
+use Test::More tests => 6;
 
 # Initialize publisher node
 my $node_publisher = get_new_node('publisher');
@@ -81,6 +81,8 @@ BEGIN
 ELSE
 RETURN NULL;
 END IF;
+ELSIF (TG_OP = 'UPDATE') THEN
+RETURN NULL;
 ELSE
 RAISE WARNING 'Unknown action';
 RETURN NULL;
@@ -88,7 +90,7 @@ BEGIN
 END;
 \$\$ LANGUAGE plpgsql;
 CREATE TRIGGER filter_basic_dml_trg
-BE

Re: What's the best way to get flex and bison on Windows?

[Tom Lane]

Thomas Munro  writes:
> Thanks.  That didn't work but helped me find my way to
> C:\msys64\usr\bin.  That version of bison complains about our grammar
> using deprecated directives, but that's a matter for another day.

Oh, that's a known issue on late-model bison.  The problem is that the
option syntax it wants us to use doesn't exist at all on older bison
versions.  So far we haven't been willing to break old platforms just
to suppress the warning.  We'll probably have to find another answer
once a decent fraction of PG hackers start using bison versions that
give the warning.

regards, tom lane

Re: Request to be allotted a project or a feature in pipeline

[Laurenz Albe]

On Sun, 2019-12-15 at 21:56 +0530, Utsav Parmar wrote:
> I'm Utsav Parmar, pursuing my B. Tech in Computer Engineering. I like to work 
> on new technologies
> and am currently looking for open-source projects to contribute to.
> 
> As it may turn out, I've got a college project in my curriculum this semester 
> under
> “Software Development Practice”, and I'd like to work upon a project and/or a 
> feature
> in pipeline spanning over 3 months in PostgreSQL organization as a part of 
> the same
> college project. My mentor cum professor has already agreed for the same, 
> given that
> I get approval from one of the maintainers. So, if possible, will you please 
> allot me
> something to work upon?
> 
> Thank you for your time. Looking forward to hearing from you.

One thing that we can never have enough of is reviewers, that is people who 
examine
patches in the commitfest and give feedback on them.
Look here for details: https://wiki.postgresql.org/wiki/Reviewing_a_Patch

Code review is definitely part of software development practice, and
reading and understanding the code of experienced developers can teach
you a lot.  Another nice aspect is that this is an activity that can easily
be adjusted to span three months; if you embark on a new feature, the
three months may pass without your patch getting accepted.

Yours,
Laurenz Albe

Re: Improvement to psql's connection defaults

[Tom Lane]

Tomas Zubiri  writes:
> The problem was that running the command psql without arguments
> returned the following
> error message:
> psql: could not connect to server: No such file or directory
> Is the server running locally and accepting
> connections on Unix domain socket "/var/run/postgresql/.s.PGSQL.5432"?

The reason this failed, most likely, is using a semi-broken installation
in which libpq has a different idea than the server of where the
unix socket should be.  The right fix is one or the other of

(a) don't mix-and-match Postgres packages from different vendors,

(b) adjust the server's unix_socket_directories parameter so that
it creates a socket where your installed libpq expects to find it.

I realize that this isn't great from a newbie-experience standpoint,
but unfortunately we don't have a lot of control over varying
packager decisions about the socket location --- both the "/tmp"
and the "/var/run/postgresql" camps have valid reasons for their
choices.

I do not think your proposal would improve matters; it'd just introduce
yet another variable, ie which transport method did libpq choose.
As Christoph noted, that affects authentication behaviors, and there
are a bunch of other user-visible impacts too (SSL, timeouts, ...).

If we were going to do something of this sort, what I'd be inclined
to think about is having an option to probe both of the common socket
directory choices, rather than getting into TCP-land.  But that still
might be a net negative from the standpoint of confusion vs. number of
cases it fixes.

regards, tom lane

Re: Improvement to psql's connection defaults

[Alvaro Herrera]

Hello,

On 2019-Dec-15, Tomas Zubiri wrote:

> Attached you will find my patch. Below you can find the form required
> for submitting patches.
> 
> Project name: Not sure, psql?
> Uniquely identifiable file name, so we can tell the difference between
> your v1 and v24:
> [...]

Please, where did you find this "form"?  We don't have a *required* form
for submitting patches; I suspect there's an opinionated page somewhere
that we should strive to fix.  (Those questions you list are appropriate
to answer, but forcing you to repeat what you had already explained in
the first part of your email is pointless bureaucracy.)

Thanks

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

Re: Improvement to psql's connection defaults

[Daniel Gustafsson]

> On 16 Dec 2019, at 15:47, Alvaro Herrera  wrote:

> Please, where did you find this "form"?

It seems to be from the wiki:

  https://wiki.postgresql.org/wiki/Submitting_a_Patch#Patch_submission

cheers ./daniel

Re: Online checksums patch - once again

[Daniel Gustafsson]

> On 5 Dec 2019, at 16:13, Robert Haas  wrote:
> 
> On Tue, Dec 3, 2019 at 6:41 PM Daniel Gustafsson  wrote:
>> Attached is a rebased v14 patchset on top of maser.  The Global Barriers 
>> patch
>> is left as a prerequisite, but it will obviously be dropped, or be
>> significantly changed, once the work Robert is doing with ProcSignalBarrier
>> lands.
> 
> Any chance you and/or Magnus could offer opinions on some of those
> patches? I am reluctant to start committing things with nobody having
> replied.

Attached is a v15 of the online checksums patchset (minus 0005), rebased on top
of your v3 ProcSignalBarrier patch rather than Andres' PoC GlobalBarrier patch.
It does take the, perhaps, controversial approach of replacing the SAMPLE
barrier with the CHECKSUM barrier.  The cfbot will be angry since this email
doesn't contain the procsignalbarrier patch, but it sounded like that would go
in shortly so opted for that.

This version also contains touchups to the documentation part, as well as a
pgindent run.

If reviewers think this version is nearing completion, then a v16 should
address the comment below, but as this version switches its underlying
infrastructure it seemed usefel for testing still.

+   /*
+* Force a checkpoint to get everything out to disk. XXX: this should
+* probably not be an IMMEDIATE checkpoint, but leave it there for now for
+* testing.
+*/
+   RequestCheckpoint(CHECKPOINT_FORCE | CHECKPOINT_WAIT | 
CHECKPOINT_IMMEDIATE);

cheers ./daniel



online_checksums15.patch
Description: Binary data

Re: small Bison optimization: remove single character literal tokens

[Tom Lane]

John Naylor  writes:
> Traditionally, when Bison generates the header file, it starts
> numbering named tokens at 258, so that numbers below 256 can be used
> for character literals. Then, during parsing, the external token
> number or character literal is mapped to Bison's internal token number
> via the yytranslate[] array.
> The newly-released Bison 3.5 has the option "%define api.token.raw",
> which causes Bison to write out the same ("raw") token numbers it
> would use internally, and thus skip building the yytranslate[] array
> as well as the code to access it. To make use of this, there cannot be
> any single character literals in the grammar, otherwise Bison will
> refuse to build.
> Attached is a draft patch to make the core grammar forward-compatible
> with this option by using FULL_NAMES for all valid single character
> tokens. Benchmarking raw parsing with "%define api.token.raw" enabled
> shows ~1.7-1.9% improvement compared with not setting the option. Not
> much, but doing one less array access per token reduces cache
> pollution and saves a few kB of binary size as well.

TBH, I'm having a hard time getting excited about this.  It seems like
you've just moved the mapping from point A to point B, that is, in
place of a lookup in the grammar you have to have the lexer translate
ASCII characters to something else.  I'm not sure that's an improvement
at all.  And I'm really unexcited about applying a patch that's this
invasive in order to chase a very small improvement ... especially a
very small improvement that we can't even have anytime soon.

> It'll be years before Bison 3.5 is common in the wild,

It'll be *decades* before we'd consider requiring it, really, unless
there are truly striking improvements unrelated to this point.

regards, tom lane

Re: segmentation fault when cassert enabled

[Jehan-Guillaume de Rorthais]

On Fri, 13 Dec 2019 12:10:07 +0530
vignesh C  wrote:

> On Fri, Dec 6, 2019 at 5:30 PM Amit Kapila  wrote:
> >
> > On Mon, Nov 25, 2019 at 8:25 PM Jehan-Guillaume de Rorthais
> >  wrote:  
> > >
> > > On Wed, 6 Nov 2019 14:34:38 +0100
> > > Peter Eisentraut  wrote:
> > >  
> > > > On 2019-11-05 17:29, Jehan-Guillaume de Rorthais wrote:  
> > > > > My best bet so far is that logicalrep_relmap_invalidate_cb is not
> > > > > called after the DDL on the subscriber so the relmap cache is not
> > > > > invalidated. So we end up with slot->tts_tupleDescriptor->natts
> > > > > superior than rel->remoterel->natts in slot_store_cstrings, leading
> > > > > to the overflow on attrmap and the sigsev.  
> > > >
> > > > It looks like something like that is happening.  But it shouldn't.
> > > > Different table schemas on publisher and subscriber are well supported,
> > > > so this must be an edge case of some kind.  Please continue
> > > > investigating.  
> > >
> > > I've been able to find the origin of the crash, but it was a long journey.
> > >
> > > 
> > >
> > >   I was unable to debug using gdb record because of this famous error:
> > >
> > > Process record does not support instruction 0xc5 at address
> > > 0x1482758a4b30.
> > >
> > > Program stopped.
> > > __memset_avx2_unaligned_erms ()
> > > at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:168
> > > 168 ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S: No such
> > >   file or directory.
> > >
> > >   Trying with rr, I had constant "stack depth limit exceeded", even with
> > >   unlimited one. Does it worth opening a discussion or a wiki page about
> > > these tools? Peter, it looks like you have some experience with rr, any
> > > feedback?
> > >
> > >   Finally, Julien Rouhaud spend some time with me after work hours,
> > > a,swering my questions about some parts of the code and pointed me to the
> > > excellent backtrace_functions GUC addition few days ago. This finally did
> > > the trick to find out what was happening. Many thanks Julien!
> > >
> > > 
> > >
> > > Back to the bug itself. Consider a working logical replication with
> > > constant update/insert activity, eg. pgbench running against provider.
> > >
> > > Now, on the subscriber side, a session issue an "ALTER TABLE ADD
> > > COLUMN" on a subscribed table, eg. pgbench_branches. A cache invalidation
> > > message is then pending for this table.
> > >
> > > In the meantime, the logical replication worker receive an UPDATE to
> > > apply. It opens the local relation using "logicalrep_rel_open". It finds
> > > the related entry in LogicalRepRelMap is valid, so it does not update its
> > > attrmap and directly opens and locks the local relation:
> > >  
> >
> > - /* Try to find and lock the relation by name. */
> > + /* Try to find the relation by name */
> >   relid = RangeVarGetRelid(makeRangeVar(remoterel->nspname,\
> > remoterel->relname, -1),
> > - lockmode, true);
> > + NoLock, true);
> >
> > I think we can't do this because it could lead to locking the wrong
> > reloid.  See RangeVarGetRelidExtended.  It ensures that after locking
> > the relation (which includes accepting invalidation messages), that
> > the reloid is correct.  I think changing the code in the way you are
> > suggesting can lead to locking incorrect reloid.

Sorry for the delay, I couldn't answer earlier.

To be honest, I was wondering about that. Since we keep in cache the relid and
use it as cache invalidation, I thought it might be fragile. But then - as far
as I could find - the only way to change the relid is to drop and create a new
table. I wasn't sure it could really cause a race condition there because of
the impact of such commands on logical replication.

But now, I realize I should have go all the way through and close this
potential bug as well. Thank you.

> I have made changes to fix the comment provided. The patch for the
> same is attached. Could not add a test case for this scenario is based
> on timing issue.

Thank you for this fix Vignesh!

Re: segmentation fault when cassert enabled

[Jehan-Guillaume de Rorthais]

On Mon, 16 Dec 2019 13:27:43 +0100
Peter Eisentraut  wrote:

> On 2019-12-16 11:11, Amit Kapila wrote:
> > I agree that this is a timing issue.  I also don't see a way to write
> > a reproducible test for this.  However, I could reproduce it via
> > debugger consistently by following the below steps.  I have updated a
> > few comments and commit messages in the attached patch.
> > 
> > Peter E., Petr J or anyone else, do you have comments or objections on
> > this patch?  If none, then I am planning to commit (and backpatch)
> > this patch in a few days time.  
> 
> The patch seems fine to me.  Writing a test seems hard.  Let's skip it.
> 
> The commit message has a duplicate "building"/"built" in the first sentence.

I think the sentence is quite long. I had to re-read it to get it.

What about:

  This patch allows building the local relmap cache for a subscribed relation
  after processing pending invalidation messages and potential relcache
  updates.

Regards,

Making psql error out on output failures

[Daniel Verite]

  Hi,

When exporting data with psql -c "..." >file or select ... \g file inside
psql,
post-creation output errors are silently ignored.
The problem can be seen easily by creating a small ramdisk and
filling it over capacity:

$ sudo mount -t tmpfs -o rw,size =1M tmpfs /mnt/ramdisk

$ psql -d postgres -At \
  -c "select repeat('abc', 100)" > /mnt/ramdisk/file

$ echo $?
0

$ ls -l /mnt/ramdisk/file
-rw-r--r-- 1 daniel daniel 1048576 Dec 16 15:57 /mnt/ramdisk/file

$ df -h /mnt/ramdisk/
Filesystem  Size  Used Avail Use% Mounted on
tmpfs   1.0M  1.0M 0 100% /mnt/ramdisk

The output that should be 3M byte long is truncated as expected, but
we got no error message and no error code from psql, which
is obviously not nice.

The reason is that PrintQuery() and the code below it in
fe_utils/print.c call puts(), putc(), fprintf(),... without checking their
return values or the result of ferror() on the output stream.
If we made them do that and had the printing bail out at the first error,
that would be a painful change, since there are a lot of such calls:
$ egrep -w '(fprintf|fputs|fputc)' fe_utils/print.c | wc -l
326
and the call sites are in functions that have no error reporting paths
anyway.

To start the discussion, here's a minimal patch that checks ferror() in
PrintQueryTuples() to raise an error when the printing is done
(better late than never).
The error message is generic as opposed to using errno, as 
I don't know whether errno has been clobbered at this point.
OTOH, I think that the error indicator on the output stream is not
cleared by successful writes after some other previous writes have failed.

Are there opinions on whether this should be addressed simply like
in the attached, or a large refactoring of print.c to bail out at the first
error would be better, or other ideas on how to proceed?


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9c0d53689e..993484d092 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -718,6 +718,7 @@ static bool
 PrintQueryTuples(const PGresult *results)
 {
 	printQueryOpt my_popt = pset.popt;
+	bool result = true;
 
 	/* one-shot expanded output requested via \gx */
 	if (pset.g_expanded)
@@ -736,6 +737,12 @@ PrintQueryTuples(const PGresult *results)
 
 		printQuery(results, &my_popt, fout, false, pset.logfile);
 
+		if (ferror(fout))
+		{
+			pg_log_error("Error printing tuples");
+			result = false;
+		}
+
 		if (is_pipe)
 		{
 			pclose(fout);
@@ -745,9 +752,16 @@ PrintQueryTuples(const PGresult *results)
 			fclose(fout);
 	}
 	else
+	{
 		printQuery(results, &my_popt, pset.queryFout, false, pset.logfile);
+		if (ferror(pset.queryFout))
+		{
+			pg_log_error("Error printing tuples");
+			result = false;
+		}
+	}
 
-	return true;
+	return result;
 }
 
 

Clarifying/rationalizing Vars' varno/varattno/varnoold/varoattno

[Tom Lane]

I started to think a little harder about the rough ideas I sketched
yesterday in [1] about making the planner deal with outer joins in
a less ad-hoc manner.  One thing that emerged very quickly is that
I was misremembering how the parser creates join alias Vars.
Consider for example

create table t1(a int, b int);
create table t2(x int, y int);

select a, t1.a, x, t2.x from t1 left join t2 on b = y;

The Vars that the parser will produce in the SELECT's targetlist have,
respectively,

 :varno 3 
 :varattno 1 

 :varno 1 
 :varattno 1 

 :varno 3 
 :varattno 3 

 :varno 2 
 :varattno 1 

(where "3" is the rangetable index of the unnamed join relation).
So as far as the parser is concerned, a "join alias" var is just
one that you named by referencing the join output column; it's
not tracking whether the value is one that's affected by the join
semantics.

What I'd like, in order to make progress with the planner rewrite,
is that all four Vars in the tlist have varno 3, showing that
they are (potentially) semantically distinct from the Vars in
the JOIN ON clause (which'd have varnos 1 and 2 in this example).

This is a pretty small change as far as most of the system is
concerned; there should be noplace that fails to cope with a
join alias Var, since it'd have been legal to write a join
alias Var in anyplace that would change.

However, it's a bit sticky for ruleutils.c, which needs to be
able to regurgitate these Vars in their original spellings.
(This is "needs", not "wants", because there are various
conditions under which we don't have the option of spelling
it either way.  For instance, if both tables expose columns
named "z", then you must write "t1.z" or "t2.z"; the columns
won't have unique names at the join level.)

What I'd like to do about that is redefine the existing
varnoold/varoattno fields as being the "syntactic" identifier
of the Var, versus the "semantic" identifier that varno/varattno
would be, and have ruleutils.c always use varnoold/varoattno
when trying to print a Var.

I think that this approach would greatly clarify what those fields
mean and how they should be manipulated --- for example, it makes
it clear that _equalVar() should ignore varnoold/varoattno, since
Vars with the same semantic meaning should be considered equal
even if they were spelled differently.

While at it, I'd be inclined to rename those fields, since the
existing names aren't even consistently spelled, much less meaningful.
Perhaps "varsno/varsattno" or "varnosyn/varattnosyn".

Thoughts?

regards, tom lane

[1] https://www.postgresql.org/message-id/7771.1576452845%40sss.pgh.pa.us

Re: Improvement to psql's connection defaults

[Tomas Zubiri]

To summarize possible enhancements to the current patch:

a- Don't hide failed attempted connections when defaults are used.
b- Attempt to connect via other common socket locations "/tmp".
c- New: Display the complete command used once a successful connection
has been made, so running plain psql would print
"Connecting with psql -h /var/run/postgresql" in most cases, psql -h
/tmp in others, psql -h localhost -p 5432 in others.

I could write a patch with them if it were to get implemented.

Regards.

El lun., 16 de dic. de 2019 a la(s) 12:54, Tomas Zubiri
(m...@tomaszubiri.com) escribió:
>
> Tom, Chris, thank you for your responses.
>
> > There's an excellent manpage for psql, which can also be found online:
> > https://www.postgresql.org/docs/current/app-psql.html
> > I'm a little confused as to why people don't read the documentation and
> > turn to the 'net - that's bound to dig up a lot of people who haven't
> > read the docs, too.
>
> For many users, Google is our user interface and manual, I can see by
> checking my browser history that I googled 'postgresql getting
> started' and arrived at this page '
> https://www.postgresql.org/docs/10/tutorial-accessdb.html ' which
> suggests to use psql without specifying host.
> 20 minutes later I was here
> https://www.postgresql.org/docs/12/app-psql.html  which probably means
> I found the -h and -p arguments in the manner you suggest.
>
> An alternative reason why someone would not use man psql would be if
> they don't know what the client's executable is. Suppose you come from
> mysql where the command for logging into your database was mysql, you
> can't man psql because that's the command you are looking for, you
> might google "postgresql command line client" which returns the psql
> doc page.
>
> Finally, you might google the error message that psql returned, which
> is a perfectly reasonable thing to do.
>
> > authentication options for TCP connections, even on localhost, are
> > often different from those for Unix-domain sockets (e.g. while
> > using peer authentication for administration purposes might make
>  > a lot of sense, TCP connections need some credential-based authentication
>  > so "any rogue process" cannot simply connect to your database).
>
> We already established that a tcp connection was subpar in terms of
> latency, we shall note then that a tcp connection is subpar in terms
> of security. Additionally, it is duly noted that connection via tcp
> might prompt the user for a password, which would mean that the user
> interface for psql could change depending on the connection made.
> These are not desirable qualities, but I must reiterate that these
> would only happen instead of showing the user an error. I still feel a
> subpar connection is the lesser of two evils. Additionally, it's
> already possible to have this subpar connection and differing
> interface on non-unix platforms.
> As a side note,the official postgres image doesn't require a password
> for localhost connections.
>
> > Do we have any guarantees that these containers always expose the
> > PostgreSQL server on what the host thinks is "localhost:5432"? I'm
> > thinking of network namespaces, dedicated container network interfaces
> > and all the other shenanigans. And what about the use cases of "more
> > than one container" and "database on the host and in a container"?
> > My concers is that adding more "magic" into the connection logic
> > will result in more confusion instead of less - the distinction
> > between the "default case Unix-domain socket" and "TCP" will be lost.
>
> There are answers to these questions, but since Docker containers
> don't expect programs to be docker-compliant, these are not things
> postgresql should be concerned about. What postgresql should be
> concerned about is that it was accesible via tcp on localhost at port
> 5432, and psql didn't reach it.
>
> Regarding the magic, this is a very valid concern, but I feel it's too
> late, someone other than us, (Robert Hass according to Git annotate)
> already implemented this magic, the roots of psql magic can probably
> be traced back to peer authentication even, that's some magical stuff
> that I personally appreciate. I feel like these arguments are directed
> towards the initial decision of having psql connect without arguments
> vs psql requiring -h and -p arguments (and possibly -d and -U
> parameters as well), a sailed ship.
>
> >(a) don't mix-and-match Postgres packages from different vendors,
>
> Since there's a client-server architecture here, I'm assuming that
> there's compatibility between different versions of the software. If I
> were to connect to an instance provided by an external team, I would
> expect any psql to work with any postgres server barring specific
> exceptions or wide version discrepancies.
>
> (b) adjust the server's unix_socket_directories parameter so that
> it creates a socket where your installed libpq expects to find it.
> Nope, I wanted to connect via 

Re: Improvement to psql's connection defaults

[Tomas Zubiri]

Tom, Chris, thank you for your responses.

> There's an excellent manpage for psql, which can also be found online:
> https://www.postgresql.org/docs/current/app-psql.html
> I'm a little confused as to why people don't read the documentation and
> turn to the 'net - that's bound to dig up a lot of people who haven't
> read the docs, too.

For many users, Google is our user interface and manual, I can see by
checking my browser history that I googled 'postgresql getting
started' and arrived at this page '
https://www.postgresql.org/docs/10/tutorial-accessdb.html ' which
suggests to use psql without specifying host.
20 minutes later I was here
https://www.postgresql.org/docs/12/app-psql.html  which probably means
I found the -h and -p arguments in the manner you suggest.

An alternative reason why someone would not use man psql would be if
they don't know what the client's executable is. Suppose you come from
mysql where the command for logging into your database was mysql, you
can't man psql because that's the command you are looking for, you
might google "postgresql command line client" which returns the psql
doc page.

Finally, you might google the error message that psql returned, which
is a perfectly reasonable thing to do.

> authentication options for TCP connections, even on localhost, are
> often different from those for Unix-domain sockets (e.g. while
> using peer authentication for administration purposes might make
 > a lot of sense, TCP connections need some credential-based authentication
 > so "any rogue process" cannot simply connect to your database).

We already established that a tcp connection was subpar in terms of
latency, we shall note then that a tcp connection is subpar in terms
of security. Additionally, it is duly noted that connection via tcp
might prompt the user for a password, which would mean that the user
interface for psql could change depending on the connection made.
These are not desirable qualities, but I must reiterate that these
would only happen instead of showing the user an error. I still feel a
subpar connection is the lesser of two evils. Additionally, it's
already possible to have this subpar connection and differing
interface on non-unix platforms.
As a side note,the official postgres image doesn't require a password
for localhost connections.

> Do we have any guarantees that these containers always expose the
> PostgreSQL server on what the host thinks is "localhost:5432"? I'm
> thinking of network namespaces, dedicated container network interfaces
> and all the other shenanigans. And what about the use cases of "more
> than one container" and "database on the host and in a container"?
> My concers is that adding more "magic" into the connection logic
> will result in more confusion instead of less - the distinction
> between the "default case Unix-domain socket" and "TCP" will be lost.

There are answers to these questions, but since Docker containers
don't expect programs to be docker-compliant, these are not things
postgresql should be concerned about. What postgresql should be
concerned about is that it was accesible via tcp on localhost at port
5432, and psql didn't reach it.

Regarding the magic, this is a very valid concern, but I feel it's too
late, someone other than us, (Robert Hass according to Git annotate)
already implemented this magic, the roots of psql magic can probably
be traced back to peer authentication even, that's some magical stuff
that I personally appreciate. I feel like these arguments are directed
towards the initial decision of having psql connect without arguments
vs psql requiring -h and -p arguments (and possibly -d and -U
parameters as well), a sailed ship.

>(a) don't mix-and-match Postgres packages from different vendors,

Since there's a client-server architecture here, I'm assuming that
there's compatibility between different versions of the software. If I
were to connect to an instance provided by an external team, I would
expect any psql to work with any postgres server barring specific
exceptions or wide version discrepancies.

(b) adjust the server's unix_socket_directories parameter so that
it creates a socket where your installed libpq expects to find it.
Nope, I wanted to connect via tcp, not via socket.

> I do not think your proposal would improve matters; it'd just introduce
> yet another variable, ie which transport method did libpq choose.
> As Christoph noted, that affects authentication behaviors, and there
> are a bunch of other user-visible impacts too (SSL, timeouts, ...)

This variable already exists, it just depends on the OS. Again, these
user-visible impacts would
only occur if the user would have received an error instead. Which is
the lesser of two evils?

> If we were going to do something of this sort, what I'd be inclined
> to think about is having an option to probe both of the common socket
> directory choices, rather than getting into TCP-land.  But that still
> might be a net negative from the 

Windows port minor fixes

[Ranier Vilela]

Hi,

According to microsoft documentation at:
https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom
The function CryptGenRandom is deprecated, and may can be removed in future 
release.
Considering that postgres only supports windows versions that have the new API, 
it would be good to make the replace.

BCryptGenRandom apparently works without having to set up an environment before 
calling it, allowing a simplification in the file that makes the call.
The drawback, its change causes need to link to bcrypt.lib.

On exec.c, have two memory leaks, and a possible access beyond heap bounds, the 
patch tries to fix them.
According to documentation at:
https://en.cppreference.com/w/c/experimental/dynamic/strdup
"The returned pointer must be passed to free to avoid a memory leak. "

* memory leak fix to src/common/exec.c
* CryptGenRandom change by BCryptGenRandom to src/port/pg_strong_random.c
* link bcrypt.lib to src/tools/msvc/Mkvcbuild.pm

regards,
Ranier Vileladiff --git a/src/common/exec.c b/src/common/exec.c
index 92dc3134a1..88a27cec78 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -72,14 +72,15 @@ validate_exec(const char *path)
 	int			is_x;
 
 #ifdef WIN32
-	char		path_exe[MAXPGPATH + sizeof(".exe") - 1];
+	char		path_exe[MAXPGPATH + sizeof(".exe")];
+	int path_len;
 
 	/* Win32 requires a .exe suffix for stat() */
-	if (strlen(path) >= strlen(".exe") &&
-		pg_strcasecmp(path + strlen(path) - strlen(".exe"), ".exe") != 0)
+	path_len = strlen(path);
+	if (path_len >= (sizeof(".exe") - 1) &&
+		pg_strcasecmp(path + path_len - (sizeof(".exe") - 1), ".exe") != 0)
 	{
-		strlcpy(path_exe, path, sizeof(path_exe) - 4);
-		strcat(path_exe, ".exe");
+		snprintf(path_exe, sizeof(path_exe) - 5, "%s.exe", path);
 		path = path_exe;
 	}
 #endif
@@ -600,8 +601,10 @@ set_pglocale_pgservice(const char *argv0, const char *app)
 		snprintf(env_path, sizeof(env_path), "PGLOCALEDIR=%s", path);
 		canonicalize_path(env_path + 12);
 		dup_path = strdup(env_path);
-		if (dup_path)
-			putenv(dup_path);
+		if (dup_path) {
+		putenv(dup_path);
+		free(dup_path);
+}
 	}
 #endif
 
@@ -613,8 +616,10 @@ set_pglocale_pgservice(const char *argv0, const char *app)
 		snprintf(env_path, sizeof(env_path), "PGSYSCONFDIR=%s", path);
 		canonicalize_path(env_path + 13);
 		dup_path = strdup(env_path);
-		if (dup_path)
-			putenv(dup_path);
+		if (dup_path) {
+		putenv(dup_path);
+		free(dup_path);
+}
 	}
 }
diff --git a/src/port/pg_strong_random.c b/src/port/pg_strong_random.c
index 6be5874cbf..6d075eaf1a 100644
--- a/src/port/pg_strong_random.c
+++ b/src/port/pg_strong_random.c
@@ -28,17 +28,13 @@
 #include 
 #endif
 #ifdef USE_WIN32_RANDOM
-#include 
+#include 
+#ifndef STATUS_SUCCESS
+#define STATUS_SUCCESS ((NTSTATUS)0xL)
 #endif
-
-#ifdef USE_WIN32_RANDOM
-/*
- * Cache a global crypto provider that only gets freed when the process
- * exits, in case we need random numbers more than once.
- */
-static HCRYPTPROV hProvider = 0;
 #endif
 
+
 #if defined(USE_DEV_URANDOM)
 /*
  * Read (random) bytes from a file.
@@ -85,7 +81,7 @@ random_from_file(const char *filename, void *buf, size_t len)
  * We support a number of sources:
  *
  * 1. OpenSSL's RAND_bytes()
- * 2. Windows' CryptGenRandom() function
+ * 2. Windows' BCryptGenRandom() function
  * 3. /dev/urandom
  *
  * The configure script will choose which one to use, and set
@@ -140,28 +136,8 @@ pg_strong_random(void *buf, size_t len)
 	 * Windows has CryptoAPI for strong cryptographic numbers.
 	 */
 #elif defined(USE_WIN32_RANDOM)
-	if (hProvider == 0)
-	{
-		if (!CryptAcquireContext(&hProvider,
- NULL,
- MS_DEF_PROV,
- PROV_RSA_FULL,
- CRYPT_VERIFYCONTEXT | CRYPT_SILENT))
-		{
-			/*
-			 * On failure, set back to 0 in case the value was for some reason
-			 * modified.
-			 */
-			hProvider = 0;
-		}
-	}
-	/* Re-check in case we just retrieved the provider */
-	if (hProvider != 0)
-	{
-		if (CryptGenRandom(hProvider, len, buf))
-			return true;
-	}
-	return false;
+	return (BCryptGenRandom(NULL, buf, len,
+BCRYPT_USE_SYSTEM_PREFERRED_RNG) == STATUS_SUCCESS);
 
 	/*
 	 * Read /dev/urandom ourselves.
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 275f3bb699..9e1ac51507 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -65,7 +65,7 @@ my @frontend_uselibpgcommon = (
 my $frontend_extralibs = {
 	'initdb' => ['ws2_32.lib'],
 	'pg_restore' => ['ws2_32.lib'],
-	'pgbench'=> ['ws2_32.lib'],
+	'pgbench'=> ['ws2_32.lib', 'bcrypt.lib'],
 	'psql'   => ['ws2_32.lib']
 };
 my $frontend_extraincludes = {
@@ -184,6 +184,7 @@ sub mkvcbuild
 	$postgres->AddFiles('src/backend/utils/adt', 'jsonpath_scan.l',
 		'jsonpath_gram.y');
 	$postgres->AddDefine('BUILDING_DLL');
+	$postgres->AddLibrary('bcrypt.lib');
 	$postgres->AddLibrary('secur32.lib');
 	$postgres->AddLi

ERROR: could not resize shared memory segment...No space left on device

[Justin Pryzby]

A customer's report query hit this error.
ERROR:  could not resize shared memory segment "/PostgreSQL.2011322019" to 
134217728 bytes: No space left on device

I found:
https://www.postgresql.org/message-id/flat/CAEepm%3D2D_JGb8X%3DLa-0PX9C8dBX9%3Dj9wY%2By1-zDWkcJu0%3DBQbA%40mail.gmail.com

work_mem | 128MB
dynamic_shared_memory_type | posix
version | PostgreSQL 12.1 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.4.7 
20120313 (Red Hat 4.4.7-23), 64-bit
Running centos 6.9 / linux 2.6.32-696.23.1.el6.x86_64

$ free -m
 total   used   free sharedbuffers cached
Mem:  7871   7223648   1531  5   1988
-/+ buffers/cache:   5229   2642
Swap: 4095   2088   2007

$ mount | grep /dev/shm
tmpfs on /dev/shm type tmpfs (rw)

$ du -hs /dev/shm
0   /dev/shm

$ df /dev/shm
Filesystem 1K-blocks  Used Available Use% Mounted on
tmpfs403027224   4030248   1% /dev/shm

Later, I see:
$ df -h /dev/shm
Filesystem  Size  Used Avail Use% Mounted on
tmpfs   3.9G  3.3G  601M  85% /dev/shm

I can reproduce the error running a single instance of the query.

The query plan is 1300 lines long, and involves 482 "Scan" nodes on a table
which currently has 93 partitions, and for which current partitions are
"daily".  I believe I repartitioned its history earlier this year to "monthly",
probably to avoid "OOM with many sorts", as reported here:
https://www.postgresql.org/message-id/20190708164401.GA22387%40telsasoft.com

$ grep Scan tmp/sql-`date +%F`.1.ex |sed 's/^ *//; s/ on .*//' |sort |uniq -c 
|sort -nr 
227 ->  Parallel Bitmap Heap Scan
227 ->  Bitmap Index Scan
 14 ->  Parallel Seq Scan
  9 ->  Seq Scan
  2 ->  Subquery Scan
  2 ->  Index Scan using sites_pkey
  1 Subquery Scan

There are total of 10 "Workers Planned":
grep -o 'Worker.*' tmp/sql-`date +%F`.1.ex
Workers Planned: 2
Workers Planned: 2
Workers Planned: 2
Workers Planned: 2
Workers Planned: 2

I will plan to repartition again to month granularity unless someone wants to
collect further information or suggest a better solution.

(gdb) bt
#0  pg_re_throw () at elog.c:1717
#1  0x00886194 in errfinish (dummy=) at elog.c:464
#2  0x00749453 in dsm_impl_posix (op=, 
handle=, request_size=, 
impl_private=, mapped_address=, 
mapped_size=, elevel=20) at dsm_impl.c:283
#3  dsm_impl_op (op=, handle=, 
request_size=, impl_private=, 
mapped_address=, mapped_size=, 
elevel=20) at dsm_impl.c:170
#4  0x0074a7c8 in dsm_create (size=100868096, flags=0) at dsm.c:459
#5  0x008a94a6 in make_new_segment (area=0x1d70208, 
requested_pages=) at dsa.c:2156
#6  0x008aa47a in dsa_allocate_extended (area=0x1d70208, 
size=100663304, flags=5) at dsa.c:712
#7  0x00670b3f in pagetable_allocate (pagetable=, 
size=) at tidbitmap.c:1511
#8  0x0067200c in pagetable_grow (tbm=0x7f82274da8e8, pageno=906296)
at ../../../src/include/lib/simplehash.h:405
#9  pagetable_insert (tbm=0x7f82274da8e8, pageno=906296)
at ../../../src/include/lib/simplehash.h:530
#10 tbm_get_pageentry (tbm=0x7f82274da8e8, pageno=906296) at tidbitmap.c:1225
#11 0x006724a0 in tbm_add_tuples (tbm=0x7f82274da8e8, 
tids=, ntids=1, recheck=false) at tidbitmap.c:405
#12 0x004d7f1f in btgetbitmap (scan=0x1d7d948, tbm=0x7f82274da8e8)
at nbtree.c:334
#13 0x004d103a in index_getbitmap (scan=0x1d7d948, 
bitmap=) at indexam.c:665
#14 0x006323d8 in MultiExecBitmapIndexScan (node=0x1dcbdb8)
at nodeBitmapIndexscan.c:105
#15 0x006317f4 in BitmapHeapNext (node=0x1d8a030)
at nodeBitmapHeapscan.c:141
#16 0x0062405c in ExecScanFetch (node=0x1d8a030, 
accessMtd=0x6316d0 , 
recheckMtd=0x631440 ) at execScan.c:133
#17 ExecScan (node=0x1d8a030, accessMtd=0x6316d0 , 
recheckMtd=0x631440 ) at execScan.c:200
#18 0x00622900 in ExecProcNodeInstr (node=0x1d8a030)
at execProcnode.c:461
#19 0x0062c66f in ExecProcNode (pstate=0x1d7ad70)
at ../../../src/include/executor/executor.h:239
#20 ExecAppend (pstate=0x1d7ad70) at nodeAppend.c:292
#21 0x00622900 in ExecProcNodeInstr (node=0x1d7ad70)
at execProcnode.c:461
#22 0x00637da2 in ExecProcNode (pstate=0x1d7a630)
at ../../../src/include/executor/executor.h:239
#23 ExecHashJoinOuterGetTuple (pstate=0x1d7a630) at nodeHashjoin.c:833
#24 ExecHashJoinImpl (pstate=0x1d7a630) at nodeHashjoin.c:356
#25 ExecHashJoin (pstate=0x1d7a630) at nodeHashjoin.c:572
#26 0x00622900 in ExecProcNodeInstr (node=0x1d7a630)
at execProcnode.c:461
#27 0x00637da2 in ExecProcNode (pstate=0x1d7bff0)
at ../../../src/include/executor/executor.h:239
#28 ExecHashJoinOuterGetTuple (pstate=0x1d7bff0) at nodeHashjoin.c:833
#29 ExecHashJoinImpl (pstate=0x1d7bff0) at nodeHashjoin.c:356
#30 ExecHashJoin (pstate=0x1d7bff0) at nodeHashjoin.c:572
#31 0x00622900 

polymorphic table functions light

[Peter Eisentraut]

I want to address the issue that calling a record-returning function 
always requires specifying a  result column list, even though there are 
cases where the function could be self-aware enough to know the result 
column list of a particular call.  For example, most of the functions in 
contrib/tablefunc are like that.


SQL:2016 has a feature called polymorphic table functions (PTF) that 
addresses this.  The full PTF feature is much larger, so I just carved 
out this particular piece of functionality.  Here is a link to some 
related information: 
https://modern-sql.com/blog/2018-11/whats-new-in-oracle-database-18c#ptf


The idea is that you attach a helper function to the main function.  The 
helper function is called at parse time with the constant arguments of 
the main function call and can compute a result row description (a 
TupleDesc in our case).


Example from the patch:

CREATE FUNCTION connectby_describe(internal)
RETURNS internal
AS 'MODULE_PATHNAME', 'connectby_describe'
LANGUAGE C;

CREATE FUNCTION connectby(text,text,text,text,int,text)
RETURNS setof record
DESCRIBE WITH connectby_describe
AS 'MODULE_PATHNAME','connectby_text'
LANGUAGE C STABLE STRICT;

(The general idea is very similar to Pavel's patch "parse time support 
function"[0] but addressing a disjoint problem.)


The original SQL:2016 syntax is a bit different: There, you'd first 
create two separate functions: a "describe" and a "fulfill" and then 
create the callable PTF referencing those two (similar to how an 
aggregate is composed of several component functions).  I think 
deviating from this makes some sense because we can then more easily 
"upgrade" existing record-returning functions with this functionality.


Another difference is that AFAICT, the standard specifies that if the 
describe function cannot resolve the call, the call fails.  Again, in 
order to be able to upgrade existing functions (instead of having to 
create a second set of functions with a different name), I have made it 
so that you can still specify an explicit column list if the describe 
function does not succeed.


In this prototype patch, I have written the C interface and several 
examples using existing functions in the source tree.  Eventually, I'd 
like to also add PL-level support for this.


Thoughts so far?


[0]: 
https://www.postgresql.org/message-id/flat/CAFj8pRARh+r4=hnwq+hws-d6msus01dw_6zjnyur6tpk1+w...@mail.gmail.com


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From c517222f5de87961e0cebd91efc5ebfea7737888 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 16 Dec 2019 18:49:10 +0100
Subject: [PATCH v1] Polymorphic table functions

---
 contrib/tablefunc/expected/tablefunc.out  | 46 +++
 contrib/tablefunc/sql/tablefunc.sql   |  8 +++
 contrib/tablefunc/tablefunc--1.0.sql  |  7 +++
 contrib/tablefunc/tablefunc.c | 69 ++
 contrib/xml2/expected/xml2.out| 10 
 contrib/xml2/sql/xml2.sql |  6 ++
 contrib/xml2/xml2--1.1.sql|  6 ++
 contrib/xml2/xpath.c  | 72 +++
 doc/src/sgml/catalogs.sgml| 12 
 doc/src/sgml/queries.sgml |  8 +++
 doc/src/sgml/ref/create_function.sgml | 14 +
 doc/src/sgml/xfunc.sgml   | 66 +
 src/backend/catalog/pg_aggregate.c|  1 +
 src/backend/catalog/pg_proc.c | 11 
 src/backend/commands/functioncmds.c   | 30 +-
 src/backend/commands/proclang.c   |  3 +
 src/backend/commands/typecmds.c   |  1 +
 src/backend/executor/execSRF.c|  1 +
 src/backend/executor/nodeFunctionscan.c   |  1 +
 src/backend/optimizer/prep/prepjointree.c |  1 +
 src/backend/parser/gram.y |  7 ++-
 src/backend/parser/parse_relation.c   |  3 +
 src/backend/utils/adt/jsonfuncs.c | 48 +++
 src/backend/utils/fmgr/funcapi.c  | 49 ++-
 src/include/catalog/pg_class.dat  |  2 +-
 src/include/catalog/pg_proc.dat   |  6 +-
 src/include/catalog/pg_proc.h |  4 ++
 src/include/funcapi.h |  1 +
 src/include/parser/kwlist.h   |  1 +
 src/interfaces/ecpg/preproc/ecpg.tokens   |  2 +-
 src/interfaces/ecpg/preproc/ecpg.trailer  | 11 ++--
 src/interfaces/ecpg/preproc/ecpg_kwlist.h |  1 -
 src/test/regress/expected/json.out|  6 ++
 src/test/regress/sql/json.sql |  2 +
 34 files changed, 503 insertions(+), 13 deletions(-)

diff --git a/contrib/tablefunc/expected/tablefunc.out 
b/contrib/tablefunc/expected/tablefunc.out
index fffadc6e1b..485ddfba87 100644
--- a/contrib/tablefunc/expected/tablefunc.out
+++ b/contrib/tablefunc/expected/tablefunc.out
@@ -328,6 +328,29 @@ SELECT * FROM connectby('connectby_text', 'keyid', 
'parent_keyid', 'pos', 'row2'
  row8  | row6  

Re: Windows port minor fixes

[Juan José Santamaría Flecha]

On Mon, Dec 16, 2019 at 6:34 PM Ranier Vilela 
wrote:

>
> Considering that postgres only supports windows versions that have the new
> API, it would be good to make the replace.
>
>
That is not actually the case. If you check the _WIN32_WINNT logic
in src/include/port/win32.h you can see that depending on your building
tools you can get a version lower than that, for example if using MinGW.


>
> * memory leak fix to src/common/exec.c
> * CryptGenRandom change by BCryptGenRandom to src/port/pg_strong_random.c
> * link bcrypt.lib to src/tools/msvc/Mkvcbuild.pm
>
>
If you want to address 2 unrelated issues, it makes little sense to use a
single thread and 3 patches.

Regards,

Juan José Santamaría Flecha

Re: polymorphic table functions light

[Pavel Stehule]

Hi

po 16. 12. 2019 v 19:53 odesílatel Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> napsal:

> I want to address the issue that calling a record-returning function
> always requires specifying a  result column list, even though there are
> cases where the function could be self-aware enough to know the result
> column list of a particular call.  For example, most of the functions in
> contrib/tablefunc are like that.
>
> SQL:2016 has a feature called polymorphic table functions (PTF) that
> addresses this.  The full PTF feature is much larger, so I just carved
> out this particular piece of functionality.  Here is a link to some
> related information:
> https://modern-sql.com/blog/2018-11/whats-new-in-oracle-database-18c#ptf
>
> The idea is that you attach a helper function to the main function.  The
> helper function is called at parse time with the constant arguments of
> the main function call and can compute a result row description (a
> TupleDesc in our case).
>
> Example from the patch:
>
> CREATE FUNCTION connectby_describe(internal)
> RETURNS internal
> AS 'MODULE_PATHNAME', 'connectby_describe'
> LANGUAGE C;
>
> CREATE FUNCTION connectby(text,text,text,text,int,text)
> RETURNS setof record
> DESCRIBE WITH connectby_describe
> AS 'MODULE_PATHNAME','connectby_text'
> LANGUAGE C STABLE STRICT;
>
> (The general idea is very similar to Pavel's patch "parse time support
> function"[0] but addressing a disjoint problem.)
>
> The original SQL:2016 syntax is a bit different: There, you'd first
> create two separate functions: a "describe" and a "fulfill" and then
> create the callable PTF referencing those two (similar to how an
> aggregate is composed of several component functions).  I think
> deviating from this makes some sense because we can then more easily
> "upgrade" existing record-returning functions with this functionality.
>
> Another difference is that AFAICT, the standard specifies that if the
> describe function cannot resolve the call, the call fails.  Again, in
> order to be able to upgrade existing functions (instead of having to
> create a second set of functions with a different name), I have made it
> so that you can still specify an explicit column list if the describe
> function does not succeed.
>
> In this prototype patch, I have written the C interface and several
> examples using existing functions in the source tree.  Eventually, I'd
> like to also add PL-level support for this.
>
> Thoughts so far?
>

What I read about it - it can be very interesting feature. It add lot of
dynamic to top queries - it can be used very easy for cross tables on
server side.

Sure - it can be used very badly - but it is nothing new for stored
procedures.

Personally I like this feature. The difference from standard syntax
probably is not problem a) there are little bit syntax already, b) I cannot
to imagine wide using of this feature. But it can be interesting for
extensions.

Better to use some special pseudotype for describe function instead
"internal" - later it can interesting for PL support

Regards

Pavel




>
>
> [0]:
>
> https://www.postgresql.org/message-id/flat/CAFj8pRARh+r4=hnwq+hws-d6msus01dw_6zjnyur6tpk1+w...@mail.gmail.com
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: non-exclusive backup cleanup is mildly broken

[Robert Haas]

On Sun, Dec 15, 2019 at 8:44 PM Kyotaro Horiguchi
 wrote:
> However I don't object to the restriction, couldn't we allow the
> cancel_before_shmem_exit to search for the given entry looping over
> the before_shmem_exit array? If we don't do that, an assrtion is needed
> instead.
>
> Since pg_stop_backup_v2 is the only caller to the function in the
> whole server code, making cancel_before_shmem_exit a bit wiser (and
> slower) cannot hurt anyone.

That's actually not true. It's called from
PG_END_ENSURE_ERROR_CLEANUP. Still, it wouldn't cost a lot to fix this
that way. However, I think that it's better to fix it the other way,
as I mentioned in my original email.

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

Re: Clarifying/rationalizing Vars' varno/varattno/varnoold/varoattno

[Robert Haas]

On Mon, Dec 16, 2019 at 12:00 PM Tom Lane  wrote:
> What I'd like, in order to make progress with the planner rewrite,
> is that all four Vars in the tlist have varno 3, showing that
> they are (potentially) semantically distinct from the Vars in
> the JOIN ON clause (which'd have varnos 1 and 2 in this example).
>
> This is a pretty small change as far as most of the system is
> concerned; there should be noplace that fails to cope with a
> join alias Var, since it'd have been legal to write a join
> alias Var in anyplace that would change.

I don't have an opinion about the merits of this change, but I'm
curious how this manages to work. It seems like there would be a fair
number of places that needed to map the join alias var back to some
baserel that can supply it. And it seems like there could be multiple
levels of join alias vars as well, since you could have joins nested
inside of other joins, possibly with subqueries involved.

At some point I had the idea that it might make sense to have
equivalence classes that had both a list of full members (which are
exactly equivalent) and nullable members (which are either equivalent
or null). I'm not sure whether that idea is of any practical use,
though. It does seems strange to me that the representation you are
proposing gets at the question only indirectly. The nullable version
of the Var has got a different varno and varattno than the
non-nullable version of the Var, but other than that there's no
connection between them. How do you go about matching those together?
I guess varnoold/varoattno can do the trick, but if that's only being
used by ruleutils.c then there must be some other mechanism.

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

Re: polymorphic table functions light

[Tom Lane]

Peter Eisentraut  writes:
> I want to address the issue that calling a record-returning function 
> always requires specifying a  result column list, even though there are 
> cases where the function could be self-aware enough to know the result 
> column list of a particular call.  For example, most of the functions in 
> contrib/tablefunc are like that.

Seems like a reasonable goal.

> SQL:2016 has a feature called polymorphic table functions (PTF) that 
> addresses this.  The full PTF feature is much larger, so I just carved 
> out this particular piece of functionality.  Here is a link to some 
> related information: 
> https://modern-sql.com/blog/2018-11/whats-new-in-oracle-database-18c#ptf

> The idea is that you attach a helper function to the main function.  The 
> helper function is called at parse time with the constant arguments of 
> the main function call and can compute a result row description (a 
> TupleDesc in our case).

> Example from the patch:

> CREATE FUNCTION connectby_describe(internal)
> RETURNS internal
> AS 'MODULE_PATHNAME', 'connectby_describe'
> LANGUAGE C;

> CREATE FUNCTION connectby(text,text,text,text,int,text)
> RETURNS setof record
> DESCRIBE WITH connectby_describe
> AS 'MODULE_PATHNAME','connectby_text'
> LANGUAGE C STABLE STRICT;

> (The general idea is very similar to Pavel's patch "parse time support 
> function"[0] but addressing a disjoint problem.)

Hm.  Given that this involves a function-taking-and-returning-internal,
I think it's fairly silly to claim that it is implementing a SQL-standard
feature, or even a subset or related feature.  Nor do I see a pathway
whereby this might end in a feature you could use without writing C code.

That being the case, I'm not in favor of using up SQL syntax space for it
if we don't have to.  Moreover, this approach requires a whole lot of
duplicative-seeming new infrastructure, such as a new pg_proc column.
And you're not even done yet --- where's the pg_dump support?

I think we'd be better off to address this by extending the existing
"support function" infrastructure by inventing a new support request type,
much as Pavel's patch did.  I've not gotten around to reviewing the latest
version of his patch, so I'm not sure if it provides enough flexibility to
solve this particular problem, or if we'd need a different request type
than he proposes.  But I'd rather go down that path than this one.
It should provide the same amount of functionality with a whole lot less
overhead code.

regards, tom lane

[PATCH] Windows port add support to BCryptGenRandom

[Ranier Vilela]

Hi,
According to microsoft documentation at:
https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom
The function CryptGenRandom is deprecated, and may can be removed in future 
release.
This patch add support to use BCryptGenRandom.

BCryptGenRandom apparently works without having to set up an environment before 
calling.
The drawback, its change causes need to link to bcrypt.lib.

regards,
Ranier Vileladiff --git a/src/port/pg_strong_random.c b/src/port/pg_strong_random.c
index 6be5874cbf..8199f89a37 100644
--- a/src/port/pg_strong_random.c
+++ b/src/port/pg_strong_random.c
@@ -28,10 +28,20 @@
 #include 
 #endif
 #ifdef USE_WIN32_RANDOM
-#include 
+#if defined(_MSC_VER) && _MSC_VER >= 1900 \
+ && defined(MIN_WINNT) && MIN_WINNT >= 0x0600
+#define USE_WIN32_BCRYPTGENRANDOM
+#endif
 #endif
 
-#ifdef USE_WIN32_RANDOM
+#ifdef USE_WIN32_BCRYPTGENRANDOM
+#include 
+#ifndef STATUS_SUCCESS
+  #define STATUS_SUCCESS ((NTSTATUS)0xL)
+#endif
+#elif USE_WIN32_RANDOM
+#include 
+
 /*
  * Cache a global crypto provider that only gets freed when the process
  * exits, in case we need random numbers more than once.
@@ -85,8 +95,9 @@ random_from_file(const char *filename, void *buf, size_t len)
  * We support a number of sources:
  *
  * 1. OpenSSL's RAND_bytes()
- * 2. Windows' CryptGenRandom() function
- * 3. /dev/urandom
+ * 2. Windows' BCryptGenRandom() function
+ * 3. Windows' CryptGenRandom() function
+ * 4. /dev/urandom
  *
  * The configure script will choose which one to use, and set
  * a USE_*_RANDOM flag accordingly.
@@ -139,6 +150,10 @@ pg_strong_random(void *buf, size_t len)
 	/*
 	 * Windows has CryptoAPI for strong cryptographic numbers.
 	 */
+#elif defined(USE_WIN32_BCRYPTGENRANDOM)
+	return (BCryptGenRandom(NULL, buf, len,
+BCRYPT_USE_SYSTEM_PREFERRED_RNG) == STATUS_SUCCESS);
+
 #elif defined(USE_WIN32_RANDOM)
 	if (hProvider == 0)
 	{

[PATCH] Memory leak, at src/common/exec.c

[Ranier Vilela]

Hi,
On exec.c, have two memory leaks, and a possible access beyond heap bounds, the 
patch tries to fix them.
According to documentation at:
https://en.cppreference.com/w/c/experimental/dynamic/strdup
"The returned pointer must be passed to free to avoid a memory leak. "

regards,
Ranier Vileladiff --git a/src/common/exec.c b/src/common/exec.c
index 92dc3134a1..88a27cec78 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -72,14 +72,15 @@ validate_exec(const char *path)
 	int			is_x;
 
 #ifdef WIN32
-	char		path_exe[MAXPGPATH + sizeof(".exe") - 1];
+	char		path_exe[MAXPGPATH + sizeof(".exe")];
+	int path_len;
 
 	/* Win32 requires a .exe suffix for stat() */
-	if (strlen(path) >= strlen(".exe") &&
-		pg_strcasecmp(path + strlen(path) - strlen(".exe"), ".exe") != 0)
+	path_len = strlen(path);
+	if (path_len >= (sizeof(".exe") - 1) &&
+		pg_strcasecmp(path + path_len - (sizeof(".exe") - 1), ".exe") != 0)
 	{
-		strlcpy(path_exe, path, sizeof(path_exe) - 4);
-		strcat(path_exe, ".exe");
+		snprintf(path_exe, sizeof(path_exe) - 5, "%s.exe", path);
 		path = path_exe;
 	}
 #endif
@@ -600,8 +601,10 @@ set_pglocale_pgservice(const char *argv0, const char *app)
 		snprintf(env_path, sizeof(env_path), "PGLOCALEDIR=%s", path);
 		canonicalize_path(env_path + 12);
 		dup_path = strdup(env_path);
-		if (dup_path)
-			putenv(dup_path);
+		if (dup_path) {
+		putenv(dup_path);
+		free(dup_path);
+}
 	}
 #endif
 
@@ -613,8 +616,10 @@ set_pglocale_pgservice(const char *argv0, const char *app)
 		snprintf(env_path, sizeof(env_path), "PGSYSCONFDIR=%s", path);
 		canonicalize_path(env_path + 13);
 		dup_path = strdup(env_path);
-		if (dup_path)
-			putenv(dup_path);
+		if (dup_path) {
+		putenv(dup_path);
+		free(dup_path);
+}
 	}
 }

RE: [PATCH] Windows port add support to BCryptGenRandom

[Ranier Vilela]

Forget Mkvcbuild_v1.patch

regards,
Ranier Vileladiff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 275f3bb699..33dc9bf5ad 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -65,7 +65,7 @@ my @frontend_uselibpgcommon = (
 my $frontend_extralibs = {
 	'initdb' => ['ws2_32.lib'],
 	'pg_restore' => ['ws2_32.lib'],
-	'pgbench'=> ['ws2_32.lib'],
+	'pgbench'=> ['ws2_32.lib', 'bcrypt.lib'],
 	'psql'   => ['ws2_32.lib']
 };
 my $frontend_extraincludes = {
@@ -184,6 +184,7 @@ sub mkvcbuild
 	$postgres->AddFiles('src/backend/utils/adt', 'jsonpath_scan.l',
 		'jsonpath_gram.y');
 	$postgres->AddDefine('BUILDING_DLL');
+	$postgres->AddLibrary('bcrypt.lib') if ($vsVersion > '12.00');
 	$postgres->AddLibrary('secur32.lib');
 	$postgres->AddLibrary('ws2_32.lib');
 	$postgres->AddLibrary('wldap32.lib') if ($solution->{options}->{ldap});
@@ -246,6 +247,7 @@ sub mkvcbuild
 	$libpq->AddDefine('FRONTEND');
 	$libpq->AddDefine('UNSAFE_STAT_OK');
 	$libpq->AddIncludeDir('src/port');
+	$libpq->AddLibrary('bcrypt.lib')  if ($vsVersion > '12.00');
 	$libpq->AddLibrary('secur32.lib');
 	$libpq->AddLibrary('ws2_32.lib');
 	$libpq->AddLibrary('wldap32.lib') if ($solution->{options}->{ldap});

Re: [PATCH] Memory leak, at src/common/exec.c

[Mark Dilger]

On 12/16/19 1:22 PM, Ranier Vilela wrote:

Hi,
On exec.c, have two memory leaks, and a possible access beyond heap bounds, the 
patch tries to fix them.
According to documentation at:
https://en.cppreference.com/w/c/experimental/dynamic/strdup
"The returned pointer must be passed to free to avoid a memory leak."


Please see the man page for putenv.  Are you certain it is safe to
free the string passed to putenv after putenv returns?  I think this
may be implemented differently on various platforms.

Taken from `man putenv`:

"NOTES
   The putenv() function is not required to be reentrant, and the 
one in glibc 2.0 is not, but the glibc 2.1 version is.


   Since  version  2.1.2,  the  glibc  implementation  conforms to 
SUSv2: the pointer string given to putenv() is used.  In particular, 
this string becomes part of the environment; changing it later will
   change the environment.  (Thus, it is an error is to call 
putenv() with an automatic variable as the argument, then return from 
the calling function while string is still  part  of  the  environment.)
   However, glibc versions 2.0 to 2.1.1 differ: a copy of the 
string is used.  On the one hand this causes a memory leak, and on the 
other hand it violates SUSv2.


   The 4.4BSD version, like glibc 2.0, uses a copy.

   SUSv2 removes the const from the prototype, and so does glibc 2.1.3.
"

--
Mark Dilger

Re: Clarifying/rationalizing Vars' varno/varattno/varnoold/varoattno

[Tom Lane]

Robert Haas  writes:
> On Mon, Dec 16, 2019 at 12:00 PM Tom Lane  wrote:
>> What I'd like, in order to make progress with the planner rewrite,
>> is that all four Vars in the tlist have varno 3, showing that
>> they are (potentially) semantically distinct from the Vars in
>> the JOIN ON clause (which'd have varnos 1 and 2 in this example).

> I don't have an opinion about the merits of this change, but I'm
> curious how this manages to work. It seems like there would be a fair
> number of places that needed to map the join alias var back to some
> baserel that can supply it. And it seems like there could be multiple
> levels of join alias vars as well, since you could have joins nested
> inside of other joins, possibly with subqueries involved.

Sure.  Right now, we smash join aliases down to the ultimately-referenced
base vars early in planning (see flatten_join_alias_vars).  After the
patch that I'm proposing right now, that would continue to be the case,
so there'd be little change in most of the planner from this.  However,
the later changes that I speculated about in the other thread would
involve delaying that smashing in cases where the join output value is
possibly different from the input value, so that we would have a clear
representational distinction between those things, something we lack
today.

> At some point I had the idea that it might make sense to have
> equivalence classes that had both a list of full members (which are
> exactly equivalent) and nullable members (which are either equivalent
> or null).

Yeah, this is another way that you might get at the problem, but it
seems to me it's not really addressing the fundamental squishiness.
If the "nullable members" might be null, then what semantics are
you promising exactly?  You certainly haven't got anything that
defines a sort order for them.

> I'm not sure whether that idea is of any practical use,
> though. It does seems strange to me that the representation you are
> proposing gets at the question only indirectly. The nullable version
> of the Var has got a different varno and varattno than the
> non-nullable version of the Var, but other than that there's no
> connection between them. How do you go about matching those together?

You'd have to look into the join's joinaliasvars list (or more likely,
some new planner data structure derived from that) to discover that
there's any connection.  That seems fine to me, because AFAICS
relatively few places would need to do that.  It's certainly better
than using a representation that suggests that two values are the same
when they're not.  (TBH, I've spent the last dozen years waiting for
someone to come up with an example that completely breaks equivalence
classes, if not our entire approach to outer joins.  So far we've been
able to work around every case, but we've sometimes had to give up on
optimizations that would be nice to have.)

A related example that is bugging me is that the grouping-sets patch
broke the meaning of Vars that represent post-grouping values ---
there again, the value might have gone to null as a result of grouping,
but you can't tell it apart from values that haven't.  I think this is
less critical because such Vars can't appear in FROM/WHERE so they're
of little interest to most of the planner, but we've still had to put
in kluges like 90947674f because of that.  We might be well advised
to invent some join-alias-like mechanism for those.  (I have a vague
memory now that Gierth wanted to do something like that and I
discouraged it because it was unlike the way we did outer joins ...
so he was right, but what we should have done was fix outer joins not
double down on the kluge.)

> I guess varnoold/varoattno can do the trick, but if that's only being
> used by ruleutils.c then there must be some other mechanism.

Actually, they're nothing but debug support currently --- ruleutils
doesn't use them either.  It's possible that this change would allow
ruleutils to save cycles in a lot of cases by not having to drill down
through subplans to identify the ultimate referent of upper-plan Vars.
But I haven't investigated that yet.

regards, tom lane

Re: ERROR: could not resize shared memory segment...No space left on device

[Tomas Vondra]

On Mon, Dec 16, 2019 at 12:49:06PM -0600, Justin Pryzby wrote:

A customer's report query hit this error.
ERROR:  could not resize shared memory segment "/PostgreSQL.2011322019" to 
134217728 bytes: No space left on device

I found:
https://www.postgresql.org/message-id/flat/CAEepm%3D2D_JGb8X%3DLa-0PX9C8dBX9%3Dj9wY%2By1-zDWkcJu0%3DBQbA%40mail.gmail.com

work_mem | 128MB
dynamic_shared_memory_type | posix
version | PostgreSQL 12.1 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.4.7 
20120313 (Red Hat 4.4.7-23), 64-bit
Running centos 6.9 / linux 2.6.32-696.23.1.el6.x86_64

$ free -m
total   used   free sharedbuffers cached
Mem:  7871   7223648   1531  5   1988
-/+ buffers/cache:   5229   2642
Swap: 4095   2088   2007

$ mount | grep /dev/shm
tmpfs on /dev/shm type tmpfs (rw)

$ du -hs /dev/shm
0   /dev/shm

$ df /dev/shm
Filesystem 1K-blocks  Used Available Use% Mounted on
tmpfs403027224   4030248   1% /dev/shm

Later, I see:
$ df -h /dev/shm
Filesystem  Size  Used Avail Use% Mounted on
tmpfs   3.9G  3.3G  601M  85% /dev/shm

I can reproduce the error running a single instance of the query.

The query plan is 1300 lines long, and involves 482 "Scan" nodes on a table
which currently has 93 partitions, and for which current partitions are
"daily".  I believe I repartitioned its history earlier this year to "monthly",
probably to avoid "OOM with many sorts", as reported here:
https://www.postgresql.org/message-id/20190708164401.GA22387%40telsasoft.com



Interestingly enough, I ran into the same ERROR (not sure if the same
root cause) while investigating bug #16104 [1], i.e. on a much simpler
query (single join).

This This particular machine is a bit smaller (only 8GB of RAM and less
disk space) so I created a smaller table with "just" 1.5B rows:

  create table test as select generate_series(1, 15)::bigint i;
  set work_mem = '150MB';
  set max_parallel_workers_per_gather = 8;
  analyze test;

  explain select count(*) from test t1 join test t2 using (i);

 QUERY PLAN

 Finalize Aggregate  (cost=67527436.36..67527436.37 rows=1 width=8)
   ->  Gather  (cost=67527435.53..67527436.34 rows=8 width=8)
 Workers Planned: 8
 ->  Partial Aggregate  (cost=67526435.53..67526435.54 rows=1 width=8)
   ->  Parallel Hash Join  (cost=11586911.03..67057685.47 
rows=187500024 width=0)
 Hash Cond: (t1.i = t2.i)
 ->  Parallel Seq Scan on test t1  (cost=0.00..8512169.24 
rows=187500024 width=8)
 ->  Parallel Hash  (cost=8512169.24..8512169.24 
rows=187500024 width=8)
   ->  Parallel Seq Scan on test t2  
(cost=0.00..8512169.24 rows=187500024 width=8)
(9 rows)

  explain analyze select count(*) from test t1 join test t2 using (i);
  
  ERROR:  could not resize shared memory segment "/PostgreSQL.1743102822" to 536870912 bytes: No space left on device


Now, work_mem = 150MB might be a bit too high considering the machine
only has 8GB of RAM (1GB of which is shared_buffers). But that's still
just 1.2GB of RAM and this is not an OOM. This actually fills the
/dev/shm mount, which is limited to 4GB on this box

  bench ~ # df | grep shm
  shm  3994752   16   3994736   1% /dev/shm

So somewhere in the parallel hash join, we allocate 4GB of shared segments ...

The filesystem usage from the moment of the query execution to the
failure looks about like this:

Time fs 1K-blocks  Used Available Use% Mounted on
--
10:13:34shm   3994752 34744   3960008   1% /dev/shm
10:13:35shm   3994752 35768   3958984   1% /dev/shm
10:13:36shm   3994752 37816   3956936   1% /dev/shm
10:13:39shm   3994752 39864   3954888   1% /dev/shm
10:13:42shm   3994752 41912   3952840   2% /dev/shm
10:13:46shm   3994752 43960   3950792   2% /dev/shm
10:13:49shm   3994752 48056   3946696   2% /dev/shm
10:13:56shm   3994752 52152   3942600   2% /dev/shm
10:14:02shm   3994752 56248   3938504   2% /dev/shm
10:14:09shm   3994752 60344   3934408   2% /dev/shm
10:14:16shm   3994752 68536   3926216   2% /dev/shm
10:14:30shm   3994752 76728   3918024   2% /dev/shm
10:14:43shm   3994752 84920   3909832   3% /dev/shm
10:14:43shm   3994752 84920   3909832   3% /dev/shm
10:14:57shm   3994752 93112   3901640   3% /dev/shm
10:15:11shm   3994752109496   3885256   3% /dev/shm
10:15:38shm   3994752125880   3868872   4% /dev/shm
10:16:06shm   3994752142264   3852488   4% /dev/shm
10

Re: [PATCH] Memory leak, at src/common/exec.c

[Tom Lane]

Mark Dilger  writes:
> Please see the man page for putenv.  Are you certain it is safe to
> free the string passed to putenv after putenv returns?  I think this
> may be implemented differently on various platforms.

POSIX requires the behavior the glibc man page describes:

The putenv() function shall use the string argument to set environment
variable values. The string argument should point to a string of the
form "name=value". The putenv() function shall make the value of
the environment variable name equal to value by altering an existing
variable or creating a new one. In either case, the string pointed to
by string shall become part of the environment, so altering the string
shall change the environment.

So yeah, that patch is completely wrong.  It might've survived light
testing with non-debug versions of malloc/free, but under any sort
of load the environment variable would become corrupted.  The reason
for the strdup in our code is exactly to make a long-lived string
that can safely be given to putenv.

regards, tom lane

RE: [PATCH] Memory leak, at src/common/exec.c

[Ranier Vilela]

According to the documentation at:
https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
"Using setenv() is easier and consequently less error prone than using 
putenv()."
putenv is problematic and error prone, better replace by setenv.

As a result, set_pglocale_pgservice, is much simpler and more readable.

regards,
Ranier Vilela
diff --git a/src/common/exec.c b/src/common/exec.c
index 92dc3134a1..82e94b4df1 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -72,14 +72,15 @@ validate_exec(const char *path)
 	int			is_x;
 
 #ifdef WIN32
-	char		path_exe[MAXPGPATH + sizeof(".exe") - 1];
+	char		path_exe[MAXPGPATH + sizeof(".exe")];
+	int path_len;
 
 	/* Win32 requires a .exe suffix for stat() */
-	if (strlen(path) >= strlen(".exe") &&
-		pg_strcasecmp(path + strlen(path) - strlen(".exe"), ".exe") != 0)
+	path_len = strlen(path);
+	if (path_len >= (sizeof(".exe") - 1) &&
+		pg_strcasecmp(path + path_len - (sizeof(".exe") - 1), ".exe") != 0)
 	{
-		strlcpy(path_exe, path, sizeof(path_exe) - 4);
-		strcat(path_exe, ".exe");
+		snprintf(path_exe, sizeof(path_exe) - 5, "%s.exe", path);
 		path = path_exe;
 	}
 #endif
@@ -566,11 +567,8 @@ set_pglocale_pgservice(const char *argv0, const char *app)
 {
 	char		path[MAXPGPATH];
 	char		my_exec_path[MAXPGPATH];
-	char		env_path[MAXPGPATH + sizeof("PGSYSCONFDIR=")];	/* longer than
- * PGLOCALEDIR */
-	char	   *dup_path;
 
-	/* don't set LC_ALL in the backend */
+/* don't set LC_ALL in the backend */
 	if (strcmp(app, PG_TEXTDOMAIN("postgres")) != 0)
 	{
 		setlocale(LC_ALL, "");
@@ -596,25 +594,19 @@ set_pglocale_pgservice(const char *argv0, const char *app)
 
 	if (getenv("PGLOCALEDIR") == NULL)
 	{
-		/* set for libpq to use */
-		snprintf(env_path, sizeof(env_path), "PGLOCALEDIR=%s", path);
-		canonicalize_path(env_path + 12);
-		dup_path = strdup(env_path);
-		if (dup_path)
-			putenv(dup_path);
+	/* set for libpq to use */
+	canonicalize_path(path);
+setenv("PGLOCALEDIR", path, 1);
 	}
 #endif
 
 	if (getenv("PGSYSCONFDIR") == NULL)
 	{
-		get_etc_path(my_exec_path, path);
-
-		/* set for libpq to use */
-		snprintf(env_path, sizeof(env_path), "PGSYSCONFDIR=%s", path);
-		canonicalize_path(env_path + 13);
-		dup_path = strdup(env_path);
-		if (dup_path)
-			putenv(dup_path);
+	get_etc_path(my_exec_path, path);
+
+	/* set for libpq to use */
+	canonicalize_path(path);
+setenv("PGSYSCONFDIR", path, 1);
 	}
 }

Re: [PATCH] Memory leak, at src/common/exec.c

[Tom Lane]

Ranier Vilela  writes:
> According to the documentation at:
> https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
> "Using setenv() is easier and consequently less error prone than using 
> putenv()."
> putenv is problematic and error prone, better replace by setenv.

setenv is also less portable: it does not appear in SUSv2, which is still
our baseline spec for Unix platforms.  We've avoided its use since 2001,
cf. ec7ddc158.

It's also fair to wonder how well this change would fly on Windows,
where we have to implement putenv for ourselves to get things to work
right (cf. src/port/win32env.c, which does not offer support for
setenv).

Please stop inventing reasons to change code that's worked fine for
decades.  We have better things to spend our time on.

regards, tom lane

Re: ERROR: could not resize shared memory segment...No space left on device

[Thomas Munro]

On Tue, Dec 17, 2019 at 10:53 AM Tomas Vondra
 wrote:
> Interestingly enough, I ran into the same ERROR (not sure if the same
> root cause) while investigating bug #16104 [1], i.e. on a much simpler
> query (single join).
>
> This This particular machine is a bit smaller (only 8GB of RAM and less
> disk space) so I created a smaller table with "just" 1.5B rows:
>
>create table test as select generate_series(1, 15)::bigint i;
>set work_mem = '150MB';
>set max_parallel_workers_per_gather = 8;
>analyze test;
>
>explain select count(*) from test t1 join test t2 using (i);
>
>   QUERY PLAN
> 
>   Finalize Aggregate  (cost=67527436.36..67527436.37 rows=1 width=8)
> ->  Gather  (cost=67527435.53..67527436.34 rows=8 width=8)
>   Workers Planned: 8
>   ->  Partial Aggregate  (cost=67526435.53..67526435.54 rows=1 
> width=8)
> ->  Parallel Hash Join  (cost=11586911.03..67057685.47 
> rows=187500024 width=0)
>   Hash Cond: (t1.i = t2.i)
>   ->  Parallel Seq Scan on test t1  
> (cost=0.00..8512169.24 rows=187500024 width=8)
>   ->  Parallel Hash  (cost=8512169.24..8512169.24 
> rows=187500024 width=8)
> ->  Parallel Seq Scan on test t2  
> (cost=0.00..8512169.24 rows=187500024 width=8)
> (9 rows)
>
>explain analyze select count(*) from test t1 join test t2 using (i);
>
>ERROR:  could not resize shared memory segment "/PostgreSQL.1743102822" to 
> 536870912 bytes: No space left on device
>
> Now, work_mem = 150MB might be a bit too high considering the machine
> only has 8GB of RAM (1GB of which is shared_buffers). But that's still
> just 1.2GB of RAM and this is not an OOM. This actually fills the
> /dev/shm mount, which is limited to 4GB on this box
>
>bench ~ # df | grep shm
>shm  3994752   16   3994736   1% /dev/shm
>
> So somewhere in the parallel hash join, we allocate 4GB of shared segments ...
>
> The filesystem usage from the moment of the query execution to the
> failure looks about like this:
>
>  Time fs 1K-blocks  Used Available Use% Mounted on
>  --
>  10:13:34shm   3994752 34744   3960008   1% /dev/shm
>  10:13:35shm   3994752 35768   3958984   1% /dev/shm
>  10:13:36shm   3994752 37816   3956936   1% /dev/shm
>  10:13:39shm   3994752 39864   3954888   1% /dev/shm
>  10:13:42shm   3994752 41912   3952840   2% /dev/shm
>  10:13:46shm   3994752 43960   3950792   2% /dev/shm
>  10:13:49shm   3994752 48056   3946696   2% /dev/shm
>  10:13:56shm   3994752 52152   3942600   2% /dev/shm
>  10:14:02shm   3994752 56248   3938504   2% /dev/shm
>  10:14:09shm   3994752 60344   3934408   2% /dev/shm
>  10:14:16shm   3994752 68536   3926216   2% /dev/shm
>  10:14:30shm   3994752 76728   3918024   2% /dev/shm
>  10:14:43shm   3994752 84920   3909832   3% /dev/shm
>  10:14:43shm   3994752 84920   3909832   3% /dev/shm
>  10:14:57shm   3994752 93112   3901640   3% /dev/shm
>  10:15:11shm   3994752109496   3885256   3% /dev/shm
>  10:15:38shm   3994752125880   3868872   4% /dev/shm
>  10:16:06shm   3994752142264   3852488   4% /dev/shm
>  10:19:57shm   3994752683208   3311544  18% /dev/shm
>  10:19:58shm   3994752   1338568   2656184  34% /dev/shm
>  10:20:02shm   3994752   1600712   2394040  41% /dev/shm
>  10:20:03shm   3994752   2125000   1869752  54% /dev/shm
>  10:20:04shm   3994752   2649288   1345464  67% /dev/shm
>  10:20:08shm   3994752   2518216   1476536  64% /dev/shm
>  10:20:10shm   3994752   3173576821176  80% /dev/shm
>  10:20:14shm   3994752   3697864296888  93% /dev/shm
>  10:20:15shm   3994752   3417288577464  86% /dev/shm
>  10:20:16shm   3994752   3697864296888  93% /dev/shm
>  10:20:20shm   3994752   3828936165816  96% /dev/shm
>
> And at the end, the contents of /dev/shm looks like this:
>
> -rw--- 1 postgres postgres  33624064 Dec 16 22:19 PostgreSQL.1005341478
> -rw--- 1 postgres postgres   1048576 Dec 16 22:20 PostgreSQL.1011142277
> -rw--- 1 postgres postgres   1048576 Dec 16 22:20 PostgreSQL.1047241463
> -rw--- 1 postgres postgres  16777216 Dec 16 22:16 PostgreSQL.1094702083
> -rw--- 1 postgres postgres 268435456 Dec 16 22:20 PostgreSQL.1143288540
> -rw--- 1 postgres postgres 536870912 Dec 16 22:20 PostgreSQL.1180709918
> -rw--- 1 postgres postgres  7408 Dec 14 15:43 PostgreSQL.1239805533
> -rw--- 1 postgres postgres 13421772

Unportable(?) use of setenv() in secure_open_gssapi()

[Tom Lane]

I noticed while investigating [1] that we have one single solitary
use of setenv(3) in our code base, in secure_open_gssapi().

It's been project policy since 2001 to avoid setenv(), and I notice
that src/port/win32env.c lacks support for setenv(), making it
pretty doubtful that the call has the semantics one would wish
on Windows.

Now, versions of the POSIX spec released in this century do have setenv(),
and even seem to regard it as "more standard" than putenv().  So maybe
there's a case for moving our goalposts and deciding to allow use of
setenv().  But then it seems like we'd better twiddle win32env.c to
support it; and I'm not sure back-patching such a change would be wise.

Alternatively, we could change secure_open_gssapi() to use putenv(),
at the cost of a couple more lines of code.

Thoughts?

regards, tom lane

[1] 
https://www.postgresql.org/message-id/SN2PR05MB264066382E2CC75E734492C8E3510%40SN2PR05MB2640.namprd05.prod.outlook.com

reducing memory usage by using "proxy" memory contexts?

[Andres Freund]

Hi,

I was responding to a question about postgres' per-backend memory usage,
making me look at the various contexts below CacheMemoryContext.  There
is pretty much always a significant number of contexts below, one for
each index:

  CacheMemoryContext: 524288 total in 7 blocks; 8680 free (0 chunks); 515608 
used
index info: 2048 total in 2 blocks; 568 free (1 chunks); 1480 used: 
pg_class_tblspc_relfilenode_index
index info: 2048 total in 2 blocks; 960 free (0 chunks); 1088 used: 
pg_statistic_ext_relid_index
index info: 2048 total in 2 blocks; 976 free (0 chunks); 1072 used: 
blarg_pkey
index info: 2048 total in 2 blocks; 872 free (0 chunks); 1176 used: 
pg_index_indrelid_index
index info: 2048 total in 2 blocks; 600 free (1 chunks); 1448 used: 
pg_attrdef_adrelid_adnum_index
index info: 2048 total in 2 blocks; 656 free (2 chunks); 1392 used: 
pg_db_role_setting_databaseid_rol_index
index info: 2048 total in 2 blocks; 544 free (2 chunks); 1504 used: 
pg_opclass_am_name_nsp_index
index info: 2048 total in 2 blocks; 928 free (2 chunks); 1120 used: 
pg_foreign_data_wrapper_name_index
index info: 2048 total in 2 blocks; 960 free (2 chunks); 1088 used: 
pg_enum_oid_index
index info: 2048 total in 2 blocks; 600 free (1 chunks); 1448 used: 
pg_class_relname_nsp_index
index info: 2048 total in 2 blocks; 960 free (2 chunks); 1088 used: 
pg_foreign_server_oid_index
index info: 2048 total in 2 blocks; 960 free (2 chunks); 1088 used: 
pg_publication_pubname_index
...
index info: 3072 total in 2 blocks; 1144 free (2 chunks); 1928 used: 
pg_conversion_default_index
...

while I also think we could pretty easily reduce the amount of memory
used for each index, I want to focus on something else here:

We waste a lot of space due to all these small contexts. Even leaving
aside the overhead of the context and its blocks - not insignificant -
they are mostly between ~1/2 a ~1/4 empty.

At the same time we probably don't want to inline all of them into
CacheMemoryContext - too likely to introduce bugs, and too hard to
maintain leak free.


But what if we had a new type of memory context that did not itself
manage memory underlying allocations, but instead did so via the parent?
If such a context tracked all the live allocations in some form of list,
it could then free them from the parent at reset time. In other words,
it'd proxy all memory management via the parent, only adding a separate
name, and tracking of all live chunks.

Obviously such a context would be less efficient to reset than a plain
aset.c one - but I don't think that'd matter much for these types of
use-cases.  The big advantage in this case would be that we wouldn't
have separate two separate "blocks" for each index cache entry, but
instead allocations could all be done within CacheMemoryContext.

Does that sound like a sensible idea?

Greetings,

Andres Freund

Re: reducing memory usage by using "proxy" memory contexts?

[Tom Lane]

Andres Freund  writes:
> I was responding to a question about postgres' per-backend memory usage,
> making me look at the various contexts below CacheMemoryContext.  There
> is pretty much always a significant number of contexts below, one for
> each index:
> index info: 2048 total in 2 blocks; 568 free (1 chunks); 1480 used: 
> pg_class_tblspc_relfilenode_index

Yup.

> But what if we had a new type of memory context that did not itself
> manage memory underlying allocations, but instead did so via the parent?
> If such a context tracked all the live allocations in some form of list,
> it could then free them from the parent at reset time. In other words,
> it'd proxy all memory management via the parent, only adding a separate
> name, and tracking of all live chunks.

I dunno, that seems like a *lot* of added overhead, and opportunity for
bugs.  Maybe it'd be all right for contexts in which alloc/dealloc is
very infrequent.  But why not just address this problem by reducing the
allocset blocksize parameter (some more) for these index contexts?

I'd even go a bit further, and suggest that the right way to exploit
our knowledge that these contexts' contents don't change much is to
go the other way, and reduce not increase their per-chunk overhead.
I've wanted for some time to build a context type that doesn't support
pfree() but just makes it a no-op, and doesn't round request sizes up
further than the next maxalign boundary.  Without pfree we don't need
a normal chunk header; the minimum requirement of a context pointer
is enough.  And since we aren't going to be recycling any chunks, there's
no need to try to standardize their sizes.  This seems like it'd be ideal
for cases like the index cache contexts.

(For testing purposes, the generation.c context type might be close
enough for this, and it'd be easier to shove in.)

regards, tom lane

Re: reducing memory usage by using "proxy" memory contexts?

[Tomas Vondra]

On Mon, Dec 16, 2019 at 03:35:12PM -0800, Andres Freund wrote:

Hi,

I was responding to a question about postgres' per-backend memory usage,
making me look at the various contexts below CacheMemoryContext.  There
is pretty much always a significant number of contexts below, one for
each index:

 CacheMemoryContext: 524288 total in 7 blocks; 8680 free (0 chunks); 515608 used
   index info: 2048 total in 2 blocks; 568 free (1 chunks); 1480 used: 
pg_class_tblspc_relfilenode_index
   index info: 2048 total in 2 blocks; 960 free (0 chunks); 1088 used: 
pg_statistic_ext_relid_index
   index info: 2048 total in 2 blocks; 976 free (0 chunks); 1072 used: 
blarg_pkey
   index info: 2048 total in 2 blocks; 872 free (0 chunks); 1176 used: 
pg_index_indrelid_index
   index info: 2048 total in 2 blocks; 600 free (1 chunks); 1448 used: 
pg_attrdef_adrelid_adnum_index
   index info: 2048 total in 2 blocks; 656 free (2 chunks); 1392 used: 
pg_db_role_setting_databaseid_rol_index
   index info: 2048 total in 2 blocks; 544 free (2 chunks); 1504 used: 
pg_opclass_am_name_nsp_index
   index info: 2048 total in 2 blocks; 928 free (2 chunks); 1120 used: 
pg_foreign_data_wrapper_name_index
   index info: 2048 total in 2 blocks; 960 free (2 chunks); 1088 used: 
pg_enum_oid_index
   index info: 2048 total in 2 blocks; 600 free (1 chunks); 1448 used: 
pg_class_relname_nsp_index
   index info: 2048 total in 2 blocks; 960 free (2 chunks); 1088 used: 
pg_foreign_server_oid_index
   index info: 2048 total in 2 blocks; 960 free (2 chunks); 1088 used: 
pg_publication_pubname_index
...
   index info: 3072 total in 2 blocks; 1144 free (2 chunks); 1928 used: 
pg_conversion_default_index
...

while I also think we could pretty easily reduce the amount of memory
used for each index, I want to focus on something else here:

We waste a lot of space due to all these small contexts. Even leaving
aside the overhead of the context and its blocks - not insignificant -
they are mostly between ~1/2 a ~1/4 empty.

At the same time we probably don't want to inline all of them into
CacheMemoryContext - too likely to introduce bugs, and too hard to
maintain leak free.


But what if we had a new type of memory context that did not itself
manage memory underlying allocations, but instead did so via the parent?
If such a context tracked all the live allocations in some form of list,
it could then free them from the parent at reset time. In other words,
it'd proxy all memory management via the parent, only adding a separate
name, and tracking of all live chunks.

Obviously such a context would be less efficient to reset than a plain
aset.c one - but I don't think that'd matter much for these types of
use-cases.  The big advantage in this case would be that we wouldn't
have separate two separate "blocks" for each index cache entry, but
instead allocations could all be done within CacheMemoryContext.

Does that sound like a sensible idea?



I do think it's an interesting idea, worth exploring.

I agree it's probably OK if the proxy contexts are a bit less efficient,
but I think we can restrict their use to places where that's not an
issue (i.e. low frequency of resets, small number of allocated chunks
etc.). And if needed we can probably find ways to improve the efficiency
e.g. by replacing the linked list with a small hash table or something
(to speed-up pfree etc.). Or something.

I think the big question is what this would mean for the parent context.
Because suddenly it's a mix of chunks with different life spans, which
would originally be segregared in different malloc-ed blocks. And now
that would not be true, so e.g. after deleting the child context the
memory would not be freed but just moved to the freelist.

It would also confuse MemoryContextStats, which would suddenly not
realize some of the chunks are actually "owned" by the child context.
Maybe this could be improved, but only partially (unless we'd want to
have a per-chunk flag if it's owned by the context or by a proxy).

Not sure if this would impact accounting (e.g. what if someone creates a
custom aggregate, creating a separate proxy context per group?). Would
that work or not?

Also, would this need to support nested proxy contexts? That might
complicate things quite a bit, I'm afraid.

FWIW I don't know answers to these questions.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 

Re: [Proposal] Level4 Warnings show many shadow vars

[Michael Paquier]

On Mon, Dec 09, 2019 at 05:11:10PM -0500, Tom Lane wrote:
> Alvaro Herrera  writes:
>> We have a not-consistently-used convention that names in CamelCase are
>> used for global variables.  Naming a function parameter in that style
>> seems pointlessly confusing.  I would rather use redorecptr as Tom
>> suggested, which fits with the style used for the other arguments of
>> that function.  BTW prepending an X or a p looks like minimum effort...
>> I'd stay away from that.
> 
> Actually, for the particular case of RemoveXlogFile(s), I wonder if it
> shouldn't be "startptr" to go with the other argument "endptr".  This line
> of thinking might not lead to nicer names in other functions, of course.
> But we shouldn't assume that a one-size-fits-all solution is going to
> improve legibility, and in the end, legibility is what this should be
> about.

Hmm.  In the case of this logic, we are referring to the current end
of WAL with endptr, and what you are calling the startptr is really
the redo LSN of the last checkpoint in all the routines which are now
confused with RedoRecPtr: RemoveOldXlogFile, RemoveXlogFile and
XLOGfileslop.  Using lower-case for all the characters of the variable
name sounds like a good improvement as well, so taking a combination
of all that I would just use "lastredoptr" in those three code paths
(note that we used to have PriorRedoPtr before).  As that's a
confusion I introduced with d9fadbf, I would like to fix that and
backpatch this change down to 11.  (Ranier gets the authorship
per se as that's extracted from a larger patch).
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3baf1b009a..71b8389ba1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -891,8 +891,8 @@ static int	emode_for_corrupt_record(int emode, XLogRecPtr RecPtr);
 static void XLogFileClose(void);
 static void PreallocXlogFiles(XLogRecPtr endptr);
 static void RemoveTempXlogFiles(void);
-static void RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr RedoRecPtr, XLogRecPtr endptr);
-static void RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr);
+static void RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr lastredoptr, XLogRecPtr endptr);
+static void RemoveXlogFile(const char *segname, XLogRecPtr lastredoptr, XLogRecPtr endptr);
 static void UpdateLastRemovedPtr(char *filename);
 static void ValidateXLOGDirectoryStructure(void);
 static void CleanupBackupHistory(void);
@@ -2298,7 +2298,7 @@ assign_checkpoint_completion_target(double newval, void *extra)
  * XLOG segments? Returns the highest segment that should be preallocated.
  */
 static XLogSegNo
-XLOGfileslop(XLogRecPtr RedoRecPtr)
+XLOGfileslop(XLogRecPtr lastredoptr)
 {
 	XLogSegNo	minSegNo;
 	XLogSegNo	maxSegNo;
@@ -2310,9 +2310,9 @@ XLOGfileslop(XLogRecPtr RedoRecPtr)
 	 * correspond to. Always recycle enough segments to meet the minimum, and
 	 * remove enough segments to stay below the maximum.
 	 */
-	minSegNo = RedoRecPtr / wal_segment_size +
+	minSegNo = lastredoptr / wal_segment_size +
 		ConvertToXSegs(min_wal_size_mb, wal_segment_size) - 1;
-	maxSegNo = RedoRecPtr / wal_segment_size +
+	maxSegNo = lastredoptr / wal_segment_size +
 		ConvertToXSegs(max_wal_size_mb, wal_segment_size) - 1;
 
 	/*
@@ -2327,7 +2327,7 @@ XLOGfileslop(XLogRecPtr RedoRecPtr)
 	/* add 10% for good measure. */
 	distance *= 1.10;
 
-	recycleSegNo = (XLogSegNo) ceil(((double) RedoRecPtr + distance) /
+	recycleSegNo = (XLogSegNo) ceil(((double) lastredoptr + distance) /
 	wal_segment_size);
 
 	if (recycleSegNo < minSegNo)
@@ -3948,12 +3948,12 @@ RemoveTempXlogFiles(void)
 /*
  * Recycle or remove all log files older or equal to passed segno.
  *
- * endptr is current (or recent) end of xlog, and RedoRecPtr is the
+ * endptr is current (or recent) end of xlog, and lastredoptr is the
  * redo pointer of the last checkpoint. These are used to determine
  * whether we want to recycle rather than delete no-longer-wanted log files.
  */
 static void
-RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
+RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr lastredoptr, XLogRecPtr endptr)
 {
 	DIR		   *xldir;
 	struct dirent *xlde;
@@ -3996,7 +3996,7 @@ RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
 /* Update the last removed location in shared memory first */
 UpdateLastRemovedPtr(xlde->d_name);
 
-RemoveXlogFile(xlde->d_name, RedoRecPtr, endptr);
+RemoveXlogFile(xlde->d_name, lastredoptr, endptr);
 			}
 		}
 	}
@@ -4070,14 +4070,14 @@ RemoveNonParentXlogFiles(XLogRecPtr switchpoint, TimeLineID newTLI)
 /*
  * Recycle or remove a log file that's no longer needed.
  *
- * endptr is current (or recent) end of xlog, and RedoRecPtr is the
+ * endptr is current (or recent) end of xlog, and lastredoptr is the
  * redo pointer of the last checkpoint. These are used to determine
  * whether we want to recycl

Re: Unportable(?) use of setenv() in secure_open_gssapi()

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> I noticed while investigating [1] that we have one single solitary
> use of setenv(3) in our code base, in secure_open_gssapi().
> 
> It's been project policy since 2001 to avoid setenv(), and I notice
> that src/port/win32env.c lacks support for setenv(), making it
> pretty doubtful that the call has the semantics one would wish
> on Windows.

Yeah, that doesn't seem good, though you'd have to be building with MIT
Kerberos for Windows to end up with GSSAPI on a Windows build in the
first place (much more common on Windows is to build with Microsoft SSPI
support instead).  Still, it looks like someone went to the trouble of
setting that up on a buildfarm animal- looks like hamerkop has it.

> Now, versions of the POSIX spec released in this century do have setenv(),
> and even seem to regard it as "more standard" than putenv().  So maybe
> there's a case for moving our goalposts and deciding to allow use of
> setenv().  But then it seems like we'd better twiddle win32env.c to
> support it; and I'm not sure back-patching such a change would be wise.
> 
> Alternatively, we could change secure_open_gssapi() to use putenv(),
> at the cost of a couple more lines of code.
> 
> Thoughts?

So, auth.c already does the song-and-dance for putenv for this exact
variable, but it happens too late if you want to use GSSAPI for an
encrypted connection.  Looking at this now, it seems like we should
really just move up where that's happening instead of having it done
once in be-secure-gssapi.c and then again in auth.c.  Maybe we could do
it in BackendInitialize..?

Thanks,

Stephen


signature.asc
Description: PGP signature

RE: reducing memory usage by using "proxy" memory contexts?

[tsunakawa.ta...@fujitsu.com]

From: Andres Freund 
> We waste a lot of space due to all these small contexts. Even leaving
> aside the overhead of the context and its blocks - not insignificant -
> they are mostly between ~1/2 a ~1/4 empty.
> 
> 
> But what if we had a new type of memory context that did not itself
> manage memory underlying allocations, but instead did so via the parent?
> If such a context tracked all the live allocations in some form of list,
> it could then free them from the parent at reset time. In other words,
> it'd proxy all memory management via the parent, only adding a separate
> name, and tracking of all live chunks.

It sounds like that it will alleviate the memory bloat caused by SAVEPOINT and 
RELEASE, which leave CurTransactionContext for each subtransaction.  The memory 
overuse got Linux down when our customer's batch application ran millions of 
SQL statements in a transaction with psqlODBC.  psqlODBC uses savepoints by 
default to enable statement rollback.

(I guess this issue of one memory context per subtransaction caused the crash 
of Amazon Aurora on the Prime Day last year.)


Regards
Takayuki Tsunakawa

Re: Unportable(?) use of setenv() in secure_open_gssapi()

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> It's been project policy since 2001 to avoid setenv(), and I notice
>> that src/port/win32env.c lacks support for setenv(), making it
>> pretty doubtful that the call has the semantics one would wish
>> on Windows.

> Yeah, that doesn't seem good, though you'd have to be building with MIT
> Kerberos for Windows to end up with GSSAPI on a Windows build in the
> first place (much more common on Windows is to build with Microsoft SSPI
> support instead).  Still, it looks like someone went to the trouble of
> setting that up on a buildfarm animal- looks like hamerkop has it.

It looks like it'd only matter if Kerberos were using a different CRT
version than PG proper, which is probably even less likely.  Still,
that could happen.

> So, auth.c already does the song-and-dance for putenv for this exact
> variable, but it happens too late if you want to use GSSAPI for an
> encrypted connection.  Looking at this now, it seems like we should
> really just move up where that's happening instead of having it done
> once in be-secure-gssapi.c and then again in auth.c.  Maybe we could do
> it in BackendInitialize..?

Hm, yeah, and it's also pretty darn inconsistent that one of them does
overwrite = 1 while the other emulates overwrite = 0.  I'd be for
unifying that code.  It'd also lead to a more safely back-patchable
fix than the other solution.

Going forward, adding support for setenv() wouldn't be an unreasonable
thing to do, I think.  It's certainly something that people find
attractive to use, and the portability issues we had with it back at
the turn of the century should be pretty much gone.  I do note that my
old dinosaur gaur, which is the last surviving buildfarm member without
unsetenv(), lacks setenv() as well --- but I'd be willing to add support
for that as a src/port module.  We'd also have to fix win32env.c, but
that's not much new code either.

regards, tom lane

Re: reducing memory usage by using "proxy" memory contexts?

[Andres Freund]

Hi,

On 2019-12-16 18:58:36 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > But what if we had a new type of memory context that did not itself
> > manage memory underlying allocations, but instead did so via the parent?
> > If such a context tracked all the live allocations in some form of list,
> > it could then free them from the parent at reset time. In other words,
> > it'd proxy all memory management via the parent, only adding a separate
> > name, and tracking of all live chunks.
> 
> I dunno, that seems like a *lot* of added overhead, and opportunity for
> bugs.

What kind of bugs are you thinking of?


> Maybe it'd be all right for contexts in which alloc/dealloc is
> very infrequent.

I don't think the overhead would be enough to matter even for moderaly
common cases. Sure, another 16bytes of overhead isn't free, nor is the
indirection upon allocation/free, but it's also not that bad. I'd be
surprised if it didn't turn out to be cheaper in a lot of cases,
actually, due to not needing a separate init block etc.  Obviously it'd
make no sense to use such a context for cases with very frequent
allocations (say parsing, copying a node tree), or where bulk
deallocations of a lot of small allocations is important - but there's
plenty other types of cases.


> But why not just address this problem by reducing the allocset
> blocksize parameter (some more) for these index contexts?

Well, but what would we set it to? The total allocated memory sizes for
different indexes varies between ~1kb and 4kb. And we'll have to cope
with that without creating waste again. We could allow much lower
initial and max block sizes for aset, I guess, so anything large gets to
be its own malloc() block.


> I'd even go a bit further, and suggest that the right way to exploit
> our knowledge that these contexts' contents don't change much is to
> go the other way, and reduce not increase their per-chunk overhead.

Yea, I was wondering about that too. However, while there's also a good
number of small allocations, a large fraction of the used space is
actually larger allocations. And using a "alloc only" context doesn't
really address the fact that the underlying memory blocks are quite
wasteful - especially given that this data essentially lives forever.

For the specific case of RelationInitIndexAccessInfo(), allocations that
commonly live for the rest of the backend's life and are frequent enough
of them to matter, it might be worth micro-optimizing the
allocations. E.g. not doing ~7 separate allocations within a few
lines...  Not primarily because of the per-allocation overheads, but
more because that'd allow to size things right directly.


Greetings,

Andres Freund

Re: reducing memory usage by using "proxy" memory contexts?

[Andres Freund]

Hi,

On 2019-12-17 01:12:43 +0100, Tomas Vondra wrote:
> On Mon, Dec 16, 2019 at 03:35:12PM -0800, Andres Freund wrote:
> > But what if we had a new type of memory context that did not itself
> > manage memory underlying allocations, but instead did so via the parent?
> > If such a context tracked all the live allocations in some form of list,
> > it could then free them from the parent at reset time. In other words,
> > it'd proxy all memory management via the parent, only adding a separate
> > name, and tracking of all live chunks.
> > 
> > Obviously such a context would be less efficient to reset than a plain
> > aset.c one - but I don't think that'd matter much for these types of
> > use-cases.  The big advantage in this case would be that we wouldn't
> > have separate two separate "blocks" for each index cache entry, but
> > instead allocations could all be done within CacheMemoryContext.
> > 
> > Does that sound like a sensible idea?
> > 
> 
> I do think it's an interesting idea, worth exploring.
> 
> I agree it's probably OK if the proxy contexts are a bit less efficient,
> but I think we can restrict their use to places where that's not an
> issue (i.e. low frequency of resets, small number of allocated chunks
> etc.). And if needed we can probably find ways to improve the efficiency
> e.g. by replacing the linked list with a small hash table or something
> (to speed-up pfree etc.). Or something.

I don't think you'd need a hash table for efficiency - I was thinking of
just using a doubly linked list. That allows O(1) unlinking.


> I think the big question is what this would mean for the parent context.
> Because suddenly it's a mix of chunks with different life spans, which
> would originally be segregared in different malloc-ed blocks. And now
> that would not be true, so e.g. after deleting the child context the
> memory would not be freed but just moved to the freelist.

I think in the case of CacheMemoryContext it'd not really be a large
change - we already have vastly different lifetimes there, e.g. for the
relcache entries themselves.  I could also see using something like this
for some of the executor sub-contexts - they commonly have only very few
allocations, but need to be resettable individually.


> It would also confuse MemoryContextStats, which would suddenly not
> realize some of the chunks are actually "owned" by the child context.
> Maybe this could be improved, but only partially (unless we'd want to
> have a per-chunk flag if it's owned by the context or by a proxy).

I'm not sure it'd really be worth fixing this fully, tbh. Maybe just
reporting at MemoryContextStats time whether a sub-context is included
in the parent's total or not.


> Not sure if this would impact accounting (e.g. what if someone creates a
> custom aggregate, creating a separate proxy context per group?). Would
> that work or not?

I'm not sure what problem you're thinking of?


> Also, would this need to support nested proxy contexts? That might
> complicate things quite a bit, I'm afraid.

I mean, it'd probably not be a great idea to do so much - due to
increased overhead - but I don't see why it wouldn't work. If it
actually is something that we'd want to make work efficiently at some
point, it shouldn't be too hard to have code to walk up the chain of
parent contexts at creation time to the next context that's not a proxy.

Greetings,

Andres Freund

Re: non-exclusive backup cleanup is mildly broken

[Fujii Masao]

On Tue, Dec 17, 2019 at 4:19 AM Robert Haas  wrote:
>
> On Sun, Dec 15, 2019 at 8:44 PM Kyotaro Horiguchi
>  wrote:
> > However I don't object to the restriction, couldn't we allow the
> > cancel_before_shmem_exit to search for the given entry looping over
> > the before_shmem_exit array? If we don't do that, an assrtion is needed
> > instead.
> >
> > Since pg_stop_backup_v2 is the only caller to the function in the
> > whole server code, making cancel_before_shmem_exit a bit wiser (and
> > slower) cannot hurt anyone.
>
> That's actually not true. It's called from
> PG_END_ENSURE_ERROR_CLEANUP. Still, it wouldn't cost a lot to fix this
> that way. However, I think that it's better to fix it the other way,
> as I mentioned in my original email.

+1

Not only PREPARE but also other commands that we may add in the future
can cause the same issue, so it's better to address the root cause rather
than working around by disallowing PREPARE.

Regards,

-- 
Fujii Masao

Re: Fastpath while arranging the changes in LSN order in logical decoding

[Dilip Kumar]

On Mon, Nov 25, 2019 at 9:22 AM Dilip Kumar  wrote:
>
> In logical decoding, while sending the changes to the output plugin we
> need to arrange them in the LSN order.  But, if there is only one
> transaction which is a very common case then we can avoid building the
> binary heap.  A small patch is attached for the same.

I have registered it in the next commitfest.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

RE: [PATCH] Memory leak, at src/common/exec.c

[Ranier Vilela]

According to [1], windows does not support setenv, so for the patch to work 
[3],  would need to add it.
With the possibility of setenv going further [2], I am submitting in this 
thread, the patch to add setenv support on the windows side, avoiding starting 
a new trhead.
It is based on pre-existing functions, and seeks to correctly emulate the 
functioning of the POSIX setenv, but has not yet been tested.

If this work is not acceptable then it is finished. And two memory leaks and a 
possible access beyond heap bounds, reported and not fixed.

regards,
Ranier Vilela

[1] https://www.postgresql.org/message-id/29478.1576537771%40sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/30119.1576538578%40sss.pgh.pa.us
[3] 
https://www.postgresql.org/message-id/SN2PR05MB264066382E2CC75E734492C8E3510%40SN2PR05MB2640.namprd05.prod.outlook.comdiff --git a/src/common/exec.c b/src/common/exec.c
index 92dc3134a1..82e94b4df1 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -72,14 +72,15 @@ validate_exec(const char *path)
 	int			is_x;
 
 #ifdef WIN32
-	char		path_exe[MAXPGPATH + sizeof(".exe") - 1];
+	char		path_exe[MAXPGPATH + sizeof(".exe")];
+	int path_len;
 
 	/* Win32 requires a .exe suffix for stat() */
-	if (strlen(path) >= strlen(".exe") &&
-		pg_strcasecmp(path + strlen(path) - strlen(".exe"), ".exe") != 0)
+	path_len = strlen(path);
+	if (path_len >= (sizeof(".exe") - 1) &&
+		pg_strcasecmp(path + path_len - (sizeof(".exe") - 1), ".exe") != 0)
 	{
-		strlcpy(path_exe, path, sizeof(path_exe) - 4);
-		strcat(path_exe, ".exe");
+		snprintf(path_exe, sizeof(path_exe) - 5, "%s.exe", path);
 		path = path_exe;
 	}
 #endif
@@ -566,11 +567,8 @@ set_pglocale_pgservice(const char *argv0, const char *app)
 {
 	char		path[MAXPGPATH];
 	char		my_exec_path[MAXPGPATH];
-	char		env_path[MAXPGPATH + sizeof("PGSYSCONFDIR=")];	/* longer than
- * PGLOCALEDIR */
-	char	   *dup_path;
 
-	/* don't set LC_ALL in the backend */
+/* don't set LC_ALL in the backend */
 	if (strcmp(app, PG_TEXTDOMAIN("postgres")) != 0)
 	{
 		setlocale(LC_ALL, "");
@@ -596,25 +594,19 @@ set_pglocale_pgservice(const char *argv0, const char *app)
 
 	if (getenv("PGLOCALEDIR") == NULL)
 	{
-		/* set for libpq to use */
-		snprintf(env_path, sizeof(env_path), "PGLOCALEDIR=%s", path);
-		canonicalize_path(env_path + 12);
-		dup_path = strdup(env_path);
-		if (dup_path)
-			putenv(dup_path);
+	/* set for libpq to use */
+	canonicalize_path(path);
+setenv("PGLOCALEDIR", path, 1);
 	}
 #endif
 
 	if (getenv("PGSYSCONFDIR") == NULL)
 	{
-		get_etc_path(my_exec_path, path);
-
-		/* set for libpq to use */
-		snprintf(env_path, sizeof(env_path), "PGSYSCONFDIR=%s", path);
-		canonicalize_path(env_path + 13);
-		dup_path = strdup(env_path);
-		if (dup_path)
-			putenv(dup_path);
+	get_etc_path(my_exec_path, path);
+
+	/* set for libpq to use */
+	canonicalize_path(path);
+setenv("PGSYSCONFDIR", path, 1);
 	}
 }
 

diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 275f3bb699..33dc9bf5ad 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -65,7 +65,7 @@ my @frontend_uselibpgcommon = (
 my $frontend_extralibs = {
 	'initdb' => ['ws2_32.lib'],
 	'pg_restore' => ['ws2_32.lib'],
-	'pgbench'=> ['ws2_32.lib'],
+	'pgbench'=> ['ws2_32.lib', 'bcrypt.lib'],
 	'psql'   => ['ws2_32.lib']
 };
 my $frontend_extraincludes = {
@@ -184,6 +184,7 @@ sub mkvcbuild
 	$postgres->AddFiles('src/backend/utils/adt', 'jsonpath_scan.l',
 		'jsonpath_gram.y');
 	$postgres->AddDefine('BUILDING_DLL');
+	$postgres->AddLibrary('bcrypt.lib') if ($vsVersion > '12.00');
 	$postgres->AddLibrary('secur32.lib');
 	$postgres->AddLibrary('ws2_32.lib');
 	$postgres->AddLibrary('wldap32.lib') if ($solution->{options}->{ldap});
@@ -246,6 +247,7 @@ sub mkvcbuild
 	$libpq->AddDefine('FRONTEND');
 	$libpq->AddDefine('UNSAFE_STAT_OK');
 	$libpq->AddIncludeDir('src/port');
+	$libpq->AddLibrary('bcrypt.lib')  if ($vsVersion > '12.00');
 	$libpq->AddLibrary('secur32.lib');
 	$libpq->AddLibrary('ws2_32.lib');
 	$libpq->AddLibrary('wldap32.lib') if ($solution->{options}->{ldap});
diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index c459a2417d..1ffddcceee 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -463,6 +463,7 @@ extern void _dosmaperr(unsigned long);
 /* in port/win32env.c */
 extern int	pgwin32_putenv(const char *);
 extern void pgwin32_unsetenv(const char *);
+extern int	pgwin32_setenv(const char *name, const char *envval, int overwrite);
 
 /* in port/win32security.c */
 extern int	pgwin32_is_service(void);
@@ -473,6 +474,7 @@ extern BOOL AddUserToTokenDacl(HANDLE hToken);
 
 #define putenv(x) pgwin32_putenv(x)
 #define unsetenv(x) pgwin32_unsetenv(x)
+#define setenv(n,v,o) pgwin32_setenv(n, v, o)
 
 /* Things that exist in MinGW headers, but need to be added to MSVC */
 #ifdef _MSC_VER
diff

RE: [Proposal] Level4 Warnings show many shadow vars

[Ranier Vilela]

De: Michael Paquier
Enviadas: Terça-feira, 17 de Dezembro de 2019 00:36

>Hmm.  In the case of this logic, we are referring to the current end
>of WAL with endptr, and what you are calling the startptr is really
>the redo LSN of the last checkpoint in all the routines which are now
>confused with RedoRecPtr: RemoveOldXlogFile, RemoveXlogFile and
>XLOGfileslop.  Using lower-case for all the characters of the variable
>name sounds like a good improvement as well, so taking a combination
>of all that I would just use "lastredoptr" in those three code paths
>(note that we used to have PriorRedoPtr before).  As that's a
>confusion I introduced with d9fadbf, I would like to fix that and
>backpatch this change down to 11.  (Ranier gets the authorship
>per se as that's extracted from a larger patch).
Hey Michael, thank you so much for considering correct at least part of an 
extensive work.

Best regards,
Ranier Vilela

Re: [PATCH] Windows port add support to BCryptGenRandom

[Michael Paquier]

On Mon, Dec 16, 2019 at 09:18:10PM +, Ranier Vilela wrote:
> According to microsoft documentation at:
> https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom
> The function CryptGenRandom is deprecated, and may can be removed in future 
> release.
> This patch add support to use BCryptGenRandom.

+#if defined(_MSC_VER) && _MSC_VER >= 1900 \
+ && defined(MIN_WINNT) && MIN_WINNT >= 0x0600
+#define USE_WIN32_BCRYPTGENRANDOM
[...]
+   $postgres->AddLibrary('bcrypt.lib') if ($vsVersion > '12.00');
 
And looking at this page, it is said that the minimum version
supported by this function is Windows 2008:
https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom

Now, your changes in MkvcBuild.pm and the backend code assume that
we need to include bcrypt.lib since MSVC 2015 (at least version
14.00 or _MSC_VER >= 1900.  Do you have a reference about when this
has been introduced in VS?  The MS docs don't seem to hold a hint
about that..
--
Michael


signature.asc
Description: PGP signature

RE: [PATCH] Windows port add support to BCryptGenRandom

[Ranier Vilela]

De: Michael Paquier
Enviadas: Terça-feira, 17 de Dezembro de 2019 03:43
Para: Ranier Vilela
Cc: pgsql-hackers@lists.postgresql.org
Assunto: Re: [PATCH] Windows port add support to BCryptGenRandom

>And looking at this page, it is said that the minimum version
>supported by this function is Windows 2008:
>https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom

>Now, your changes in MkvcBuild.pm and the backend code assume that
>we need to include bcrypt.lib since MSVC 2015 (at least version
>14.00 or _MSC_VER >= 1900.  Do you have a reference about when this
>has been introduced in VS?  The MS docs don't seem to hold a hint
>about that..
Sorry Perl I understand a little bit.
Windows Vista I believe.
https://github.com/openssl/openssl/blob/master/crypto/rand/rand_win.c
is the primary font and have more information.

Best regards,
Ranier Vilela
[https://avatars0.githubusercontent.com/u/3279138?s=400&v=4]
openssl/rand_win.c at master · openssl/openssl · 
GitHub
TLS/SSL and crypto library. Contribute to openssl/openssl development by 
creating an account on GitHub.
github.com

Re: reducing memory usage by using "proxy" memory contexts?

[Tom Lane]

Andres Freund  writes:
> For the specific case of RelationInitIndexAccessInfo(), allocations that
> commonly live for the rest of the backend's life and are frequent enough
> of them to matter, it might be worth micro-optimizing the
> allocations. E.g. not doing ~7 separate allocations within a few
> lines...  Not primarily because of the per-allocation overheads, but
> more because that'd allow to size things right directly.

Hmm ... that would be worth trying, for sure, since it's so easy ...

regards, tom lane

Re: Allow cluster owner to bypass authentication

[Andrew Dunstan]

On Thu, Aug 15, 2019 at 9:07 PM Peter Eisentraut
 wrote:
>
> This is an implementation of the idea I mentioned in [0].
>
> The naming and description perhaps isn't ideal yet but it works in
> principle.
>
> The idea is that if you connect over a Unix-domain socket and the local
> (effective) user is the same as the server's (effective) user, then
> access should be granted immediately without any checking of
> pg_hba.conf.  Because it's "your own" server and you can do anything you
> want with it anyway.
>
> I included an option to turn this off because (a) people are going to
> complain, (b) you need this for the test suites to be able to test
> pg_hba.conf, and (c) conceivably, someone might want to have all access
> to go through pg_hba.conf for some auditing reasons (perhaps via PAM).
>
> This addresses the shortcomings of using peer as the default mechanism
> in initdb.  In a subsequent step, my idea would be to make the default
> initdb authentication setup to use md5 (or scram, tbd.) for both local
> and host.
>


This has been hanging around for a while. I guess the reason it hasn't
got much attention is that on its own it's not terribly useful.
However, when you consider that it's a sensible prelude to setting a
more secure default for auth in initdb (I'd strongly advocate
SCRAM-SHA-256 for that) it takes on much more significance.

The patch on its own is very small and straightforward, The actual
code is smaller than the docco.

Let's do this so we can move to a better default auth.

cheers

andrew

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

Re: [PATCH] Windows port add support to BCryptGenRandom

[Michael Paquier]

On Tue, Dec 17, 2019 at 03:57:56AM +, Ranier Vilela wrote:
> Windows Vista I believe.
> https://github.com/openssl/openssl/blob/master/crypto/rand/rand_win.c
> is the primary font and have more information.

So, this basically matches with what the MS documents tell us, and my
impression: this API is available down to at least MSVC 2008, which is
much more than what we support on HEAD where one can use MSVC 2013 and
newer versions.  Note that for the minimal platforms supported our
documentation cite Windows Server 2008 R2 SP1 and Windows 7, implying
_WIN32_WINNT >= 0x0600.

In short, this means two things:
- Your patch, as presented, is wrong.
- There is no need to make conditional the use of BCryptGenRandom.
--
Michael


signature.asc
Description: PGP signature

Re: segmentation fault when cassert enabled

[Amit Kapila]

On Mon, Dec 16, 2019 at 9:16 PM Jehan-Guillaume de Rorthais
 wrote:
>
> On Mon, 16 Dec 2019 13:27:43 +0100
> Peter Eisentraut  wrote:
>
> > On 2019-12-16 11:11, Amit Kapila wrote:
> > > I agree that this is a timing issue.  I also don't see a way to write
> > > a reproducible test for this.  However, I could reproduce it via
> > > debugger consistently by following the below steps.  I have updated a
> > > few comments and commit messages in the attached patch.
> > >
> > > Peter E., Petr J or anyone else, do you have comments or objections on
> > > this patch?  If none, then I am planning to commit (and backpatch)
> > > this patch in a few days time.
> >
> > The patch seems fine to me.  Writing a test seems hard.  Let's skip it.
> >

Okay.

> > The commit message has a duplicate "building"/"built" in the first sentence.
>
> I think the sentence is quite long. I had to re-read it to get it.
>
> What about:
>
>   This patch allows building the local relmap cache for a subscribed relation
>   after processing pending invalidation messages and potential relcache
>   updates.
>

Attached patch with updated commit message based on suggestions.  I am
planning to commit this tomorrow unless there are more comments.

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


0001-Fix-subscriber-invalid-memory-access-on-DDL.amit.2.patch
Description: Binary data

Re: Allow cluster owner to bypass authentication

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> The idea is that if you connect over a Unix-domain socket and the local
> (effective) user is the same as the server's (effective) user, then
> access should be granted immediately without any checking of
> pg_hba.conf.  Because it's "your own" server and you can do anything you
> want with it anyway.

While I understand where you're generally coming from, I'm not entirely
convinced that this is a good direction to go in.  Yes, you could go
change pg_hba.conf (maybe..)- but would doing so trigger an email to
someone else?  Would you really be able to change pg_hba.conf when you
consider more restrictive environments, like where there are SELinux
checks?  These days, a simple getpeerid() doesn't actually convey all of
the information about a process that would let you be confident that the
client really has the same access to the system that the running PG
server does.

> I included an option to turn this off because (a) people are going to
> complain, (b) you need this for the test suites to be able to test
> pg_hba.conf, and (c) conceivably, someone might want to have all access
> to go through pg_hba.conf for some auditing reasons (perhaps via PAM).

Auditing is certainly an important consideration.

> This addresses the shortcomings of using peer as the default mechanism
> in initdb.  In a subsequent step, my idea would be to make the default
> initdb authentication setup to use md5 (or scram, tbd.) for both local
> and host.

I'm definitely in favor of having 'peer' be used by default in initdb.

I am, however, slightly confused as to why we'd then want to, in a
subsequent step, make the default set up use md5 or scram...?

* Andrew Dunstan (andrew.duns...@2ndquadrant.com) wrote:
> This has been hanging around for a while. I guess the reason it hasn't
> got much attention is that on its own it's not terribly useful.
> However, when you consider that it's a sensible prelude to setting a
> more secure default for auth in initdb (I'd strongly advocate
> SCRAM-SHA-256 for that) it takes on much more significance.

I'm all for improving the default for auth in initdb, but why wouldn't
that be peer auth first, followed by SCRAM..?  If that's what you're
suggesting then great, but that wasn't very clear from the email text,
at least.  I've not done more than glanced at the patch.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: psql's \watch is broken

[Michael Paquier]

On Mon, Dec 16, 2019 at 11:40:07AM +0900, Michael Paquier wrote:
> As the concepts behind cancel_pressed and CancelRequested are
> different, we need to keep cancel_pressed and make psql use it.  And
> the callback used for WIN32 also needs to set the flag. I also think
> that we should do a better effort in documenting CancelRequested
> properly in cancel.c.  All that should be fixed as of the attached,
> tested on Linux and from a Windows console.  From a point of view of
> consistency, this actually brings back the code of psql to the same
> state as it was before a4fd3aa, except that we still have the
> refactored pieces.

Merging both flags can actually prove to be tricky, as we have some
code paths involving --single-step where psql visibly assumes that a
cancellation pressed does not necessarily imply one that succeeds is
there is a cancellation object around (ExecQueryTuples, tuple printing
and \copy).  So I have fixed the issue by making the code of psql
consistent with what we had before a4fd3aa.  I think that it should be
actually possible to merge CancelRequested and cancel_pressed while
keeping the user-visible changes acceptable, and this requires a very
careful lookup.
--
Michael


signature.asc
Description: PGP signature

Re: Wrong assert in TransactionGroupUpdateXidStatus

[Amit Kapila]

On Mon, Dec 16, 2019 at 8:53 AM Amit Kapila  wrote:
>
> On Sun, Dec 15, 2019 at 8:51 AM Robert Haas  wrote:
> >
> > On Thu, Dec 12, 2019 at 9:23 PM Amit Kapila  wrote:
> > > Do you think we need such an Assert after having StaticAssert for
> > > (THRESHOLD_SUBTRANS_CLOG_OPT <= PGPROC_MAX_CACHED_SUBXIDS)  and then
> > > an if statement containing (nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT)
> > > just before this Assert?  Sure, we can keep this for extra safety, but
> > > I don't see the need for it.
> >
> > I don't have strong feelings about it.
> >
>
> Okay, in that case, I am planning to push this patch [1] tomorrow
> morning unless I see any other comments.  I am also planning to
> backpatch this through 10 where it got introduced,
>

This was introduced in 11, so pushed and backpatched through 11.

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

Re: Windows port minor fixes

[Michael Paquier]

On Mon, Dec 16, 2019 at 07:57:10PM +0100, Juan José Santamaría Flecha wrote:
> If you want to address 2 unrelated issues, it makes little sense to use a
> single thread and 3 patches.

And if you actually group things together so as any individual looking
at your patches does not have to figure out which piece applies to
what, that's also better.  Anyway, the patch for putenv() is wrong in
the way the memory is freed, but this has been mentioned on another
thread.  We rely on MAXPGPATH heavily so your patch trying to change
the buffer length does not bring much, and the windows-crypt call is
also wrong based for the version handling, as discussed on another
thread.
--
Michael


signature.asc
Description: PGP signature

Re: Request to be allotted a project or a feature in pipeline

[Michael Paquier]

On Mon, Dec 16, 2019 at 03:09:51PM +0100, Laurenz Albe wrote:
> Code review is definitely part of software development practice, and
> reading and understanding the code of experienced developers can teach
> you a lot.  Another nice aspect is that this is an activity that can easily
> be adjusted to span three months; if you embark on a new feature, the
> three months may pass without your patch getting accepted.

Code review can be very challenging, but that's very fruitful in the
long-term as you gain experience reading other's code.  You will most
likely begin to dig into parts of the code you are not familiar of,
still there are a couple of areas which are more simple than others if
you want to get used to the Postgres code, like changes involving
in-core extensions or client binaries.  If you begin working on a
feature, I would recommend beginning with something small-ish.  And
even such things can sometimes get more complicated depending on the
reviews you get regarding issues you did not even imagine :)
--
Michael


signature.asc
Description: PGP signature

Re: Rework manipulation and structure of attribute mappings

[Amit Langote]

Hi Michael,

On Mon, Dec 9, 2019 at 11:57 AM Michael Paquier  wrote:
> On Fri, Dec 06, 2019 at 06:03:12PM +0900, Amit Langote wrote:
> > Sorry I don't understand this.  Do you mean we should rename the
> > routines left behind in tupconvert.c?  For example,
> > convert_tuples_by_name() doesn't really "convert" tuples, only builds
> > a map needed to do so.  Maybe build_tuple_conversion_map_by_name()
> > would be a more suitable name.
>
> I had no plans to touch this area nor to rename this layer because
> that was a bit out of the original scope of this patch which is to
> remove the confusion and random bets with map lengths.  I see your
> point though and actually a name like what you are suggesting reflects
> better what the routine does in reality. :p

Maybe another day. :)

> So, a couple of hours after looking at the code I am finishing with
> the updated and indented version attached.  What do you think?

Thanks for the updated patch.  I don't have any comments, except that
the text I suggested couple of weeks ago no longer reads clear:

+ * by DDL operating on inheritance and partition trees to convert fully
+ * transformed expression trees from parent rowtype to child rowtype or
+ * vice-versa.

Maybe:

...to adjust the Vars in fully transformed expression trees to bear
output relation's attribute numbers.

Thanks,
Amit

Re: PATCH: standby crashed when replay block which truncated in standby but failed to truncate in master node

[Michael Paquier]

On Mon, Dec 16, 2019 at 12:22:18PM +0900, Fujii Masao wrote:
> > +Detection of WAL records having references to invalid pages during
> > +recovery causes PostgreSQL to report
> > +an error, aborting the recovery. Setting
> > Well, that's not really an error.  This triggers a PANIC, aka crashes
> > the server.  And in this case the actual problem is that you may not
> > be able to move on with recovery when restarting the server again,
> > except if luck is on your side because you would continuously face
> > it..
> 
> So you're thinking that "report an error" should be changed to
> "trigger a PANIC"? Personally "report an error" sounds ok because
> PANIC is one of "error", I think. But if that misleads people,
> I will change the sentence.

In the context of a recovery, an ERROR is promoted to a FATAL, but
here are talking about something that bypasses the crash of the
server.  So this could bring confusion.  I think that the
documentation should be crystal clear about that, with two aspects
outlined when the parameter is disabled, somewhat like data_sync_retry
actually:
- A PANIC-level error is triggered.
- It crashes the cluster.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Improve documentation of REINDEX options

[Michael Paquier]

On Fri, Dec 13, 2019 at 10:28:33AM +0100, Josef Šimánek wrote:
> I have prepared patch to improve documentation for REINDEX. It
> should be more inline with another documentation pages.
> 
> You can see the change applied in attached file. Patch can be found at
> https://github.com/simi/postgres/pull/3 (diff -
> https://github.com/simi/postgres/pull/3.diff, patch -
> https://github.com/simi/postgres/pull/3.patch).

Please, always attach your patches to emails sent on this mailing
list.  If for a reason or another, the data located to with external
link is lost (imagine for example that your github account is gone or
that github is reduced to ashes), then such patches would be lost, and
anybody looking at this email 10 years from now would not know what
you have been writing about here.  I am attaching it here for the
archive's sake.

+where option can
be:
+
+VERBOSE
Why not...  We did that in the docs of ANALYZE for v11 when
introducing the parenthesized grammar flavor for the options
available.

-   Rebuild all the indexes on the table my_table:
+   Rebuild all the indexes on the table my_table
with progress report per index:

 
-REINDEX TABLE my_table;
+REINDEX (VERBOSE) TABLE my_table;
Not sure if this part brings much to the reader though.  It is not
like the command description of REINDEX is complicated with dozens
of option choices.
--
Michael
From 992fbb946daf0e5483319665c74a1a75dfea503b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Josef=20=C5=A0im=C3=A1nek?= 
Date: Fri, 13 Dec 2019 01:59:07 +0100
Subject: [PATCH] Unify REINDEX options documentation and extend examples.

---
 doc/src/sgml/ref/reindex.sgml | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 10881ab03a85..007cceb320ee 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURRENTLY ] name
+REINDEX [ ( option [, ...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURRENTLY ] name
+
+where option can be:
+
+VERBOSE
 
  
 
@@ -422,10 +426,10 @@ REINDEX INDEX my_index;
   
 
   
-   Rebuild all the indexes on the table my_table:
+   Rebuild all the indexes on the table my_table with progress report per index:
 
 
-REINDEX TABLE my_table;
+REINDEX (VERBOSE) TABLE my_table;
 
   
 


signature.asc
Description: PGP signature

Re: backup manifests

[Suraj Kharage]

Hi,


> I think that what we should actually do here is try to use simplehash.
>> Right now, it won't work for frontend code, but I posted some patches
>> to try to address that issue:
>>
>>
>> https://www.postgresql.org/message-id/CA%2BTgmob8oyh02NrZW%3DxCScB%2B5GyJ-jVowE3%2BTWTUmPF%3DFsGWTA%40mail.gmail.com
>>
>> That would have a few advantages. One, we wouldn't need to know the
>> number of elements in advance, because simplehash can grow
>> dynamically. Two, we could use the iteration interfaces to walk the
>> hash table.  Your solution to that is pgrhash_seq_search, but that's
>> actually not well-designed, because it's not a generic iterator
>> function but something that knows specifically about the 'touch' flag.
>> I incidentally suggest renaming 'touch' to 'matched;' 'touch' is not
>> bad, but I think 'matched' will be a little more recognizable.
>>
>
> Thanks for the suggestion. Will try to implement the same and update
> accordingly.
> I am assuming that I need to build the patch based on the changes that you
> proposed on the mentioned thread.
>
>

I have implemented the simplehash in backup validator patch as Robert
suggested. Please find attached 0002 patch for the same.

kindly review and let me know your thoughts.

Also attached the remaining patches. 0001 and 0003 are same as v2, only
patch version is bumped.


-- 
--

Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.


v3-0001-Backup-manifest-with-file-names-sizes-timestamps-.patch
Description: Binary data


v3-0002-Implementation-of-backup-validator.patch
Description: Binary data


v3-0003-Tap-test-case-patch-to-verify-the-backup-using-ve.patch
Description: Binary data

Re: non-exclusive backup cleanup is mildly broken

[Kyotaro Horiguchi]

At Tue, 17 Dec 2019 11:46:03 +0900, Fujii Masao  wrote 
in 
> On Tue, Dec 17, 2019 at 4:19 AM Robert Haas  wrote:
> >
> > On Sun, Dec 15, 2019 at 8:44 PM Kyotaro Horiguchi
> >  wrote:
> > > However I don't object to the restriction, couldn't we allow the
> > > cancel_before_shmem_exit to search for the given entry looping over
> > > the before_shmem_exit array? If we don't do that, an assrtion is needed
> > > instead.
> > >
> > > Since pg_stop_backup_v2 is the only caller to the function in the
> > > whole server code, making cancel_before_shmem_exit a bit wiser (and
> > > slower) cannot hurt anyone.
> >
> > That's actually not true. It's called from
> > PG_END_ENSURE_ERROR_CLEANUP. Still, it wouldn't cost a lot to fix this
> > that way. However, I think that it's better to fix it the other way,
> > as I mentioned in my original email.

Sorry. I knew that.

> +1
> 
> Not only PREPARE but also other commands that we may add in the future
> can cause the same issue, so it's better to address the root cause rather
> than working around by disallowing PREPARE.

I stand on that side. I'm not sure what we are thinking as the root
cause, but PREPARE is avoiding duplicate registration using the static
bool twophaseExitRegistered and the most reasonable way to fix the
crash of the current patch would be to do the same thing as
PREPARE. The attached does that and changes the if condition of
cancel_before_shmem_exit into assertion.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index cac5854fcf..6f13084c43 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -80,6 +80,7 @@ pg_start_backup(PG_FUNCTION_ARGS)
 	}
 	else
 	{
+		static bool exit_handler_registered = false;
 		MemoryContext oldcontext;
 
 		/*
@@ -91,7 +92,12 @@ pg_start_backup(PG_FUNCTION_ARGS)
 		tblspc_map_file = makeStringInfo();
 		MemoryContextSwitchTo(oldcontext);
 
-		before_shmem_exit(do_pg_abort_backup, BoolGetDatum(true));
+		/* We don't remove this handler so register it only once */
+		if (!exit_handler_registered)
+		{
+			before_shmem_exit(do_pg_abort_backup, BoolGetDatum(true));
+			exit_handler_registered = true;
+		}
 
 		startpoint = do_pg_start_backup(backupidstr, fast, NULL, label_file,
 		NULL, tblspc_map_file, false, true);
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 05d02c23f5..44a1b24009 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -389,11 +389,11 @@ on_shmem_exit(pg_on_exit_callback function, Datum arg)
 void
 cancel_before_shmem_exit(pg_on_exit_callback function, Datum arg)
 {
-	if (before_shmem_exit_index > 0 &&
-		before_shmem_exit_list[before_shmem_exit_index - 1].function
-		== function &&
-		before_shmem_exit_list[before_shmem_exit_index - 1].arg == arg)
-		--before_shmem_exit_index;
+	Assert(before_shmem_exit_index > 0 &&
+		   before_shmem_exit_list[before_shmem_exit_index - 1].function
+		   == function &&
+		   before_shmem_exit_list[before_shmem_exit_index - 1].arg == arg);
+	--before_shmem_exit_index;
 }
 
 /* 

Re: automating pg_config.h.win32 maintenance

[Michael Paquier]

On Mon, Dec 16, 2019 at 01:12:27PM +0100, Peter Eisentraut wrote:
> OK, here is an updated patch set that has all defines in one big Perl hash,
> and also requires that all symbols in pg_config.h.in are accounted for.
> (The indentation is from pgperltidy.)

The patch looks pretty clean.  I have a few minor comments.

-   if (/^AC_INIT\(\[PostgreSQL\], \[([^\]]+)\]/)
+   if (/^AC_INIT\(\[([^\]]+)\], \[([^\]]+)\], \[([^\]]+)\]/)
{
Why did you remove the bit about "PostgreSQL"?

+   ENABLE_GSS   => $self->{options}->{gss} ? 1 :
undef,
[...]
-   if ($self->{options}->{gss})
-   {
-   print $o "#define ENABLE_GSS 1\n";
I found the part about gss and nls better with the old style.  A
matter of taste, not really an objection.  And your style is actually
consistent with USE_ASSERT_CHECKING as well.

+   else
+   {
+   croak "missing: $macro";
+   }
[...]
+   if (scalar(keys %define) > 0)
+   {
+   croak "unused defines: @{[%define]}";
+   }
So you have checks both ways.  That's nice. 

+   open(my $i, '<', "src/include/pg_config.h.in")
+ || confess "Could not open pg_config.h.in\n";
+   open(my $o, '>', "src/include/pg_config.h")
+ || confess "Could not write to pg_config.h\n";
Failure to open pg_config.h.

Wouldn't it be better to remove pg_config_ext.h.win32 as well?
--
Michael


signature.asc
Description: PGP signature

Re: non-exclusive backup cleanup is mildly broken

[Kyotaro Horiguchi]

At Tue, 17 Dec 2019 15:11:55 +0900 (JST), Kyotaro Horiguchi 
 wrote in 
> At Tue, 17 Dec 2019 11:46:03 +0900, Fujii Masao  wrote 
> in 
> PREPARE. The attached does that and changes the if condition of
> cancel_before_shmem_exit into assertion.

The patch can cause removal of a wrong cleanup function on non-cassert
build. That might be unwanted. But I think the assertion is needed
anyway.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

empty column name in error message

[Amit Langote]

Hi,

I wonder if it's worthwhile to fix the following not-so-friendly error message:

create index on foo ((row(a)));
ERROR:  column "" has pseudo-type record

For example, the attached patch makes it this:

create index on foo ((row(a)));
ERROR:  column "row" has pseudo-type record

Note that "row" as column name has been automatically chosen by the caller.

Thanks,
Amit


ConstructTupleDescriptor-set-attname-earlier.patch
Description: Binary data

Re: Allow cluster owner to bypass authentication

[Andrew Dunstan]

> > This has been hanging around for a while. I guess the reason it hasn't
> > got much attention is that on its own it's not terribly useful.
> > However, when you consider that it's a sensible prelude to setting a
> > more secure default for auth in initdb (I'd strongly advocate
> > SCRAM-SHA-256 for that) it takes on much more significance.
>
> I'm all for improving the default for auth in initdb, but why wouldn't
> that be peer auth first, followed by SCRAM..?  If that's what you're
> suggesting then great, but that wasn't very clear from the email text,
> at least.



What this is suggesting is in effect, for the db owner only and only
on a Unix domain socket, peer auth falling back to whatever is in the
hba file. That makes setting something like scram-sha-256 as the
default more practicable.

If we don't do something like this then changing the default could
cause far more disruption than our users might like.

>  I've not done more than glanced at the patch.

That might pay dividends :-)

cheers

andrew


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