PostgreSQL and Homomorphic Encryption

[Tal Glozman]

Hello PostgreSQL Team,

I'm doing a project at my university (HU Berlin) involving homomorphic
encrypted searches on data bases. Does PostgreSQL support homomorphic
encryption-based searches? I couldn’t find the information about it on the
internet. Could you provide me a web link or a PDF on the matter?

If you can not answer my question, can you give me an email of  someone or
another support group who can?

Thanks a lot,
Tal Glozman

Re: [HACKERS] Transactions involving multiple postgres foreign servers

[Masahiko Sawada]

On Mon, May 21, 2018 at 10:42 AM, Tsunakawa, Takayuki
 wrote:
> From: Masahiko Sawada [mailto:sawada.m...@gmail.com]
>> Regarding to API design, should we use 2PC for a distributed
>> transaction if both two or more 2PC-capable foreign servers and
>> 2PC-non-capable foreign server are involved with it?  Or should we end
>> up with an error? the 2PC-non-capable server might be either that has
>> 2PC functionality but just disables it or that doesn't have it.
>
>>but I think we also could take
>> the latter way because it doesn't make sense for user even if the
>> transaction commit atomically among not all participants.
>
> I'm for the latter.  That is, COMMIT or PREPARE TRANSACTION statement issued 
> from an application reports an error.

I'm not sure that we should end up with an error in such case, but if
we want then we can raise an error when the transaction tries to
modify 2PC-non-capable server after modified 2PC-capable server.

> DBMS, particularly relational DBMS (, and even more particularly Postgres?) 
> places high value on data correctness.  So I think transaction atomicity 
> should be preserved, at least by default.  If we preferred updatability and 
> performance to data correctness, why don't we change the default value of 
> synchronous_commit to off in favor of performance?  On the other hand, if we 
> want to allow 1PC commit when not all FDWs support 2PC, we can add a new GUC 
> parameter like "allow_nonatomic_commit = on", just like synchronous_commit 
> and fsync trade-offs data correctness and performance.

Honestly I'm not sure we should use atomic commit by default at this
point. Because it also means to change default behavior though the
existing users use them without 2PC. But I think control of global
transaction atomicity by GUC parameter would be a good idea. For
example, synchronous_commit = 'global' makes backends wait for
transaction to be resolved globally before returning to the user.

>
>
>> Also, regardless whether we take either way  I think it would be
>> better to manage not only 2PC transaction but also non-2PC transaction
>> in the core and add two_phase_commit argument. I think we can use it
>> without breaking existing FDWs. Currently FDWs manage transactions
>> using XactCallback but new APIs being added also manage transactions.
>> I think it might be better if users use either way (using XactCallback
>> or using new APIs) for transaction management rather than use both
>> ways with combination. Otherwise two codes for transaction management
>> will be required: the code that manages foreign transactions using
>> XactCallback for non-2PC transactions and code that manages them using
>> new APIs for 2PC transactions. That would not be easy for FDW
>> developers. So what I imagined for new API is that if FDW developers
>> use new APIs they can use both 2PC and non-2PC transaction, but if
>> they use XactCallback they can use only non-2PC transaction.
>> Any thoughts?
>
> If we add new functions, can't we just add functions whose names are 
> straightforward like PrepareTransaction() and CommitTransaction()?  FDWs 
> without 2PC support returns NULL for the function pointer of 
> PrepareTransaction().
>
> This is similar to XA: XA requires each RM to provide function pointers for 
> xa_prepare() and xa_commit().  If we go this way, maybe we could leverage the 
> artifact of postgres_fdw to create the XA library for C/C++.  I mean we put 
> transaction control functions in the XA library, and postgres_fdw also uses 
> it.  i.e.:
>
> postgres_fdw.so -> libxa.so -> libpq.so
>  \-/

I might not understand your comment correctly but the current patch is
implemented in such way. The patch introduces new FDW APIs:
PrepareForeignTransaction, EndForeignTransaction,
ResolvePreparedForeignTransaction and GetPreapreId. The postgres core
calls each APIs at appropriate timings while managing each foreign
transactions. FDWs that don't support 2PC set the function pointers of
them to NULL.

Also, regarding the current API design it might not fit to other
databases than PostgreSQL. For example, in MySQL we have to start xa
transaction explicitly using by XA START whereas PostgreSQL can
prepare the transaction that is started by BEGIN TRANSACTION. So in
MySQL global transaction id is required at beginning of xa
transaction. And we have to execute XA END is required before we
prepare or commit it at one phase. So it would be better to define
APIs according to X/Open XA in order to make it more general.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: PostgreSQL and Homomorphic Encryption

[Craig Ringer]

On 22 May 2018 at 14:34, Tal Glozman  wrote:

> Hello PostgreSQL Team,
>
> I'm doing a project at my university (HU Berlin) involving homomorphic
> encrypted searches on data bases. Does PostgreSQL support homomorphic
> encryption-based searches?
>

Not natively, as far as I know anyway.

I don't think pgcrypto offers any facilities you could use for useful
homomorphic encryption, except for the obvious degenerate case of
comparison by equality of unsalted encrypted data.

PostgreSQL is very extensible and you could definitely add data types for
homomorphic encryption + the required index access method implementations
etc. A quick Google search for "postgres homomorphic" finds various
information on the topic.

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

Re: Fix some error handling for read() and errno

[Kyotaro HORIGUCHI]

Hello.

At Sun, 20 May 2018 09:05:22 +0900, Michael Paquier  wrote 
in <2018052522.gb1...@paquier.xyz>
> Hi all,
> 
> This is basically a new thread after what has been discussed for
> pg_controldata with its error handling for read():
> https://www.postgresql.org/message-id/CABUevEx8ZRV5Ut_FvP2etXiQppx3xVzm7oOaV3AcdHxX81Yt8Q%40mail.gmail.com
> 
> While reviewing the core code, I have noticed similar weird error
> handling for read().  At the same time, some of those places may use an
> incorrect errno, as an error is invoked using an errno which may be
> overwritten by another system call.  I found a funny one in slru.c,
> for which I have added a note in the patch.  I don't think that this is
> worth addressing with more facility, thoughts are welcome.
> 
> Attached is a patch addressing the issues I found.

I see the same issue in snapbuild.c(4 places).

| readBytes = read(fd, &ondisk, SnapBuildOnDiskConstantSize);
| pgstat_report_wait_end();
| if (readBytes != SnapBuildOnDiskConstantSize)
| {
|   CloseTransientFile(fd);
|   ereport(ERROR,
|   (errcode_for_file_access(),
|errmsg("could not read file \"%s\", read %d of %d: %m",
|   path, readBytes, (int) SnapBuildOnDiskConstantSize)));
| }

and walsender.c (2 places)

|   if (nread <= 0)
| ereport(ERROR,
| (errcode_for_file_access(),
|  errmsg("could not read file \"%s\": %m",
| path)));

and pg_receivewal.c

| if (read(fd, (char *) buf, sizeof(buf)) != sizeof(buf))
| {
|   fprintf(stderr, _("%s: could not read compressed file \"%s\": %s\n"),
|   progname, fullpath, strerror(errno));

pg_waldump.c

| if (readbytes <= 0)
...
|   fatal_error("could not read from log file %s, offset %u, length %d: %s",
| fname, sendOff, segbytes, strerror(err));


A bit differnt issue, but in pg_waldump.c, search_directory can
check uninitialized errno when read returns a non-zero value.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: perl checking

[Kyotaro HORIGUCHI]

At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan 
 wrote in 
<5a6d6de8-cff8-1ffb-946c-ccf381800...@2ndquadrant.com>
> 
> These two small patches allow us to run "perl -cw" cleanly on all our
> perl code.
> 
> 
> One patch silences a warning from convutils.pl about the unportability
> of the literal 0x1. We've run for many years without this
> giving us a problem, so I think we can turn the warning off pretty
> safely.

It was introduced by aeed17d000 (in 2017). The history of the
file is rather short. Over 32-bit values do not apperar as a
character so there's no problem in ignoring the warning for now,
but can't we use bigint to silence it instead?

> The other patch provides a dummy library that emulates just enough of
> the Win32 perl infrastructure to allow us to run these checks. That
> means that Unix-based developers who might want to make changes in the
> msvc code can actually run a check against their code without having
> to put it on a Windows machine. The invocation goes like this (to
> check Mkvcbuild.pl for example):
> 
> 
>PERL5LIB=src/tools/msvc/dummylib perl -cw src/tools/Mkvcbuild.pm
> 
> 
> This also allows us to check src/tools/win32tzlist.pl.
> 
> 
> In due course I'll submit a script to automate this syntax checking.
> 
> 
> cheers
> 
> 
> andrew

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:
> The previous patch has actually problems with servers using "trust",
> "password" and any protocol which can send directly AUTH_REQ_OK as those
> could still enforce a downgrade to not use channel binding, so we
> actually need to make sure that AUTH_REQ_SASL_FIN has been received when 
> channel binding is required when looking at a AUTH_REQ_OK message.

Okay, so I have digested the previous comments with the attached.
scram_channel_binding is modified as follows (from commit message): 
- "prefer", is the default and behaves so as channel binding is used if
available.  If the cluster does not support it then it is not used. This
does not protect from downgrade attacks.
- "disable", which is the equivalent of the empty value previously,
disables channel binding.
- "tls-unique" and "tls-server-end-point" make channel binding a
requirement and use the channel binding of the same name for
connection.  Note that in this case, if the connection is attempted
without SSL or if server does not support channel binding then libpq
refuses the connection as well.

In order to make sure that a client is not tricked by a "trust"
connection downgrade which sends back directly AUTH_REQ_OK, one way I
have thought about is to check if the client has achieved with a server
a full SASL exchange when receiving this message type, which is
something that we know about as the exchange state is saved in
PGconn->sasl_state.

The patch includes documentation and tests, which check that connections
are refused without SSL and or if the server downgrades authentication
requests.

Thanks,
--
Michael
From 5bf51e7bdcfaf2d6e8af5132bb7884bc307f440b Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 22 May 2018 17:03:48 +0900
Subject: [PATCH] Rework scram_channel_binding to protect from downgrade
 attacks

When a client attempts to connect to a PostgreSQL cluster, it may be
possible that it requested channel binding with SCRAM authentication,
but that the server tricks the clister and forcibly downgrades the
authentication request.  For example, a v10 cluster supports SCRAM but
not channel binding so authentication could be achieved without channel
binding used.

In order to protect from such attacks, rework libpq handling of the
connection parameter scram_channel_binding as follows:
- "prefer", is the default and behaves so as channel binding is used if
available.  If the cluster does not support it then it is not used.
This does not protect from downgrade attacks.
- "disable", which is the equivalent of the empty value previously,
disables channel binding.
- "tls-unique" and "tls-server-end-point" make channel binding a
requirement and use the channel binding of the same name for
connection.

For both "tls-unique" and "tsl-server-end-point, if the cluster does not
support channel binding with SCRAM (v10 server) or if SSL is not used,
then the connection is refused by libpq, which is something that can
happen if SSL is not used (prevention also for users forgetting to use
sslmode=require at least in connection strings).  This also allows users
to enforce the use of SCRAM with channel binding even if the targetted
cluster downgrades to "trust" or similar.

In order to achieve that, when receiving AUTH_REQ_OK from the server, we
check if a SASL exchange has happened and has finished, where the client
makes sure that the server knows the connection proof.  If the cluster
does not publish the -PLUS mechanism, then connection is also refused.

Discussion: https://postgr.es/m/20180519123557.gb2...@paquier.xyz
---
 doc/src/sgml/libpq.sgml  | 34 +++-
 src/interfaces/libpq/fe-auth-scram.c | 59 ++--
 src/interfaces/libpq/fe-auth.c   | 57 +--
 src/interfaces/libpq/fe-auth.h   |  4 +-
 src/interfaces/libpq/fe-connect.c| 20 +-
 src/test/ssl/ServerSetup.pm  | 22 +++
 src/test/ssl/t/002_scram.pl  | 38 --
 7 files changed, 198 insertions(+), 36 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 498b8df988..f1fa744a8b 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1250,18 +1250,34 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 

 The list of channel binding types supported by the server are
-listed in .  An empty value
-specifies that the client will not use channel binding.  If this
-parameter is not specified, tls-unique is used,
-if supported by both server and client.
-Channel binding is only supported on SSL connections.  If the
-connection is not using SSL, then this setting is ignored.
+listed in .  A value of
+disable disables channel binding completely,
+hence the client decides to not use it even if the server supports
+it.  A value of prefer, the default, means that
+channel binding

Re: [PATCH] Clear up perlcritic 'missing return' warning

[Michael Paquier]

On Mon, May 21, 2018 at 08:07:03PM -0500, Mike Blackwell wrote:
> This particular patch addresses the warning caused by falling off the end
> of a subroutine rather than explicitly returning.

Do we really want to make that a requirement?  Making sure that there is
a return clause if a subroutine returns a result makes sense to me, but
making it mandatory if the subroutine does not return anything and
callers don't expect it do is not really a gain in my opinion.  And this
maps with any C code.

Just today, I coded a perl subroutine which does not return any
results...  This is most likely going to be forgotten.
--
Michael


signature.asc
Description: PGP signature

Re: Time to put context diffs in the grave

[Michael Paquier]

On Mon, May 21, 2018 at 09:51:11PM -0400, Andrew Dunstan wrote:
> Unless someone objects really violently I'm going to rip all that stuff out
> and let sanity prevail.

Please!
--
Michael


signature.asc
Description: PGP signature

Update backend/lib/README

[Ideriha, Takeshi]

Hi, 

When I looked into the dshash.c, I noticed that dshash.c, bipartite_match.c and 
knapsack.c are not mentioned at README.
The other files at src/backend/lib are mentioned. I'm not sure this is an 
intentional one or just leftovers.

Does anyone have opinions?

Patch attached.
- add summary of three files mentioned above (copied from each file's comment)
- change the list to alphabetical order

===
Takeshi Ideriha
Fujitsu Limited




backend_lib_readme.patch
Description: backend_lib_readme.patch

A Japanese-unfriendy error message contruction

[Kyotaro HORIGUCHI]

Hello.

While I revised the translation, I ran across the following code.

>form_policy = (Form_pg_policy) GETSTRUCT(tuple);
>
>appendStringInfo(&buffer, _("policy %s on "),
> NameStr(form_policy->polname));
>getRelationDescription(&buffer, form_policy->polrelid);

getRelationDescription appends a string like "table %s" to the
buffer so finally a message like "policy %s on table %s" is
constructed but this is very unfriendly to Japanese syntax.

The "policy %s" and " %s" are transposed in Japaese
syntax.  Specifically " %s NO  %s" is the
natural translation of "policy %s on  %s". But currently
we cannot get the natural error message in Japanese.

For the reason, I'd like to propose to refactor
getObjectDescription:OPCLASS_POLICY as the attached patch. The
same structure is seen for OPCLASS_AMOP.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index d371c47842..baa77e4e79 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -3356,6 +3356,7 @@ getObjectDescription(const ObjectAddress *object)
 SysScanDesc sscan;
 HeapTuple	tuple;
 Form_pg_policy form_policy;
+StringInfoData reldesc;
 
 policy_rel = heap_open(PolicyRelationId, AccessShareLock);
 
@@ -3375,9 +3376,13 @@ getObjectDescription(const ObjectAddress *object)
 
 form_policy = (Form_pg_policy) GETSTRUCT(tuple);
 
-appendStringInfo(&buffer, _("policy %s on "),
- NameStr(form_policy->polname));
-getRelationDescription(&buffer, form_policy->polrelid);
+initStringInfo(&reldesc);
+getRelationDescription(&reldesc, form_policy->polrelid);
+
+appendStringInfo(&buffer, _("policy %s on %s"),
+ NameStr(form_policy->polname),
+ reldesc.data);
+pfree(reldesc.data);
 
 systable_endscan(sscan);
 heap_close(policy_rel, AccessShareLock);

Re: Transform for pl/perl

[Anthony Bykov]

On Wed, 02 May 2018 17:41:38 +0100
ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) wrote:

> Peter Eisentraut  writes:
> 
> > These two items are now outstanding:
> >
> > On 4/10/18 07:33, Dagfinn Ilmari Mannsåker wrote:  
> >> 2) jsonb scalar values are passed to the plperl function wrapped
> >> in not one, but _two_ layers of references  
> >
> > I don't understand this one, or why it's a problem, or what to do
> > about it.  
> 
> It means that if you call a jsonb-transforming pl/perl function like
> 
>select somefunc(jsonb '42');
> 
> it receives not the scalar 42, but reference to a reference to the
> scalar (**int instead of an int, in C terms).  This is not caught by
> the current round-trip tests because the output transform
> automatically dereferences any number of references on the way out
> again.
> 
> The fix is to reshuffle the newRV() calls in Jsonb_to_SV() and
> jsonb_to_plperl().  I am working on a patch (and improved tests) for
> this, but have not have had time to finish it yet.  I hope be able to
> in the next week or so.
> 
> >> 3) jsonb numeric values are passed as perl's NV (floating point)
> >> type, losing precision if they're integers that would fit in an IV
> >> or UV.  
> >
> > This seems fixable, but perhaps we need to think through whether
> > this will result in other strange behaviors.  
> 
> Nubers > 2⁵³ are not "interoperable" in the sense of the JSON spec,
> because JavaScript only has doubles, but it seems desirable to
> preserve whatever precision one reasonably can, and I can't think of
> any downsides.  We already support the full numeric range when
> processing JSONB in SQL, it's just in the PL/Perl transform (and
> possibly PL/Python, I didn't look) we're losing precision.
> 
> Perl can also be configured to use long double or __float128 (via
> libquadmath) for its NV type, but I think preserving 64bit integers
> when building against a Perl with a 64bit integer type would be
> sufficient.
> 
> - ilmari

Hello,
need any help with the patch?

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Time to put context diffs in the grave

[Kyotaro HORIGUCHI]

At Mon, 21 May 2018 21:51:11 -0400, Andrew Dunstan 
 wrote in 
<247f4d65-31f3-d426-2b24-fc434fa67...@2ndquadrant.com>
> 
> We haven't insisted on context diffs in years now, and one of my
> interlocutors has just turned handsprings trying to follow the advice
> at  to produce his
> first patch.
> 
> 
> Unless someone objects really violently I'm going to rip all that
> stuff out and let sanity prevail.

+1.

filterdiff has a bug that we can lose a part of hunks with it,
and I actually have stepped on it. I saw Álvaro made a complaint
about the bug but it doesn't seem to have been fixed. It is the
most major reason I'm posting patches in unified format
hesitently (really I am!). The second reason is git format-patch
doesn't give me diffs in context format.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Update backend/lib/README

[Heikki Linnakangas]

On 22/05/18 11:44, Ideriha, Takeshi wrote:

Hi,

When I looked into the dshash.c, I noticed that dshash.c, bipartite_match.c and 
knapsack.c are not mentioned at README.
The other files at src/backend/lib are mentioned. I'm not sure this is an 
intentional one or just leftovers.

Does anyone have opinions?

Patch attached.
- add summary of three files mentioned above (copied from each file's comment)
- change the list to alphabetical order


Seems reasonable. Pushed, thanks!

- Heikki

Re: [WIP PATCH] Index scan offset optimisation using visibility map

[Andrey Borodin]

Hi, Michail!

> 21 мая 2018 г., в 20:43, Michail Nikolaev  
> написал(а):
> This letter related to “Extended support for index-only-scan” from my 
> previous message in the thread. 
This is great that you continue your work in this direction! The concept of 
scan that you propose looks interesting.
I have few questions:
1. Charts are measured in percents of pgbench TPS, right?
2. For example, is 97% actually 3% degrade?
3. The results are obtained on actual "sort of TPC-B" script?

Best regards, Andrey Borodin.

plperl fails with perl 5.28

[Christoph Berg]

plperl fails to install with perl 5.27.11, which is to be released as 5.28.0:

2018-05-17 20:01:44.556 UTC [22629] pg_regress ERROR:
2018-05-17 20:01:44.556 UTC [22629] pg_regress CONTEXT:  while running Perl 
initialization
2018-05-17 20:01:44.556 UTC [22629] pg_regress STATEMENT:  CREATE EXTENSION IF 
NOT EXISTS "plperl"

Unfortunately this is all what I could extract from the server log,
even log_min_messages = debug5 doesn't print more.

Origin: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=899209

Christoph

PostgreSQL “tuple already updated by self”

[SG]

Our database seems to be broken, normally it uses about 1-2% of cpu, but if
we run some additional backend services making UPDATE and INSERT queries
for 10M rows table (about 1 query per 3 second) everything is going to hell
(including CPU increase from 2% to 98% usage).

We have decided to debug what's going on, run VACUUM and ANALYZE to learn
what's wrong with db but...

production=# ANALYZE VERBOSE users_user;
INFO:  analyzing "public.users_user"
INFO:  "users_user": scanned 280 of 280 pages, containing 23889 live
rows and 57 dead rows; 23889 rows in sample, 23889 estimated total
rows
INFO:  analyzing "public.users_user"
INFO:  "users_user": scanned 280 of 280 pages, containing 23889 live
rows and 57 dead rows; 23889 rows in sample, 23889 estimated total
rows
ERROR:  tuple already updated by self

we are not able to finish ANALYZE on ANY of the tables and could not find
any information about this issue. Any suggestions what can be wrong?

 PostgreSQL 9.6.8 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.8.5
20150623 (Red Hat 4.8.5-16), 64-bit


Stackoverflow thread:
https://stackoverflow.com/questions/50450162/postgresql-tuple-already-updated-by-self?noredirect=1#comment87937347_50450162

Re: [WIP PATCH] Index scan offset optimisation using visibility map

[Michail Nikolaev]

Hello.

> 1. Charts are measured in percents of pgbench TPS, right?
Yes, correct. Actual values are calculated as TPS_of_patched /
TPS_of_vanilla. TPS was measured using single postgres process (one core)
(I was also did tests with multiple processes, but they shows pretty same
results).

> 2. For example, is 97% actually 3% degrade?
Yes, such degrade happens for indexes with high correlation and predicates
with low selectivity. In such cases 2-4% overhead is caused by index data
read and page visibility check. But it is possible to detect such cases in
planner and use regular IndexScan instead.

> 3. The results are obtained on actual "sort of TPC-B" script?
You could check testing script (
https://gist.github.com/michail-nikolaev/23e1520a1db1a09ff2b48d78f0cde91d)
for SQL queries.

But briefly:
* Vanilla pg_bench initialization
* ALTER TABLE pgbench_accounts drop constraint pgbench_accounts_pkey; --
drop non-required constraint
* UPDATE pgbench_accounts SET bid = TRUNC(RANDOM() * {ROWS_N} + 1 --
randomize BID (used for selectivy predicate)
* UPDATE pgbench_accounts SET aid = TRUNC(RANDOM() * {ROWS_N} + 1) WHERE
random() <= (1.0 - {CORRELATION}) -- emulate index correlation by changing
some part of AID values
* CREATE index test_index ON pgbench_accounts USING btree(aid, bid) --
create index used for test
* VACUUM FULL;
* VACUUM ANALYZE pgbench_accounts;
* SELECT * FROM pgbench_accounts WHERE aid > {RANDOM} and bid % 100 <=
{SELECTIVITY} order by aid limit 50

Thanks,
Michail.

Re: perl checking

[Andrew Dunstan]

On 05/22/2018 04:11 AM, Kyotaro HORIGUCHI wrote:

At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan  
wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800...@2ndquadrant.com>

These two small patches allow us to run "perl -cw" cleanly on all our
perl code.


One patch silences a warning from convutils.pl about the unportability
of the literal 0x1. We've run for many years without this
giving us a problem, so I think we can turn the warning off pretty
safely.

It was introduced by aeed17d000 (in 2017). The history of the
file is rather short. Over 32-bit values do not apperar as a
character so there's no problem in ignoring the warning for now,
but can't we use bigint to silence it instead?




It would impose an additional dependency. bigint isn't installed by 
default on many systems AFAICT, so I think we'd need a better reason 
than this to require it.


I was a little optimistic about claiming that 'perl -cw' would run 
cleanly with these two patches - there's a little remediation that will 
be required in the src/msvc/tools directory. These patches at least let 
it run to completion.


cheers

andrew

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

Re: Fix for FETCH FIRST syntax problems

[Robert Haas]

On Sun, May 20, 2018 at 5:16 PM, Tom Lane  wrote:
> "David G. Johnston"  writes:
>> The risk here is significantly reduced since the existing user-visible
>> behavior is an error which presumably no one is relying upon.  Between that
>> and being able to conform to the standard syntax for a long-standing
>> feature I would say the benefit outweighs the cost and risk.
>
> The risk you're ignoring is that this patch will break something that
> *did* work before.  Given that the first version did exactly that,
> I do not think that risk should be considered negligible.  I'm going
> to change my vote for back-patching from -0.5 to -1.

I'm also -1 for back-patching, although it seems that the ship has
already sailed.  I don't think that the failure of something to work
that could have been made to work if the original feature author had
tried harder rises to the level of a bug.  If we start routinely
back-patching things that fall into that category, we will certainly
manage to destabilize older releases on a regular basis.

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

Re: A Japanese-unfriendy error message contruction

[Euler Taveira]

2018-05-22 6:20 GMT-03:00 Kyotaro HORIGUCHI :
> For the reason, I'd like to propose to refactor
> getObjectDescription:OPCLASS_POLICY as the attached patch. The
> same structure is seen for OPCLASS_AMOP.
>
+1.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: PostgreSQL “tuple already updated by self”

[Robert Haas]

On Tue, May 22, 2018 at 8:01 AM, SG  wrote:
> Our database seems to be broken, normally it uses about 1-2% of cpu, but if
> we run some additional backend services making UPDATE and INSERT queries for
> 10M rows table (about 1 query per 3 second) everything is going to hell
> (including CPU increase from 2% to 98% usage).
>
> We have decided to debug what's going on, run VACUUM and ANALYZE to learn
> what's wrong with db but...
>
> production=# ANALYZE VERBOSE users_user;
> INFO:  analyzing "public.users_user"
> INFO:  "users_user": scanned 280 of 280 pages, containing 23889 live rows
> and 57 dead rows; 23889 rows in sample, 23889 estimated total rows
> INFO:  analyzing "public.users_user"
> INFO:  "users_user": scanned 280 of 280 pages, containing 23889 live rows
> and 57 dead rows; 23889 rows in sample, 23889 estimated total rows
> ERROR:  tuple already updated by self
>
> we are not able to finish ANALYZE on ANY of the tables and could not find
> any information about this issue. Any suggestions what can be wrong?

Well, as Laurenz Albe said on the Stack Overflow thread, it sure looks
like ANALYZE is finding the same table (public.users_user) twice,
which it shouldn't do.  So something is probably wrong with pg_class.
He suggested a duplicate heap tuple, which could well be right, or
maybe it's a broken HOT chain or a bad index or something.  If it were
me, I'd probably try what he suggested and then get busy with
pageinspect and/or pg_filedump if that didn't work.

Another interesting question is how the system got into this state in
the first place, of course.  Was it a bug, or did you do something
that corrupted the database?

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

Re: perl checking

[Tom Lane]

Andrew Dunstan  writes:
> On 05/22/2018 04:11 AM, Kyotaro HORIGUCHI wrote:
>> At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan 
>>  wrote in 
>> <5a6d6de8-cff8-1ffb-946c-ccf381800...@2ndquadrant.com>
>>> One patch silences a warning from convutils.pl about the unportability
>>> of the literal 0x1. We've run for many years without this
>>> giving us a problem, so I think we can turn the warning off pretty
>>> safely.

>> It was introduced by aeed17d000 (in 2017). The history of the
>> file is rather short. Over 32-bit values do not apperar as a
>> character so there's no problem in ignoring the warning for now,
>> but can't we use bigint to silence it instead?

> It would impose an additional dependency. bigint isn't installed by 
> default on many systems AFAICT, so I think we'd need a better reason 
> than this to require it.

I agree with not adding a dependency (although FWIW, bigint does seem
to be there in my minimal perl setups).  But can't we fix it like this:

-   elsif ($in < 0x1)
+   elsif ($in <= 0x)

At least in a quick test here, "-cw" doesn't moan about 0x.

For consistency, the other arms of the "if" should be adjusted
similarly.

regards, tom lane

Re: Fix for FETCH FIRST syntax problems

[David G. Johnston]

On Tue, May 22, 2018 at 5:59 AM, Robert Haas  wrote:

> If we start routinely
> back-patching things that fall into that category, we will certainly
> manage to destabilize older releases on a regular basis.
>

Just because something is bad if done in excess doesn't mean specific
moderate partaking is bad too.

We actually did backpatch the NaN stuff and reverted that because, for me,
it was a silent change of functioning behavior.​  I find the decision to
back-patch this syntax oversight considerably more obvious than that one
was.

David J.

Re: ppc64le support in 9.3 branch?

[Christoph Berg]

Re: Tom Lane 2018-03-23 <1219.1521831...@sss.pgh.pa.us>
> The compromise I'm inclined to offer is to see what happens if we
> back-patch 9.4's config.guess and config.sub.  If that makes these
> animals go green, and doesn't break any others, we'll call it good.
> Otherwise, we revert that change and say we're not putting any
> additional effort into it.

Fwiw, apt.postgresql.org has been building+regression testing 9.3 on
ppc64el on various Debian+Ubuntu releases for more than a year now.
The only requirement is updating config.guess/sub.

Christoph

PG11 jit failing on ppc64el

[Christoph Berg]

PG 11 beta1 is failing on ppc64el. All Debian/Ubuntu releases except
Jessie are affected. My guess is on problems with llvm/jit, because of
the C++ style error message (and LLVM is disabled on Jessie).

Debian sid:
15:59:29 2018-05-22 13:59:24.914 UTC [29081] pg_regress/strings STATEMENT:  
SELECT chr(0);
15:59:29 terminate called after throwing an instance of 'std::bad_function_call'
15:59:29   what():  bad_function_call
15:59:29 2018-05-22 13:59:25.026 UTC [28961] LOG:  server process (PID 29085) 
was terminated by signal 6: Aborted
15:59:29 2018-05-22 13:59:25.026 UTC [28961] DETAIL:  Failed process was 
running: INSERT INTO TEMP_GROUP
15:59:29  SELECT 1, (- i.f1), (- f.f1)
15:59:29  FROM INT4_TBL i, FLOAT8_TBL f;
15:59:29 2018-05-22 13:59:25.026 UTC [28961] LOG:  terminating any other active 
server processes
15:59:29 2018-05-22 13:59:25.026 UTC [29078] WARNING:  terminating connection 
because of crash of another server process

Debian stretch:
15:58:45 2018-05-22 13:58:43.778 UTC [29981] pg_regress/indexing STATEMENT:  
insert into fastpath values (1, 'b1', 100.00);
15:58:45 terminate called after throwing an instance of 'std::bad_function_call'
15:58:45   what():  bad_function_call
15:58:45 2018-05-22 13:58:43.975 UTC [28908] LOG:  server process (PID 29981) 
was terminated by signal 6: Aborted
15:58:45 2018-05-22 13:58:43.975 UTC [28908] DETAIL:  Failed process was 
running: select md5(string_agg(a::text, b order by a, b asc)) from fastpath
15:58:45where a >= 1000 and a < 2000 and b > 'b1' and b < 'b3';
15:58:45 2018-05-22 13:58:43.975 UTC [28908] LOG:  terminating any other active 
server processes
15:58:45 2018-05-22 13:58:43.975 UTC [30037] WARNING:  terminating connection 
because of crash of another server process

Christoph

COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[David Rowley]

I'd been looking over the COPY FROM code tonight when I saw something
pretty scary looking:

/* on input just throw the header line away */
if (cstate->cur_lineno == 0 && cstate->header_line)
{
cstate->cur_lineno++;
if (CopyReadLine(cstate))
   return false; /* done */
}

while it might not look too scary by itself, it gets a bit more so
when you learn that the cur_lineno is only 32 bits wide. This will
result in skipping a tuple every time the 32-bit variable wraps back
around to 0 again.

Maybe when this code was written copying > 4 billion rows was just a
far-off dream, but with today's hardware, it really didn't take that
long to see this actually happen for real.

postgres=# create unlogged table t(a int);
CREATE TABLE
Time: 1.339 ms
postgres=# insert into t select 0 from generate_series(1, 43);
INSERT 0 43
Time: 2128367.019 ms (35:28.367)
postgres=# copy t to '/home/ubuntu/t.csv' with (format csv, header);
COPY 43
Time: 2294331.494 ms (38:14.331)
postgres=# truncate t;
TRUNCATE TABLE
Time: 30.367 ms
postgres=# copy t from '/home/ubuntu/t.csv' with (format csv, header);
COPY 42
Time: 2693186.475 ms (44:53.186)

A patch to fix is attached.

(I just made the variable 64-bit)

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


0001-Fix-COPY-FROM-not-to-skip-a-tuple-every-2-32-tuples.patch
Description: Binary data

Re: PG11 jit failing on ppc64el

[Andres Freund]

Hi,

On 2018-05-22 16:33:57 +0200, Christoph Berg wrote:
> PG 11 beta1 is failing on ppc64el. All Debian/Ubuntu releases except
> Jessie are affected. My guess is on problems with llvm/jit, because of
> the C++ style error message (and LLVM is disabled on Jessie).

It was bug in LLVM that's fixed now. I guess you can either disable jit
on arm or ask the LLVM maintainer to backport it...

r328687 - but the expanded tests created a few problems (windows mainly,
but somewhere else too), so I'd just backport the actual code change.

- Andres

Re: Postgres, fsync, and OSs (specifically linux)

[Dmitry Dolgov]

> On 22 May 2018 at 03:08, Andres Freund  wrote:
> On 2018-05-19 18:12:52 +1200, Thomas Munro wrote:
>> On Sat, May 19, 2018 at 4:51 PM, Thomas Munro
>>  wrote:
>> > Next, make check hangs in initdb on both of my pet OSes when md.c
>> > raises an error (fseek fails) and we raise and error while raising and
>> > error and deadlock against ourselves.  Backtrace here:
>> > https://paste.debian.net/1025336/
>>
>> Ah, I see now that something similar is happening on Linux too, so I
>> guess you already knew this.
>
> I didn't. I cleaned something up and only tested installcheck
> after... Singleuser mode was broken.
>
> Attached is a new version.
>
> I've changed my previous attempt at using transient files to using File
> type files, but unliked from the LRU so that they're kept open. Not sure
> if that's perfect, but seems cleaner.

Thanks for the patch. Out of curiosity I tried to play with it a bit.
`pgbench -i -s 100` actually hang on my machine, because the
copy process ended up with waiting after `pg_uds_send_with_fd`
had

errno == EWOULDBLOCK || errno == EAGAIN

as well as the checkpointer process. Looks like with the default
configuration and `max_wal_size=1GB` it writes more than reads to a
socket, and a buffer eventually becomes full. I've increased
SO_RCVBUF/SO_SNDBUF and `max_wal_size` independently to
check it, and in both cases the problem disappeared (but I assume
only for this particular scale). Is it something that was already considered?

Re: COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[Tom Lane]

David Rowley  writes:
> while it might not look too scary by itself, it gets a bit more so
> when you learn that the cur_lineno is only 32 bits wide. This will
> result in skipping a tuple every time the 32-bit variable wraps back
> around to 0 again.

Hm, so why is the correct rowcount returned --- are we running
a separate counter for that purpose, and if so why?

regards, tom lane

Re: ppc64le support in 9.3 branch?

[Tom Lane]

Christoph Berg  writes:
> Re: Tom Lane 2018-03-23 <1219.1521831...@sss.pgh.pa.us>
>> The compromise I'm inclined to offer is to see what happens if we
>> back-patch 9.4's config.guess and config.sub.  If that makes these
>> animals go green, and doesn't break any others, we'll call it good.
>> Otherwise, we revert that change and say we're not putting any
>> additional effort into it.

> Fwiw, apt.postgresql.org has been building+regression testing 9.3 on
> ppc64el on various Debian+Ubuntu releases for more than a year now.
> The only requirement is updating config.guess/sub.

FWIW, I'm still in favor of that back-patch.  But I was outvoted
before, and I dunno if this new info will change anyone's mind.

regards, tom lane

Re: Postgres, fsync, and OSs (specifically linux)

[Andres Freund]

Hi,


On 2018-05-22 17:37:28 +0200, Dmitry Dolgov wrote:
> Thanks for the patch. Out of curiosity I tried to play with it a bit.

Thanks.


> `pgbench -i -s 100` actually hang on my machine, because the
> copy process ended up with waiting after `pg_uds_send_with_fd`
> had

Hm, that had worked at some point...


> errno == EWOULDBLOCK || errno == EAGAIN
> 
> as well as the checkpointer process.

What do you mean with that latest sentence?


> Looks like with the default
> configuration and `max_wal_size=1GB` it writes more than reads to a
> socket, and a buffer eventually becomes full.

That's intended to then wake up the checkpointer immediately, so it can
absorb the requests.  So something isn't right yet.


> I've increased SO_RCVBUF/SO_SNDBUF and `max_wal_size` independently to
> check it, and in both cases the problem disappeared (but I assume only
> for this particular scale). Is it something that was already
> considered?

It's considered. Tuning up those might help with performance, but
shouldn't required from a correctness POV.  Hm.

Greetings,

Andres Freund

pgAdmin4 Docker behind load balancer

[Lenain]

Hello hackers,

We are currently using the dpage/pgadmin4 image to run a pgAdmin4 web
interface behind an AWS application load balancer.
The load balancer is configured to check the health of containers by
querying the /login URI and checking if it answers with a 200 HTTP code.

However the app always send a new cookie for this page, storing it into the
mounted docker volume.
It is understandable that it is wanted to generate a new session on login,
but as load balancers check numerous times a day this URI, it quickly fill
and use all of the inodes of the volume as it generate session tokens, and
consequently saturate also the inodes of the underlying system.

We are therefore looking for another URI to do our healthcheck that won't
generate a new session item.
However it seems that even on statics assets or redirects, the app set the
pga4_session cookie.

Is there another way available to do these checks ? Am I missing something ?

Thanks for your advices,
Kind regards.
/Lenain

Re: COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[Andres Freund]

On 2018-05-22 11:55:26 -0400, Tom Lane wrote:
> David Rowley  writes:
> > while it might not look too scary by itself, it gets a bit more so
> > when you learn that the cur_lineno is only 32 bits wide. This will
> > result in skipping a tuple every time the 32-bit variable wraps back
> > around to 0 again.
> 
> Hm, so why is the correct rowcount returned --- are we running
> a separate counter for that purpose, and if so why?

Yes, it's a local counter in CopyFrom/CopyTo.  It's probably not
entirely trivial to unify the two. The batching etc makes us modify
cur_lineno in a bit weird ways at times.  It's noteworthy that the
comment for cur_lineno says: /* line number for error messages */

Greetings,

Andres Freund

Re: plperl fails with perl 5.28

[Andrew Dunstan]

On 05/22/2018 07:18 AM, Christoph Berg wrote:

plperl fails to install with perl 5.27.11, which is to be released as 5.28.0:

2018-05-17 20:01:44.556 UTC [22629] pg_regress ERROR:
2018-05-17 20:01:44.556 UTC [22629] pg_regress CONTEXT:  while running Perl 
initialization
2018-05-17 20:01:44.556 UTC [22629] pg_regress STATEMENT:  CREATE EXTENSION IF NOT EXISTS 
"plperl"

Unfortunately this is all what I could extract from the server log,
even log_min_messages = debug5 doesn't print more.

Origin: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=899209




This is repeatable on Ubuntu.

I have a tiny bit more info:

   andrew=# load 'plperl.so';
   ERROR:
   CONTEXT:  while running Perl initialization
   andrew=#

That means it's failing at line 860 of plperl.c.

Maybe we need to fire up the perl debugger ...


cheers

andrew

--

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

Re: COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[Tom Lane]

Andres Freund  writes:
> On 2018-05-22 11:55:26 -0400, Tom Lane wrote:
>> Hm, so why is the correct rowcount returned --- are we running
>> a separate counter for that purpose, and if so why?

> Yes, it's a local counter in CopyFrom/CopyTo.  It's probably not
> entirely trivial to unify the two. The batching etc makes us modify
> cur_lineno in a bit weird ways at times.

OK, we'll just do it like David suggests then.  I haven't checked the
patch in detail yet, but it seemed generally sane if we're just going
to widen the duplicate counter.

regards, tom lane

Re: Postgres, fsync, and OSs (specifically linux)

[Andres Freund]

On 2018-05-22 08:57:18 -0700, Andres Freund wrote:
> Hi,
> 
> 
> On 2018-05-22 17:37:28 +0200, Dmitry Dolgov wrote:
> > Thanks for the patch. Out of curiosity I tried to play with it a bit.
> 
> Thanks.
> 
> 
> > `pgbench -i -s 100` actually hang on my machine, because the
> > copy process ended up with waiting after `pg_uds_send_with_fd`
> > had
> 
> Hm, that had worked at some point...
> 
> 
> > errno == EWOULDBLOCK || errno == EAGAIN
> > 
> > as well as the checkpointer process.
> 
> What do you mean with that latest sentence?
> 
> 
> > Looks like with the default
> > configuration and `max_wal_size=1GB` it writes more than reads to a
> > socket, and a buffer eventually becomes full.
> 
> That's intended to then wake up the checkpointer immediately, so it can
> absorb the requests.  So something isn't right yet.

Doesn't hang here, but it's way too slow. Reason for that is that I've
wrongly resolved a merge conflict. Attached is a fixup patch - does that
address the issue for you?

Greetings,

Andres Freund
>From 3b98662adf5e0f82375b50833bc618403614a461 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Fri, 6 Apr 2018 16:17:01 -0700
Subject: [PATCH 1/2] Fix and improve pg_atomic_flag fallback implementation.

The atomics fallback implementation for pg_atomic_flag was broken,
returning the inverted value from pg_atomic_test_set_flag().  This was
unnoticed because a) atomic flags were unused until recently b) the
test code wasn't run when the fallback implementation was in
use (because it didn't allow to test for some edge cases).

Fix the bug, and improve the fallback so it has the same behaviour as
the non-fallback implementation in the problematic edge cases. That
breaks ABI compatibility in the back branches when fallbacks are in
use, but given they were broken until now...

Author: Andres Freund
Reported-by: Daniel Gustafsson
Discussion: https://postgr.es/m/fb948276-7b32-4b77-83e6-d00167f8e...@yesql.se
Backpatch: 9.5-, where the atomics abstraction was introduced.
---
 src/backend/port/atomics.c  | 21 +++--
 src/include/port/atomics/fallback.h | 13 ++---
 src/test/regress/regress.c  | 14 --
 3 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/src/backend/port/atomics.c b/src/backend/port/atomics.c
index e4e4734dd23..caa84bf2b62 100644
--- a/src/backend/port/atomics.c
+++ b/src/backend/port/atomics.c
@@ -68,18 +68,35 @@ pg_atomic_init_flag_impl(volatile pg_atomic_flag *ptr)
 #else
 	SpinLockInit((slock_t *) &ptr->sema);
 #endif
+
+	ptr->value = false;
 }
 
 bool
 pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr)
 {
-	return TAS((slock_t *) &ptr->sema);
+	uint32		oldval;
+
+	SpinLockAcquire((slock_t *) &ptr->sema);
+	oldval = ptr->value;
+	ptr->value = true;
+	SpinLockRelease((slock_t *) &ptr->sema);
+
+	return oldval == 0;
 }
 
 void
 pg_atomic_clear_flag_impl(volatile pg_atomic_flag *ptr)
 {
-	S_UNLOCK((slock_t *) &ptr->sema);
+	SpinLockAcquire((slock_t *) &ptr->sema);
+	ptr->value = false;
+	SpinLockRelease((slock_t *) &ptr->sema);
+}
+
+bool
+pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
+{
+	return ptr->value == 0;
 }
 
 #endif			/* PG_HAVE_ATOMIC_FLAG_SIMULATION */
diff --git a/src/include/port/atomics/fallback.h b/src/include/port/atomics/fallback.h
index 7b9dcad8073..88a967ad5b9 100644
--- a/src/include/port/atomics/fallback.h
+++ b/src/include/port/atomics/fallback.h
@@ -80,6 +80,7 @@ typedef struct pg_atomic_flag
 #else
 	int			sema;
 #endif
+	volatile bool value;
 } pg_atomic_flag;
 
 #endif /* PG_HAVE_ATOMIC_FLAG_SUPPORT */
@@ -132,17 +133,7 @@ extern bool pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr);
 extern void pg_atomic_clear_flag_impl(volatile pg_atomic_flag *ptr);
 
 #define PG_HAVE_ATOMIC_UNLOCKED_TEST_FLAG
-static inline bool
-pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
-{
-	/*
-	 * Can't do this efficiently in the semaphore based implementation - we'd
-	 * have to try to acquire the semaphore - so always return true. That's
-	 * correct, because this is only an unlocked test anyway. Do this in the
-	 * header so compilers can optimize the test away.
-	 */
-	return true;
-}
+extern bool pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr);
 
 #endif /* PG_HAVE_ATOMIC_FLAG_SIMULATION */
 
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index e14322c798a..8bc562ee4f0 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -633,7 +633,6 @@ wait_pid(PG_FUNCTION_ARGS)
 	PG_RETURN_VOID();
 }
 
-#ifndef PG_HAVE_ATOMIC_FLAG_SIMULATION
 static void
 test_atomic_flag(void)
 {
@@ -663,7 +662,6 @@ test_atomic_flag(void)
 
 	pg_atomic_clear_flag(&flag);
 }
-#endif			/* PG_HAVE_ATOMIC_FLAG_SIMULATION */
 
 static void
 test_atomic_uint32(void)
@@ -846,19 +844,7 @@ PG_FUNCTION_INFO_V1(test_atomic_ops);
 Datum
 test_atomic_ops(PG_FUNCTION_ARGS)
 {
-	/* ---
-	 * Can't run the test under the semaphore emulation,

Re: Commit fest 2017-11

[Robert Haas]

On Thu, Mar 29, 2018 at 4:19 AM, Magnus Hagander  wrote:
> I wonder if we should consider adding a field to the CF app *specifically*
> to track things like this. What I'm thinking is a field that's set (or at
> least verified) by the person who flags a patch as committed with choices
> like Trivial/Simple/Medium/Complex (trivial being things like typo fixes
> etc, which today can hugely skew the stats).

I think this would be pretty subjective ... and there are also a LOT
of patches that don't go through the CF process.  The ratio of commits
to commitfest entries is at least 5:1.  If we only track what gets
registered in the CF app we're ignoring a huge amount of stuff.

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

Re: Commit fest 2017-11

[Magnus Hagander]

On Tue, May 22, 2018 at 7:57 PM, Robert Haas  wrote:

> On Thu, Mar 29, 2018 at 4:19 AM, Magnus Hagander 
> wrote:
> > I wonder if we should consider adding a field to the CF app
> *specifically*
> > to track things like this. What I'm thinking is a field that's set (or at
> > least verified) by the person who flags a patch as committed with choices
> > like Trivial/Simple/Medium/Complex (trivial being things like typo fixes
> > etc, which today can hugely skew the stats).
>
> I think this would be pretty subjective ... and there are also a LOT
> of patches that don't go through the CF process.  The ratio of commits
> to commitfest entries is at least 5:1.  If we only track what gets
> registered in the CF app we're ignoring a huge amount of stuff.
>

Definitely. It's easier to add structured data there than in the commit
message though -- but we could also define a standard to add it to the
commit messages. Or some inbetween. But whichever way we do it, it's likely
going to lead to a non-trivial amount of work to maintain. So the big
question is, is the data we can get out of it worth it?


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

Re: Commit fest 2017-11

[Andres Freund]

Hi,

On 2018-05-22 19:59:29 +0200, Magnus Hagander wrote:
> So the big question is, is the data we can get out of it worth it?

I can't see it being worth it personally.

Greetings,

Andres Freund

Re: Commit fest 2017-11

[Jesper Pedersen]

Hi,

On 05/22/2018 01:57 PM, Robert Haas wrote:

On Thu, Mar 29, 2018 at 4:19 AM, Magnus Hagander  wrote:

I wonder if we should consider adding a field to the CF app *specifically*
to track things like this. What I'm thinking is a field that's set (or at
least verified) by the person who flags a patch as committed with choices
like Trivial/Simple/Medium/Complex (trivial being things like typo fixes
etc, which today can hugely skew the stats).


I think this would be pretty subjective ... and there are also a LOT
of patches that don't go through the CF process.  The ratio of commits
to commitfest entries is at least 5:1.  If we only track what gets
registered in the CF app we're ignoring a huge amount of stuff.



If it was set during the submission it could provide an insight to 
reviewers to the level of difficulty. Maybe that could help reviewers 
pick the "correct" entry.


Of course there would need to be a link to the Wiki with a description 
of each of the levels.


The committer could of course always change the level upon Rejected, 
Returned with Feedback or Committed.


Best regards,
 Jesper

Re: A Japanese-unfriendy error message contruction

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> The "policy %s" and " %s" are transposed in Japaese
> syntax.  Specifically " %s NO  %s" is the
> natural translation of "policy %s on  %s". But currently
> we cannot get the natural error message in Japanese.

> For the reason, I'd like to propose to refactor
> getObjectDescription:OPCLASS_POLICY as the attached patch. The
> same structure is seen for OPCLASS_AMOP.

Taking a quick look through objectaddress.c, I see quite a lot of
deficiencies:

* Is it OK for the OCLASS_CLASS-with-subId case to assume that it can
tack on "column COLNAME" after the description of the relation containing
the column?  Perhaps this is tolerable but I'm not sure.  In English it'd
be at least as plausible to write "column COLNAME of ", and
maybe there are other languages where there's no good way to deal with
the current message structure.

* Column default values are written as "default for %s" where %s is what
you get from the above.  This seems likely to be even more awkward; do we
need to flatten that into one translatable string instead of recursing?
(At the very least I wish it was "default value for %s".)

* Collations have namespaces, but the OCLASS_COLLATION code doesn't
account for that.  Likewise for conversions, extended statistics
objects, and all four types of text search objects.

* OCLASS_PUBLICATION_REL likewise neglects schema-qualification of the
relation name.  I wonder why it's not using getRelationDescription, too.

* Both OCLASS_AMOP and OCLASS_AMPROC have the problem that they plug
the translation of "operator family %s for access method %s" into
" of %s".  I'm not sure how big a problem this is, or whether
it's worth the code-duplication needed to expose the whole thing as
one translatable message.  Opfamily members are pretty advanced things,
so maybe we shouldn't expend too much work on them.  (But if we do,
we should also take a look at the no-such-member error strings in
get_object_address_opf_member, which do the same thing.)

* The DEFACL code appends " in schema %s" after an otherwise
translatable message.  Do we need to fix that?

* OCLASS_RULE and OCLASS_TRIGGER use the same untranslatable style
as you complain of for OCLASS_POLICY.

The missing-schema-qualification issues seem like must-fix problems to me.
But not being a translator, I'm not sure how much of a problem each of the
other issues is.  I think that there was a deliberate decision at some
point that we'd accept some translation awkwardness in this code.  How
far do we want to go to clean it up?

regards, tom lane

future of contrib/xml2 and xslt processing

[Peter Eisentraut]

contrib/xml2 has been claimed to be deprecated since PostgreSQL 8.3.  I
took a look through the functions it offers and perhaps with xmltable
now being available, they could all be replaced using built-in
functions, perhaps with some small tweaking.

But we don't have any replacement lined up for the XSLT processing
function xslt_process.  What should we do with that, assuming that we
eventually want to remove contrib/xml2 altogether?

1. Just remove it, leaving users to find other solutions.  (PL/P* can
probably fill the gap.)

2. Create a new extension contrib/xslt, move the implementation there.
(Optionally, have contrib/xml2 depend on this new extension if it is not
ready to be removed.)

3. Add XSLT functionality to core (unlikely).

Thoughts?

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

Re: future of contrib/xml2 and xslt processing

[Tom Lane]

Peter Eisentraut  writes:
> contrib/xml2 has been claimed to be deprecated since PostgreSQL 8.3.  I
> took a look through the functions it offers and perhaps with xmltable
> now being available, they could all be replaced using built-in
> functions, perhaps with some small tweaking.

> But we don't have any replacement lined up for the XSLT processing
> function xslt_process.  What should we do with that, assuming that we
> eventually want to remove contrib/xml2 altogether?

> 1. Just remove it, leaving users to find other solutions.  (PL/P* can
> probably fill the gap.)

> 2. Create a new extension contrib/xslt, move the implementation there.
> (Optionally, have contrib/xml2 depend on this new extension if it is not
> ready to be removed.)

> 3. Add XSLT functionality to core (unlikely).

Option 2 seems like useless churn; we'd still have the same
not-very-high-quality code, just moved around.

I agree that moving the libxslt dependency into core isn't real
attractive either.

I suspect that the realistic alternatives are either option 1
or option 0: do nothing.

regards, tom lane

Re: future of contrib/xml2 and xslt processing

[Andres Freund]

Hi,

On 2018-05-22 14:38:32 -0400, Peter Eisentraut wrote:
> 1. Just remove it, leaving users to find other solutions.  (PL/P* can
> probably fill the gap.)

I don't have access to the code anymore, but a good number of past
customers and employers of mine relied on xslt.  I think we'd get some
pushback for this.

Greetings,

Andres Freund

Re: [PATCH] (Windows) psql echoes password when reading from pipe

[Tom Lane]

Matthew Stickney  writes:
> On windows, if you pipe data to psql, the password prompt correctly 
> reads from and writes to the console, but the password text is echoed to 
> the console. This is because echoing is disabled on the handle for 
> stdin, but as part of a pipeline stdin doesn't refer to the console. 
> I've attached a patch that gets a handle to the console's input buffer 
> by opening CONIN$ instead, which corrects the problem.

Thanks for the report and patch!

I know zip about Windows coding, but I can't help comparing this:

-   t = GetStdHandle(STD_INPUT_HANDLE);
+   t = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, 
FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL);

to the code a little bit above:

termin = fopen("CONIN$", "r");

Is it possible to operate on "termin" instead of doing a second open
(which might fail, which we are failing to cope with :-()?

regards, tom lane

Re: Postgres, fsync, and OSs (specifically linux)

[Dmitry Dolgov]

> On 22 May 2018 at 18:47, Andres Freund  wrote:
> On 2018-05-22 08:57:18 -0700, Andres Freund wrote:
>> Hi,
>>
>>
>> On 2018-05-22 17:37:28 +0200, Dmitry Dolgov wrote:
>> > Thanks for the patch. Out of curiosity I tried to play with it a bit.
>>
>> Thanks.
>>
>>
>> > `pgbench -i -s 100` actually hang on my machine, because the
>> > copy process ended up with waiting after `pg_uds_send_with_fd`
>> > had
>>
>> Hm, that had worked at some point...
>>
>>
>> > errno == EWOULDBLOCK || errno == EAGAIN
>> >
>> > as well as the checkpointer process.
>>
>> What do you mean with that latest sentence?

To investigate what's happening I attached with gdb to two processes, COPY
process from pgbench and checkpointer (since I assumed it may be involved).
Both were waiting in WaitLatchOrSocket right after SendFsyncRequest.

>> > Looks like with the default
>> > configuration and `max_wal_size=1GB` it writes more than reads to a
>> > socket, and a buffer eventually becomes full.
>>
>> That's intended to then wake up the checkpointer immediately, so it can
>> absorb the requests.  So something isn't right yet.
>
> Doesn't hang here, but it's way too slow.

Yep, in my case it was also getting slower, but eventually hang.

> Reason for that is that I've wrongly resolved a merge conflict. Attached is a
> fixup patch - does that address the issue for you?

Hm...is it a correct patch? I see the same committed in
8c3debbbf61892dabd8b6f3f8d55e600a7901f2b, so I can't really apply it.

Re: Postgres, fsync, and OSs (specifically linux)

[Andres Freund]

On 2018-05-22 20:54:46 +0200, Dmitry Dolgov wrote:
> > On 22 May 2018 at 18:47, Andres Freund  wrote:
> > On 2018-05-22 08:57:18 -0700, Andres Freund wrote:
> >> Hi,
> >>
> >>
> >> On 2018-05-22 17:37:28 +0200, Dmitry Dolgov wrote:
> >> > Thanks for the patch. Out of curiosity I tried to play with it a bit.
> >>
> >> Thanks.
> >>
> >>
> >> > `pgbench -i -s 100` actually hang on my machine, because the
> >> > copy process ended up with waiting after `pg_uds_send_with_fd`
> >> > had
> >>
> >> Hm, that had worked at some point...
> >>
> >>
> >> > errno == EWOULDBLOCK || errno == EAGAIN
> >> >
> >> > as well as the checkpointer process.
> >>
> >> What do you mean with that latest sentence?
> 
> To investigate what's happening I attached with gdb to two processes, COPY
> process from pgbench and checkpointer (since I assumed it may be involved).
> Both were waiting in WaitLatchOrSocket right after SendFsyncRequest.

Huh? Checkpointer was in SendFsyncRequest()? Coudl you share the
backtrace?


> >> > Looks like with the default
> >> > configuration and `max_wal_size=1GB` it writes more than reads to a
> >> > socket, and a buffer eventually becomes full.
> >>
> >> That's intended to then wake up the checkpointer immediately, so it can
> >> absorb the requests.  So something isn't right yet.
> >
> > Doesn't hang here, but it's way too slow.
> 
> Yep, in my case it was also getting slower, but eventually hang.
> 
> > Reason for that is that I've wrongly resolved a merge conflict. Attached is 
> > a
> > fixup patch - does that address the issue for you?
> 
> Hm...is it a correct patch? I see the same committed in
> 8c3debbbf61892dabd8b6f3f8d55e600a7901f2b, so I can't really apply it.

Yea, sorry for that. Too many files in my patch directory... Right one
attached.

Greetings,

Andres Freund
>From 483b98fd21b40e2997a1f164155cae698204ec25 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 22 May 2018 09:38:58 -0700
Subject: [PATCH] fixup! WIP: Optimize register_dirty_segment() to not
 repeatedly queue fsync requests.

Merge failure.
---
 src/backend/storage/smgr/md.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index ae3a5bf023f..942e2dcf788 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1540,6 +1540,8 @@ register_dirty_segment(SMgrRelation reln, ForkNumber forknum, MdfdVec *seg)
 	}
 	else
 		ForwardFsyncRequest(reln->smgr_rnode.node, forknum, seg->mdfd_segno, seg->mdfd_vfd);
+
+	seg->mdfd_dirtied_cycle = cycle;
 }
 
 /*
-- 
2.17.0.rc1.dirty

Re: perl checking

[Andrew Dunstan]

On 05/22/2018 10:09 AM, Tom Lane wrote:

Andrew Dunstan  writes:

On 05/22/2018 04:11 AM, Kyotaro HORIGUCHI wrote:

At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan  
wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800...@2ndquadrant.com>

One patch silences a warning from convutils.pl about the unportability
of the literal 0x1. We've run for many years without this
giving us a problem, so I think we can turn the warning off pretty
safely.

It was introduced by aeed17d000 (in 2017). The history of the
file is rather short. Over 32-bit values do not apperar as a
character so there's no problem in ignoring the warning for now,
but can't we use bigint to silence it instead?

It would impose an additional dependency. bigint isn't installed by
default on many systems AFAICT, so I think we'd need a better reason
than this to require it.

I agree with not adding a dependency (although FWIW, bigint does seem
to be there in my minimal perl setups).  But can't we fix it like this:

-   elsif ($in < 0x1)
+   elsif ($in <= 0x)

At least in a quick test here, "-cw" doesn't moan about 0x.

For consistency, the other arms of the "if" should be adjusted
similarly.





Yeah. I tested this on the oldest 32 but perls I could find, the msys 
and activestate perls on the XP machine that runs frogmouth and friends. 
Even though they both have an ivsize of 4, the arithmetic seems to work 
properly. Perhaps they store larger numbers as doubles, which you should 
be able to do exactly up to about 52 bit integers. The other 32 bit 
machine I have is an Ubuntu 16.04 VM, but there the perl has an ivsize 
of 8, so of course it does the right thing.


We don't normally use these scripts anyway, so I'll go with this 
suggestion without further investigation.


cheers

andrew

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

Re: pgAdmin4 Docker behind load balancer

[Daniel Gustafsson]

> On 22 May 2018, at 18:07, Lenain  wrote:
> 
> Hello hackers,
> 
> We are currently using the dpage/pgadmin4 image to run a pgAdmin4 web 
> interface behind an AWS application load balancer.
> The load balancer is configured to check the health of containers by querying 
> the /login URI and checking if it answers with a 200 HTTP code.
> 
> However the app always send a new cookie for this page, storing it into the 
> mounted docker volume.
> It is understandable that it is wanted to generate a new session on login, 
> but as load balancers check numerous times a day this URI, it quickly fill 
> and use all of the inodes of the volume as it generate session tokens, and 
> consequently saturate also the inodes of the underlying system.
> 
> We are therefore looking for another URI to do our healthcheck that won't 
> generate a new session item.
> However it seems that even on statics assets or redirects, the app set the 
> pga4_session cookie.
> 
> Is there another way available to do these checks ? Am I missing something ?

This is the mailinglist for the core postgres database server.  While there
certainly are lots of people skilled in pgadmin here, you will probably have a
better chance of getting help on the pgadmin-support mailinglist:

https://www.postgresql.org/list/pgadmin-support/

cheers ./daniel

Re: [PATCH] (Windows) psql echoes password when reading from pipe

[Matthew Stickney]

It is possible, at the cost of two extra function calls, which could
theoretically fail (and require a little extra munging to work on
Windows CE -- is that a target platform for postgres?).

Similar to using CreateFile, I think the cases in which those calls
could fail are so extraordinary that psql probably wouldn't run at all
(i.e. there's no console for the process, in which case psql would
crash as soon as it attempted to do IO). It also requires opening
termin with "w+", because SetConsoleMode needs write permissions. You
could avoid that by doing a DuplicateHandle on the underlying handle
that you retrieve from termin, but I doubt it's worth it.

I'll do up a modified patch tonight, if that sounds like it'd be better.

-Matt Stickney

On Tue, May 22, 2018 at 2:55 PM, Tom Lane  wrote:
> Matthew Stickney  writes:
>> On windows, if you pipe data to psql, the password prompt correctly
>> reads from and writes to the console, but the password text is echoed to
>> the console. This is because echoing is disabled on the handle for
>> stdin, but as part of a pipeline stdin doesn't refer to the console.
>> I've attached a patch that gets a handle to the console's input buffer
>> by opening CONIN$ instead, which corrects the problem.
>
> Thanks for the report and patch!
>
> I know zip about Windows coding, but I can't help comparing this:
>
> -   t = GetStdHandle(STD_INPUT_HANDLE);
> +   t = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, 
> FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL);
>
> to the code a little bit above:
>
> termin = fopen("CONIN$", "r");
>
> Is it possible to operate on "termin" instead of doing a second open
> (which might fail, which we are failing to cope with :-()?
>
> regards, tom lane

Re: Postgres, fsync, and OSs (specifically linux)

[Dmitry Dolgov]

> On 22 May 2018 at 20:59, Andres Freund  wrote:
> On 2018-05-22 20:54:46 +0200, Dmitry Dolgov wrote:
>> > On 22 May 2018 at 18:47, Andres Freund  wrote:
>> > On 2018-05-22 08:57:18 -0700, Andres Freund wrote:
>> >> Hi,
>> >>
>> >>
>> >> On 2018-05-22 17:37:28 +0200, Dmitry Dolgov wrote:
>> >> > Thanks for the patch. Out of curiosity I tried to play with it a bit.
>> >>
>> >> Thanks.
>> >>
>> >>
>> >> > `pgbench -i -s 100` actually hang on my machine, because the
>> >> > copy process ended up with waiting after `pg_uds_send_with_fd`
>> >> > had
>> >>
>> >> Hm, that had worked at some point...
>> >>
>> >>
>> >> > errno == EWOULDBLOCK || errno == EAGAIN
>> >> >
>> >> > as well as the checkpointer process.
>> >>
>> >> What do you mean with that latest sentence?
>>
>> To investigate what's happening I attached with gdb to two processes, COPY
>> process from pgbench and checkpointer (since I assumed it may be involved).
>> Both were waiting in WaitLatchOrSocket right after SendFsyncRequest.
>
> Huh? Checkpointer was in SendFsyncRequest()? Coudl you share the
> backtrace?

Well, that's what I've got from gdb:

#0  0x7fae03fae9f3 in __epoll_wait_nocancel () at
../sysdeps/unix/syscall-template.S:84
#1  0x0077a979 in WaitEventSetWaitBlock (nevents=1,
occurred_events=0x7ffe37529ec0, cur_timeout=-1, set=0x23cddf8) at
latch.c:1048
#2  WaitEventSetWait (set=set@entry=0x23cddf8,
timeout=timeout@entry=-1,
occurred_events=occurred_events@entry=0x7ffe37529ec0,
nevents=nevents@entry=1, wait_event_info=wait_event_info@entry=0) at
latch.c:1000
#3  0x0077ad08 in WaitLatchOrSocket
(latch=latch@entry=0x0, wakeEvents=wakeEvents@entry=4, sock=8,
timeout=timeout@entry=-1, wait_event_info=wait_event_info@entry=0) at
latch.c:385
#4  0x007152cb in SendFsyncRequest
(request=request@entry=0x7ffe37529f40, fd=fd@entry=-1) at
checkpointer.c:1345
#5  0x00716223 in AbsorbAllFsyncRequests () at checkpointer.c:1207
#6  0x0079a5f0 in mdsync () at md.c:1339
#7  0x0079c672 in smgrsync () at smgr.c:766
#8  0x0076dd53 in CheckPointBuffers (flags=flags@entry=64)
at bufmgr.c:2581
#9  0x0051c681 in CheckPointGuts
(checkPointRedo=722254352, flags=flags@entry=64) at xlog.c:9079
#10 0x00523c4a in CreateCheckPoint (flags=flags@entry=64)
at xlog.c:8863
#11 0x00715f41 in CheckpointerMain () at checkpointer.c:494
#12 0x005329f4 in AuxiliaryProcessMain (argc=argc@entry=2,
argv=argv@entry=0x7ffe3752a220) at bootstrap.c:451
#13 0x00720c28 in StartChildProcess
(type=type@entry=CheckpointerProcess) at postmaster.c:5340
#14 0x00721c23 in reaper (postgres_signal_arg=) at postmaster.c:2875
#15 
#16 0x7fae03fa45b3 in __select_nocancel () at
../sysdeps/unix/syscall-template.S:84
#17 0x00722968 in ServerLoop () at postmaster.c:1679
#18 0x00723cde in PostmasterMain (argc=argc@entry=3,
argv=argv@entry=0x23a00e0) at postmaster.c:1388
#19 0x0068979f in main (argc=3, argv=0x23a00e0) at main.c:228

>> >> > Looks like with the default
>> >> > configuration and `max_wal_size=1GB` it writes more than reads to a
>> >> > socket, and a buffer eventually becomes full.
>> >>
>> >> That's intended to then wake up the checkpointer immediately, so it can
>> >> absorb the requests.  So something isn't right yet.
>> >
>> > Doesn't hang here, but it's way too slow.
>>
>> Yep, in my case it was also getting slower, but eventually hang.
>>
>> > Reason for that is that I've wrongly resolved a merge conflict. Attached 
>> > is a
>> > fixup patch - does that address the issue for you?
>>
>> Hm...is it a correct patch? I see the same committed in
>> 8c3debbbf61892dabd8b6f3f8d55e600a7901f2b, so I can't really apply it.
>
> Yea, sorry for that. Too many files in my patch directory... Right one
> attached.

Yes, this patch solves the problem, thanks.

Re: Commit fest 2017-11

[Robert Haas]

On Tue, May 22, 2018 at 2:02 PM, Andres Freund  wrote:
> On 2018-05-22 19:59:29 +0200, Magnus Hagander wrote:
>> So the big question is, is the data we can get out of it worth it?
>
> I can't see it being worth it personally.

I tend to agree.  First, it sounds like a lot of work.  If we had the
opportunity to improve our process in a way that would naturally
gather this data while providing various benefits to the project, I
could see doing it.  But if you ask any group of people to do extra
work just for reporting purposes, it tends to annoy people ... and
they often don't take the trouble to make it accurate, either.  Even
with best intentions, it's all pretty subjective.  Look at the
arguments we have about Bruce's count of release note items -- does a
change in the number represent a change in his standards, a change in
the number of items meeting his standards, an artifact of how he
groups things together, or a real effect?  And that's with all the
classification being done by one person.  With ~19 active committers,
it's bound to diverge even more.

I have found counting "number of lines added" with "git log -w -M
--stat" to be a decent barometer of contribution strength for a lot of
patches, but it greatly overstates the value of mechanical change.
I'm not sure there's any automated way to take that into account, but
it would be nice if there were.  Anyhow, I'm inclined to view
automatically generated metrics, even if imperfect, as a better option
than manual classification.

Also, while it's useful to know how much is getting committed, I'm not
sure how much it matters in the end.  It should be clear to everyone
at this point that there is insufficient committer bandwidth to deal
with all of the good patches that get sent in.  I suggest we discuss
at PGCon what we want to do about that problem.

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

Re: Postgres, fsync, and OSs (specifically linux)

[Andres Freund]

On 2018-05-22 21:58:06 +0200, Dmitry Dolgov wrote:
> > On 22 May 2018 at 20:59, Andres Freund  wrote:
> > On 2018-05-22 20:54:46 +0200, Dmitry Dolgov wrote:
> > Huh? Checkpointer was in SendFsyncRequest()? Coudl you share the
> > backtrace?
> 
> Well, that's what I've got from gdb:

> #3  0x0077ad08 in WaitLatchOrSocket
> (latch=latch@entry=0x0, wakeEvents=wakeEvents@entry=4, sock=8,
> timeout=timeout@entry=-1, wait_event_info=wait_event_info@entry=0) at
> latch.c:385
> #4  0x007152cb in SendFsyncRequest
> (request=request@entry=0x7ffe37529f40, fd=fd@entry=-1) at
> checkpointer.c:1345
> #5  0x00716223 in AbsorbAllFsyncRequests () at checkpointer.c:1207

Oh, I see. That makes sense. So it's possible to self-deadlock
here. Should be easy to fix...  Thanks for finding that one.


> Yes, this patch solves the problem, thanks.

Just avoids it, I'm afraid... It probably can't be hit easily, but the
issue is there...

- Andres

Re: [PATCH] (Windows) psql echoes password when reading from pipe

[Tom Lane]

[ please don't top-post ]

Matthew Stickney  writes:
> On Tue, May 22, 2018 at 2:55 PM, Tom Lane  wrote:
>> Is it possible to operate on "termin" instead of doing a second open
>> (which might fail, which we are failing to cope with :-()?

> It is possible, at the cost of two extra function calls, which could
> theoretically fail (and require a little extra munging to work on
> Windows CE -- is that a target platform for postgres?).

AFAIK we don't claim to support CE.

> Similar to using CreateFile, I think the cases in which those calls
> could fail are so extraordinary that psql probably wouldn't run at all
> (i.e. there's no console for the process, in which case psql would
> crash as soon as it attempted to do IO). It also requires opening
> termin with "w+", because SetConsoleMode needs write permissions. You
> could avoid that by doing a DuplicateHandle on the underlying handle
> that you retrieve from termin, but I doubt it's worth it.

Hm.  The failure mode I was thinking about was insufficient resources
to allocate another handle; so it seems like all of these are more or
less equivalent on that basis, and we might as well go with whatever's
shortest.  But perhaps it's worth adding logic to deal with failure
of the call?  Or can we expect the additional calls to fall through
silently anyway if given an invalid handle?  (If so, maybe a comment
about that is sufficient.)

regards, tom lane

Re: [PATCH] (Windows) psql echoes password when reading from pipe

[Matthew Stickney]

On Tue, May 22, 2018 at 4:09 PM, Tom Lane  wrote:
> [ please don't top-post ]

Sorry, I'm used to using a better mail client than this.

> Hm.  The failure mode I was thinking about was insufficient resources
> to allocate another handle

You have a point here; CreateFile does create a new handle (as would
DuplicateHandle), while _fileno and _get_osfhandle just retrieve the
existing file descriptor and handle, respectively. I checked the docs
more carefully, and both calls should never fail unless the input
stream is invalid.

> But perhaps it's worth adding logic to deal with failure of the call?

I think it would be sufficient to check whether the SetConsoleMode
call fails, because that can fail even on a valid handle (e.g. if you
don't have a handle with write access). That would catch the case
where opening termin fails, too, although that might deserve it's own
check to get better error information.

simple_prompt seems to be used in a lot of different utilities for
different reasons; there seem to be a number of conventions for
reporting errors in src/port/ code, but it looks like other
interactive utilities generally print a message to stderr, and return
a basic success/failure value. Does that sound like the right
approach? I'm not sure if it's obvious how to handle errors in the
other utilities, but I can take a look.

-Matt Stickney

Re: COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[David Rowley]

Thanks for pushing.

On 23 May 2018 at 03:55, Tom Lane  wrote:
> Hm, so why is the correct rowcount returned --- are we running
> a separate counter for that purpose, and if so why?

I thought the output I pasted was clearly showing it not to be the
same. 42 vs 43.

Did I misunderstand you?

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

Re: COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[Vik Fearing]

On 22/05/18 23:04, David Rowley wrote:
> Thanks for pushing.
> 
> On 23 May 2018 at 03:55, Tom Lane  wrote:
>> Hm, so why is the correct rowcount returned --- are we running
>> a separate counter for that purpose, and if so why?
> 
> I thought the output I pasted was clearly showing it not to be the
> same. 42 vs 43.
> 
> Did I misunderstand you?

I think Tom was wondering why it isn't showing 5032703.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Re: Error on vacuum: xmin before relfrozenxid

[Andres Freund]

Hi,

On 2018-05-22 21:43:01 +0200, Paolo Crosato wrote:
> > Could you report the result of
> > select ctid, xmin, xmax from pg_authid ;
> >
> 
> This is the result:
> 
> postgres=# select ctid, xmin, xmax from pg_authid ;

>  (0,16) | 3031994631 |0
>  16 |   6496 |1 |144 | 3031994631 |  0 |0 | (0,16)
> |   32779 |  10507 | 32 | 1100 |  675851 |
> \x6e6167696f7300010100496d64356333633236616163636439616665346437383061396239613464663634653639

> postgres=# select relfrozenxid from pg_class where relname='pg_authid';
>  relfrozenxid
> --
> 400011439

That tuple looks, to me, like it indeed shouldn't exist, given the
relfrozenxid.  Decoding infomask (10507 / 0x290B), if I did it right,
shows:

HEAP_HASNULL
HEAP_HASVARWIDTH
HEAP_HASOID
HEAP_XMIN_COMMITTED
HEAP_XMAX_INVALID
HEAP_UPDATED

so it's not frozen.  That suggests there's either a bug, or you had
corruption in your cluster.


Could you give a bit of "history" about that postgres instance? What
version of postgres was it run on earliest? Was it ever pg_upgraded?
Were there any OS crashes? Other hardware issues?  Was the cluster ever
used with fsync=off or full_page_writes=off?

Greetings,

Andres Freund

Re: COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[David Rowley]

On 23 May 2018 at 09:16, Vik Fearing  wrote:
> I think Tom was wondering why it isn't showing 5032703.

You'll need to explain that one. The number just looks like nonsense to me.

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

Re: COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[Andres Freund]

On 2018-05-23 09:04:35 +1200, David Rowley wrote:
> Thanks for pushing.
> 
> On 23 May 2018 at 03:55, Tom Lane  wrote:
> > Hm, so why is the correct rowcount returned --- are we running
> > a separate counter for that purpose, and if so why?
> 
> I thought the output I pasted was clearly showing it not to be the
> same. 42 vs 43.
> 
> Did I misunderstand you?

Well, the row-returned counter is obviously wide enough, otherwise
42 couldn't be returned. Tom's point, as I understood it, is
that we obviously have one wide enough counter - why can't we reuse that
for the one you made wider.  And it doesn't seem entirely trivial to do
so, so your patch is easier.

Greetings,

Andres Freund

Re: COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[David Rowley]

On 23 May 2018 at 09:31, Andres Freund  wrote:
>> On 23 May 2018 at 03:55, Tom Lane  wrote:
>> > Hm, so why is the correct rowcount returned --- are we running
>> > a separate counter for that purpose, and if so why?
>>
>> I thought the output I pasted was clearly showing it not to be the
>> same. 42 vs 43.
>>
>> Did I misunderstand you?
>
> Well, the row-returned counter is obviously wide enough, otherwise
> 42 couldn't be returned. Tom's point, as I understood it, is
> that we obviously have one wide enough counter - why can't we reuse that
> for the one you made wider.  And it doesn't seem entirely trivial to do
> so, so your patch is easier.

*moment of realisation*

Oh, this makes sense now.

They can't be the same. One tracks the line number in the COPY FROM
input, the other tracks the number of rows inserted. You'd only have
to add a BEFORE INSERT ROW trigger which blocks some rows to
understand why they need to be separate.

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

Re: plperl fails with perl 5.28

[Tom Lane]

Andrew Dunstan  writes:
> On 05/22/2018 07:18 AM, Christoph Berg wrote:
>> plperl fails to install with perl 5.27.11, which is to be released as 5.28.0:

> I have a tiny bit more info:
> andrew=# load 'plperl.so';
> ERROR:
> CONTEXT:  while running Perl initialization
> andrew=#

I get the same behavior with a build of 5.27.11 on Fedora 28.

> That means it's failing at line 860 of plperl.c.

Tracing through it, the problem is that perl_run is returning 0x100,
rather than zero as we expect, even though there was no failure.
This happens because perl.c:S_run_body() falls off the end of the
initialization code and does "my_exit(0);".  Apparently it's done that for
a long time, but what's new is that perl_run() does this in response
after catching the longjmp from my_exit():

if (exit_called) {
ret = STATUS_EXIT;
if (ret == 0) ret = 0x100;
} else {
ret = 0;
}

That traces to this recent commit:

https://perl5.git.perl.org/perl.git/commitdiff/0301e899536a22752f40481d8a1d141b7a7dda82

which seems rather brain-dead from here, because it claims that it's
defining perl_run's result as a truth value, which it is not any more.

So assuming that this holds and the Perl guys don't see the error
of their ways, we'd need something like this, I think:

-   if (perl_run(plperl) != 0)
+   if ((perl_run(plperl) & 0xFF) != 0)

but TBH I think someone oughta file a bug report first.  They can't
seriously intend that callers must do that, especially when there does
not seem to be any big bold warning about it in perl*delta.pod.

(Also, since perl_parse and perl_run are now specified to have identical
return conventions, we'd probably want to change the perl_parse call
likewise, even though it's not failing today.)

(Alternatively, perhaps it's a bad idea that the plperl initialization
script falls off the end rather than explicitly returning?)

regards, tom lane

Re: Error on vacuum: xmin before relfrozenxid

[Paolo Crosato]

Good evening,

2018-05-22 23:19 GMT+02:00 Andres Freund :

> Hi,
>
> On 2018-05-22 21:43:01 +0200, Paolo Crosato wrote:
> > > Could you report the result of
> > > select ctid, xmin, xmax from pg_authid ;
> > >
> >
> > This is the result:
> >
> > postgres=# select ctid, xmin, xmax from pg_authid ;
>
> >  (0,16) | 3031994631 |0
> >  16 |   6496 |1 |144 | 3031994631 |  0 |0 |
> (0,16)
> > |   32779 |  10507 | 32 | 1100 |  675851 |
> > \x6e6167696f73000
> 
> 10100496d64356333633236616163636
> 439616665346437383061396239613464663634653639
>
> > postgres=# select relfrozenxid from pg_class where relname='pg_authid';
> >  relfrozenxid
> > --
> > 400011439
>
> That tuple looks, to me, like it indeed shouldn't exist, given the
> relfrozenxid.  Decoding infomask (10507 / 0x290B), if I did it right,
> shows:
>
> HEAP_HASNULL
> HEAP_HASVARWIDTH
> HEAP_HASOID
> HEAP_XMIN_COMMITTED
> HEAP_XMAX_INVALID
> HEAP_UPDATED
>
> so it's not frozen.  That suggests there's either a bug, or you had
> corruption in your cluster.
>
>
> Could you give a bit of "history" about that postgres instance? What
> version of postgres was it run on earliest? Was it ever pg_upgraded?
> Were there any OS crashes? Other hardware issues?  Was the cluster ever
> used with fsync=off or full_page_writes=off?
>
> Greetings,
>
> Andres Freund
>

The cluster is made of a primary master instance, and a secondary slave in
hot standby, with streaming replication. There is a barman server for the
backups.
The first version it ran on was 10.2, the cluster was promoted in
production in mid february. It was never pg_upgraded, we upgraded it
yesterday to 10.4 and restarted, in hope that this would resolve the issue.
The restart was performed by the upgrading process. There was never any
crash or hardware issue, the instance run without any problem. we never
enabled fsync=off or full_page_writes=off. This is the only real issue so
far. We have many nagios alerts monitoring the instance, the only problem
is we are creating many temporary files, most of them are caused by a query
that doesn't overflow work_mem when run alone with explain analyze buffers,
which is weird. I should mention that we had a major creation of temp files
in the first hours of the cluster history, however that was quickly
resolved.

I managed to recover the log of the first time we run into the issue, the
error was the same but on template1:

May  8 11:26:46 xxx postgres[32543]: [1154-1] user=,db=,client= ERROR:
found xmin 2600758304 from before relfrozenxid 400011439
May  8 11:26:46 xxx postgres[32543]: [1154-2] user=,db=,client= CONTEXT:
automatic vacuum of table "template1.pg_catalog.pg_authid"

Deleting the row worked, but, as you see, the problem with  400011439 was
there already. This happened when we added a user, shortly afterwards these
errors started popping up in the log. We found the issue because the
autovacuums stopped working, they were blocked on the pg_authid table. The
same time we found out the long running transaction, it was a multi session
kill that someway got stuck. Killing the transaction and deleting the
offending row was enough for the autovacuums to restart. We also had a
materialized view that was created months before and nobody used. Since i
found in the past there was a similar error related to materialized views,
I dropped it.

The instance is doing a lot of transactions every day, probably around 100
millions. I would need to do more checks to provide an accurate measure.

I still have the logs since the first time the error appeared, so I can
provide further details if needed.

Thanks,
Best Regards

Paolo Crosato

Re: COPY FROM WITH HEADER skips a tuple every 4 billion tuples

[Tom Lane]

Andres Freund  writes:
> On 2018-05-23 09:04:35 +1200, David Rowley wrote:
>> I thought the output I pasted was clearly showing it not to be the
>> same. 42 vs 43.

> Well, the row-returned counter is obviously wide enough, otherwise
> 42 couldn't be returned. Tom's point, as I understood it, is
> that we obviously have one wide enough counter - why can't we reuse that
> for the one you made wider.  And it doesn't seem entirely trivial to do
> so, so your patch is easier.

Right.  Obviously there was a 64-bit counter someplace, but it wasn't
being used for this purpose.

I think after looking at the code that the cur_lineno counter is
counting input *lines* whereas the other thing counts finished *rows*,
so unifying them would be a bad idea anyway.

regards, tom lane

Re: [PATCH] (Windows) psql echoes password when reading from pipe

[Tom Lane]

Matthew Stickney  writes:
> On Tue, May 22, 2018 at 4:09 PM, Tom Lane  wrote:
>> But perhaps it's worth adding logic to deal with failure of the call?

> I think it would be sufficient to check whether the SetConsoleMode
> call fails, because that can fail even on a valid handle (e.g. if you
> don't have a handle with write access). That would catch the case
> where opening termin fails, too, although that might deserve it's own
> check to get better error information.

> simple_prompt seems to be used in a lot of different utilities for
> different reasons; there seem to be a number of conventions for
> reporting errors in src/port/ code, but it looks like other
> interactive utilities generally print a message to stderr, and return
> a basic success/failure value. Does that sound like the right
> approach? I'm not sure if it's obvious how to handle errors in the
> other utilities, but I can take a look.

Well, the question that ought to be answered first is whether to do
anything at all, beyond not-crashing.  It doesn't seem to me that
refusing to accept a password if we can't disable echo is a net win,
so I'm inclined to think it's okay to silently ignore failure to
turn off echo.

The other aspect of this code that maybe needs consideration is the
silent fallback to use stdin/stderr if we can't open the console.
It seems like that could be a really bad idea if stdin is a pipe.
But on the other hand, maybe somebody is depending on it somewhere?
I'm not sure I'd want to back-patch a change in that behavior, anyway.

regards, tom lane

Re: plperl fails with perl 5.28

[Andrew Dunstan]

On 05/22/2018 06:02 PM, Tom Lane wrote:

Andrew Dunstan  writes:

On 05/22/2018 07:18 AM, Christoph Berg wrote:

plperl fails to install with perl 5.27.11, which is to be released as 5.28.0:

I have a tiny bit more info:
 andrew=# load 'plperl.so';
 ERROR:
 CONTEXT:  while running Perl initialization
 andrew=#

I get the same behavior with a build of 5.27.11 on Fedora 28.


That means it's failing at line 860 of plperl.c.

Tracing through it, the problem is that perl_run is returning 0x100,
rather than zero as we expect, even though there was no failure.
This happens because perl.c:S_run_body() falls off the end of the
initialization code and does "my_exit(0);".  Apparently it's done that for
a long time, but what's new is that perl_run() does this in response
after catching the longjmp from my_exit():

 if (exit_called) {
 ret = STATUS_EXIT;
 if (ret == 0) ret = 0x100;
 } else {
 ret = 0;
 }

That traces to this recent commit:

https://perl5.git.perl.org/perl.git/commitdiff/0301e899536a22752f40481d8a1d141b7a7dda82

which seems rather brain-dead from here, because it claims that it's
defining perl_run's result as a truth value, which it is not any more.

So assuming that this holds and the Perl guys don't see the error
of their ways, we'd need something like this, I think:

-   if (perl_run(plperl) != 0)
+   if ((perl_run(plperl) & 0xFF) != 0)

but TBH I think someone oughta file a bug report first.  They can't
seriously intend that callers must do that, especially when there does
not seem to be any big bold warning about it in perl*delta.pod.

(Also, since perl_parse and perl_run are now specified to have identical
return conventions, we'd probably want to change the perl_parse call
likewise, even though it's not failing today.)

(Alternatively, perhaps it's a bad idea that the plperl initialization
script falls off the end rather than explicitly returning?)





Well diagnosed. Maybe it's worth pointing out that almost all the 
examples of perl_run() in the perlembed manual simply ignore the 
returned value. OTOH, if that's what we're supposed to do why isn't the 
function declared that way?


cheers

andrew

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

Re: plperl fails with perl 5.28

[Tom Lane]

Andrew Dunstan  writes:
> Well diagnosed. Maybe it's worth pointing out that almost all the 
> examples of perl_run() in the perlembed manual simply ignore the 
> returned value. OTOH, if that's what we're supposed to do why isn't the 
> function declared that way?

Yeah, it seems that mainly what they're doing is trying to resolve
the confusion about that.

regards, tom lane

Re: Error on vacuum: xmin before relfrozenxid

[Andres Freund]

Hi,

On 2018-05-23 00:04:26 +0200, Paolo Crosato wrote:
> I managed to recover the log of the first time we run into the issue, the
> error was the same but on template1:
> 
> May  8 11:26:46 xxx postgres[32543]: [1154-1] user=,db=,client= ERROR:
> found xmin 2600758304 from before relfrozenxid 400011439
> May  8 11:26:46 xxx postgres[32543]: [1154-2] user=,db=,client= CONTEXT:
> automatic vacuum of table "template1.pg_catalog.pg_authid"

pg_authid (along with a few other tables) is shared between
databases. So that's just hte same error.  At which rate are you
creating / updating database users / roles?

Greetings,

Andres Freund

Re: Allowing printf("%m") only where it actually works

[Thomas Munro]

On Mon, May 21, 2018 at 2:41 PM, Thomas Munro
 wrote:
> I've raised this on the freebsd-hackers
> list, let's see... I bet there's other software out there that just
> prints out "m" when things go wrong.  It's arguably something that
> you'd want the complier to understand as a C dialect thing.

Result: FreeBSD printf() now supports %m, and an issue is being raised
with clang about lack of warnings on OSes that don't grok %m, and lack
of warnings when you say -Wformat-non-iso on any OS.  Let's see how
that goes.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: PostgreSQL and Homomorphic Encryption

[David Fetter]

On Tue, May 22, 2018 at 08:34:18AM +0200, Tal Glozman wrote:
> Hello PostgreSQL Team,
> 
> I'm doing a project at my university (HU Berlin) involving
> homomorphic encrypted searches on data bases. Does PostgreSQL
> support homomorphic encryption-based searches?

Yes, in the sense that PostgreSQL has Turing-complete languages for
expressional indexes, so to the extent that Turing machines can solve
the problem you want solved, the capability is there.

What would a system that supported homomorphic encrypted searches look
like from an operational point of view?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

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

Re: [PATCH] (Windows) psql echoes password when reading from pipe

[Matthew Stickney]

On Tue, May 22, 2018 at 6:14 PM, Tom Lane  wrote:
> Well, the question that ought to be answered first is whether to do
> anything at all, beyond not-crashing.  It doesn't seem to me that
> refusing to accept a password if we can't disable echo is a net win,
> so I'm inclined to think it's okay to silently ignore failure to
> turn off echo.

As a middle ground, I suppose you could print a warning message but
not bail out of simple_prompt; that doesn't mean much for users, since
echo only happens when you're actually typing something at a console,
where echoing is obvious. The chief benefit is for developers: once
somebody notices echoing hasn't been disabled, somebody's going to
have to figure out why, and a good error message might make that
easier.

> The other aspect of this code that maybe needs consideration is the
> silent fallback to use stdin/stderr if we can't open the console.
> It seems like that could be a really bad idea if stdin is a pipe.
> But on the other hand, maybe somebody is depending on it somewhere?
> I'm not sure I'd want to back-patch a change in that behavior, anyway.

In terms of the echoing, this isn't an issue: if there's no pipe,
stdin is a wrapper around CONIN$ already, and if it's a pipe (or other
redirect) echoing isn't an issue. It seems to be possible to trigger
the fallback by doing hijinks with the console in a parent process and
having psql inherit the now-restricted console. The MSDN docs say
parent processes /should/ use sharing modes that would avoid this
problem, but it's not actually enforced.

Based on the current code, the fallback is necessary for things to
work with terminal emulators like mintty on windows, because they
operate by hooking child processes up to a pipe, since the terminal
emulator isn't an actual console (windows doesn't have ptys). I don't
think echo disabling works there, but psql doesn't seem to run in that
environment at all.

Re: Fix some error handling for read() and errno

[Michael Paquier]

On Tue, May 22, 2018 at 04:51:00PM +0900, Kyotaro HORIGUCHI wrote:
> I see the same issue in snapbuild.c(4 places).
> 
> | readBytes = read(fd, &ondisk, SnapBuildOnDiskConstantSize);
> | pgstat_report_wait_end();
> | if (readBytes != SnapBuildOnDiskConstantSize)
> | {
> |   CloseTransientFile(fd);
> |   ereport(ERROR,
> |   (errcode_for_file_access(),
> |errmsg("could not read file \"%s\", read %d of %d: %m",
> |   path, readBytes, (int) SnapBuildOnDiskConstantSize)));
> | }

Four times the same pattern, which also bloat errno when closing the
file descriptor.  I did not catch those.

> and walsender.c (2 places)
> 
> |   if (nread <= 0)
> | ereport(ERROR,
> | (errcode_for_file_access(),
> |  errmsg("could not read file \"%s\": %m",
> | path)));

Those two ones I saw, but I was not sure if it is worth the complication
to error on an empty file.  We could do something like the attached which
would be an improvement in readability?

> and pg_receivewal.c
> 
> | if (read(fd, (char *) buf, sizeof(buf)) != sizeof(buf))
> | {
> |   fprintf(stderr, _("%s: could not read compressed file \"%s\": %s\n"),
> |   progname, fullpath, strerror(errno));

Okay.

> pg_waldump.c
> 
> | if (readbytes <= 0)
> ...
> |   fatal_error("could not read from log file %s, offset %u, length %d: %s",
> | fname, sendOff, segbytes, strerror(err));
> 
> 
> A bit different issue, but in pg_waldump.c, search_directory can
> check uninitialized errno when read returns a non-zero value.

Yeah, the error message could be improved as well if the result is an
empty file.

Updated patch is attached.  Thanks for your review.
--
Michael
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 87942b4cca..d487347cc6 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -683,6 +683,11 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
 	pgstat_report_wait_start(WAIT_EVENT_SLRU_READ);
 	if (read(fd, shared->page_buffer[slotno], BLCKSZ) != BLCKSZ)
 	{
+		/*
+		 * XXX: Note that this may actually report sucess if the number
+		 * of bytes read is positive, but lacking data so that errno is
+		 * not set.
+		 */
 		pgstat_report_wait_end();
 		slru_errcause = SLRU_READ_FAILED;
 		slru_errno = errno;
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 65194db70e..52f634e706 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1219,6 +1219,7 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
 	uint32		crc_offset;
 	pg_crc32c	calc_crc,
 file_crc;
+	int			r;
 
 	TwoPhaseFilePath(path, xid);
 
@@ -1272,15 +1273,28 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
 	buf = (char *) palloc(stat.st_size);
 
 	pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_READ);
-	if (read(fd, buf, stat.st_size) != stat.st_size)
+	r = read(fd, buf, stat.st_size);
+	if (r != stat.st_size)
 	{
+		int		save_errno = errno;
+
 		pgstat_report_wait_end();
 		CloseTransientFile(fd);
 		if (give_warnings)
-			ereport(WARNING,
-	(errcode_for_file_access(),
-	 errmsg("could not read two-phase state file \"%s\": %m",
-			path)));
+		{
+			if (r < 0)
+			{
+errno = save_errno;
+ereport(WARNING,
+		(errcode_for_file_access(),
+		 errmsg("could not read two-phase state file \"%s\": %m",
+path)));
+			}
+			else
+ereport(WARNING,
+		(errmsg("could not read two-phase state file \"%s\": %d read, expected %zu",
+path, r, stat.st_size)));
+		}
 		pfree(buf);
 		return NULL;
 	}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index adbd6a2126..2f2102eb71 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3395,21 +3395,24 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
 
 		if (nread > 0)
 		{
+			int		r;
+
 			if (nread > sizeof(buffer))
 nread = sizeof(buffer);
 			errno = 0;
 			pgstat_report_wait_start(WAIT_EVENT_WAL_COPY_READ);
-			if (read(srcfd, buffer, nread) != nread)
+			r = read(srcfd, buffer, nread);
+			if (r != nread)
 			{
-if (errno != 0)
+if (r < 0)
 	ereport(ERROR,
 			(errcode_for_file_access(),
 			 errmsg("could not read file \"%s\": %m",
 	path)));
 else
 	ereport(ERROR,
-			(errmsg("not enough data in file \"%s\"",
-	path)));
+			(errmsg("not enough data in file \"%s\": read %d, expected %d",
+	path, r, nread)));
 			}
 			pgstat_report_wait_end();
 		}
@@ -11594,6 +11597,7 @@ XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
 	int			emode = private->emode;
 	uint32		targetPageOff;
 	XLogSegNo	targetSegNo PG_USED_FOR_ASSERTS_ONLY;
+	int			r;
 
 	XLByteToSeg(targetPagePtr, targetSegNo, wal_segment_size);
 	targetPageOff = XLogSegmentOffset(targetPagePtr, wal_segment_size);
@@ -11675,

Re: PostgreSQL “tuple already updated by self”

[Michael Paquier]

On Tue, May 22, 2018 at 09:17:15AM -0400, Robert Haas wrote:
> Another interesting question is how the system got into this state in
> the first place, of course.  Was it a bug, or did you do something
> that corrupted the database?

Perhaps a side effect which has been fixed in d2599ecf for 9.6.9?
The thing is that pruning chains broken by bugs like freeze-the-dead
could remain undetected for some time.  On top of that autovacuum does
not check after duplicated OIDs registered when building the list, and
executes each table in its own transaction, so an autoanalyze would not
have seen the issue, still it was doing twice the work.  One thing could
be to run diagnostic checks on all system catalogs (pg_catcheck can
help here).
--
Michael


signature.asc
Description: PGP signature

Re: Time to put context diffs in the grave

[Michael Paquier]

On Tue, May 22, 2018 at 06:59:56PM +0900, Kyotaro HORIGUCHI wrote:
> filterdiff has a bug that we can lose a part of hunks with it,
> and I actually have stepped on it. I saw Álvaro made a complaint
> about the bug but it doesn't seem to have been fixed. It is the
> most major reason I'm posting patches in unified format
> hesitently (really I am!). The second reason is git format-patch
> doesn't give me diffs in context format.

Yeah, I have been bitten by that one too, as well as others:
https://www.postgresql.org/message-id/cajghg4j6anwob_c-g8b6cx6bzh81l9qnnb0f44r-x59fhmu...@mail.gmail.com
https://www.postgresql.org/message-id/20170913.174239.25978735.horiguchi.kyot...@lab.ntt.co.jp
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:
> On Fri, May 18, 2018 at 09:30:22AM -0400, Bruce Momjian wrote:
> > Good work, but I think the existance of both scram_channel_binding and
> > channel_binding_mode in libpq is confusing.  I am thinking we should
> > have one channel binding parameter that can take values "prefer",
> > "tls-unique", "tls-server-end-point", and "require".  I don't see the
> > point to having "none" and "allow" that sslmode supports.   "tls-unique"
> > and "tls-server-end-point" would _require_ those channel binding modes; 
> > the setting would never be ignored, e.g. if no SSL.
> 
> I can see the point you are coming at.  Do you think that we should
> worry about users willing to use specifically tls-server-end-point
> (which is something actually in the backend protocol only for JDBC) and
> manage a cluster of both v10 and v11 servers?  In this case we assume
> that "prefer" means always using tls-unique as channel binding, but
> there is no way to say "I would prefer channel binding if available,
> only with end-point".  It would not be that hard to let the application
> layer on top of libpq handle that complexity with channel binding
> handling, though it makes the life of libpq consumers a bit harder.

This is going to be hard enough so let's do whatever we can to simplify
it.

> Hence, I would actually eliminate "require", as that would be actually
> the same as "tls-unique", and the possibility to use an empty value from
> the list of options available but add "none" as that's actually the same
> as the current empty value.  This gives as list:
> - tls-unique
> - tls-server-end-point
> - none
> - prefer, which has the meaning of preferring tls-unique
> - And prefer-end-point?  But thinking why end-point has been added it
> would not be worth it, and for tests only the option requiring end-point
> is enough.

Since tls-unique and tls-server-end-point provide the same level of
security, I don't see any value in allowing prefer-tls-server-end-point,
as you stated above.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Tue, May 22, 2018 at 05:22:19PM +0900, Michael Paquier wrote:
> On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:
> > The previous patch has actually problems with servers using "trust",
> > "password" and any protocol which can send directly AUTH_REQ_OK as those
> > could still enforce a downgrade to not use channel binding, so we
> > actually need to make sure that AUTH_REQ_SASL_FIN has been received when 
> > channel binding is required when looking at a AUTH_REQ_OK message.
> 
> Okay, so I have digested the previous comments with the attached.
> scram_channel_binding is modified as follows (from commit message): 
> - "prefer", is the default and behaves so as channel binding is used if
> available.  If the cluster does not support it then it is not used. This
> does not protect from downgrade attacks.
> - "disable", which is the equivalent of the empty value previously,
> disables channel binding.

Yes, I never liked the 'empty value' idea since I don't know of any
other libpq settings that use that API.  "disable" matches "sslmode"
too, which is nice.

> In order to make sure that a client is not tricked by a "trust"
> connection downgrade which sends back directly AUTH_REQ_OK, one way I
> have thought about is to check if the client has achieved with a server
> a full SASL exchange when receiving this message type, which is
> something that we know about as the exchange state is saved in
> PGconn->sasl_state.

I had not thought of 'trust'.  I was more worried about the password
hash being downgraded in robustness or passed through a
man-in-the-middle, while the 'trust' does not require.  However, you are
right that channel binding, when required, does require the other end to
know the same password as the client knows.  Good point.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Postgres 11 release notes

[Bruce Momjian]

On Fri, May 18, 2018 at 10:28:12AM -0400, Tom Lane wrote:
> While we're all griping about omissions from the release notes ...
> I think you should have documented that we fixed plpgsql to cope
> (or cope better, at least) with intrasession changes in the rowtypes
> of composite-type variables.  See bug #15203 for the latest in a long
> line of user complaints about that --- so it's not like this is
> something users don't care about.

It was fixed in this commit, right?

commit 4b93f57999a2ca9b9c9e573ea32ab1aeaa8bf496
Author: Tom Lane 
Date:   Tue Feb 13 18:52:21 2018 -0500

Make plpgsql use its DTYPE_REC code paths for composite-type 
variables.

Sure I can add it, once you confirm.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: PostgreSQL and Homomorphic Encryption

[Craig Ringer]

On 23 May 2018 at 07:52, David Fetter  wrote:

> On Tue, May 22, 2018 at 08:34:18AM +0200, Tal Glozman wrote:
> > Hello PostgreSQL Team,
> >
> > I'm doing a project at my university (HU Berlin) involving
> > homomorphic encrypted searches on data bases. Does PostgreSQL
> > support homomorphic encryption-based searches?
>
> Yes, in the sense that PostgreSQL has Turing-complete languages for
> expressional indexes, so to the extent that Turing machines can solve
> the problem you want solved, the capability is there.
>
> What would a system that supported homomorphic encrypted searches look
> like from an operational point of view?
>

Presumably it'd have to support some non-equality ops like < and > for
b-tree indexing, so you can compare two encrypted texts without decryption.

If the user can supply cleartext to be compared against, this exposes
search-based plaintext attacks where you can discover the plaintext over
time with iterative searches over modified plaintext.

My understanding of homomorphic encryption is that it's generally more
useful for data-modifying operations. For example, you might want to add a
value to a balance without being able to read the balance and learn the
current value. I haven't heard of it being used for searches before.

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

Re: Postgres 11 release notes

[Bruce Momjian]

On Sat, May 19, 2018 at 12:58:04AM +0900, Amit Langote wrote:
> Hi Bruce.
> 
> On Tue, May 15, 2018 at 12:46 PM, Amit Langote
>  wrote:
> > On 2018/05/15 5:30, Bruce Momjian wrote:
> >> I like it, done.
> >
> > Thank you.
> 
> I wonder what you think about including this little performance item:
> 
> https://www.postgresql.org/message-id/e1eotsq-0005v0...@gemulon.postgresql.org
> 
> especially considering the part of the commit message which states
> 
> ...Still, testing shows
> that this makes single-row inserts significantly faster on a table
> with many partitions without harming the bulk-insert case.
> 
> I recall seeing those inserts being as much as 2x faster as partition
> count grows beyond hundreds.  One might argue that we should think
> about publicizing this only after we've dealt with the
> lock-all-partitions issue that's also mentioned in the commit message
> which is still a significant portion of the time spent and I'm totally
> fine with that.

Uh, we already have this in the release notes:

Allow faster partition elimination during query processing (Amit
Langote, David Rowley, Dilip Kumar)

This speeds access to partitioned tables with many partitions.

Do you want me to add the git commit hash to this release note entry?

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Postgres 11 release notes

[Bruce Momjian]

On Mon, May 21, 2018 at 07:34:18PM +1200, David Rowley wrote:
> On 19 May 2018 at 03:58, Amit Langote  wrote:
> > I wonder what you think about including this little performance item:
> >
> > https://www.postgresql.org/message-id/e1eotsq-0005v0...@gemulon.postgresql.org
> >
> > especially considering the part of the commit message which states
> >
> > ...Still, testing shows
> > that this makes single-row inserts significantly faster on a table
> > with many partitions without harming the bulk-insert case.
> >
> > I recall seeing those inserts being as much as 2x faster as partition
> > count grows beyond hundreds.  One might argue that we should think
> > about publicizing this only after we've dealt with the
> > lock-all-partitions issue that's also mentioned in the commit message
> > which is still a significant portion of the time spent and I'm totally
> > fine with that.
> 
> While I do think that was a good change, I do think there's much still
> left to do to speed up usage of partitioned tables with many
> partitions.
> 
> I've been working a bit in this area over the past few weeks and with
> PG11 I measured a single INSERT into a 10k RANGE partitioned table at
> just 84 tps (!), while inserting the same row into a non-partitioned
> table was about 11.1k tps. I have patches locally that take this up to
> ~9.8k tps, which I'll submit for PG12. I'm unsure if we should be

Yikes!  I think the question is whether we need to _remove_ the item I
just posted that is already in the release notes:

Allow faster partition elimination during query processing (Amit
Langote, David Rowley, Dilip Kumar)

This speeds access to partitioned tables with many partitions.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Sender Address Forgery]Re: Postgres 11 release notes

[Amit Langote]

On 2018/05/23 10:16, Bruce Momjian wrote:
> On Sat, May 19, 2018 at 12:58:04AM +0900, Amit Langote wrote:
>> Hi Bruce.
>>
>> On Tue, May 15, 2018 at 12:46 PM, Amit Langote
>>  wrote:
>>> On 2018/05/15 5:30, Bruce Momjian wrote:
 I like it, done.
>>>
>>> Thank you.
>>
>> I wonder what you think about including this little performance item:
>>
>> https://www.postgresql.org/message-id/e1eotsq-0005v0...@gemulon.postgresql.org
>>
>> especially considering the part of the commit message which states
>>
>> ...Still, testing shows
>> that this makes single-row inserts significantly faster on a table
>> with many partitions without harming the bulk-insert case.
>>
>> I recall seeing those inserts being as much as 2x faster as partition
>> count grows beyond hundreds.  One might argue that we should think
>> about publicizing this only after we've dealt with the
>> lock-all-partitions issue that's also mentioned in the commit message
>> which is still a significant portion of the time spent and I'm totally
>> fine with that.
> 
> Uh, we already have this in the release notes:
> 
> Allow faster partition elimination during query processing (Amit
> Langote, David Rowley, Dilip Kumar)
> 
> This speeds access to partitioned tables with many partitions.
> 
> Do you want me to add the git commit hash to this release note entry?

I suppose you meant the above as an entry for performance improvement of
partition "pruning".  The commit I quoted is concerned with making "tuple
routing" a bit faster, but as David said that's not making it as fast as
it could really be.  So, we should hold off from touting it as an
improvement at this point and I have to agree.  Sorry for the noise.

Thanks,
Amit

Re: Postgres 11 release notes

[Bruce Momjian]

On Mon, May 21, 2018 at 06:28:36PM +1000, Haribabu Kommi wrote:
> On Sat, May 12, 2018 at 1:08 AM, Bruce Momjian  wrote:
> 
> I have committed the first draft of the Postgres 11 release notes.  I
> will add more markup soon.  You can view the most current version here:
> 
>         http://momjian.us/pgsql_docs/release-11.html
> 
> 
> Thanks for preparing the release notes.
> 
> >Have pg_dump dump all aspects of a database (Haribabu Kommi)
> >
> >pg_dump and pg_restore, without --clean, no longer dump/restore database
> > comments and security labels.
> 
> There is small change in option name, the option to print database comments is
> --create not --clean.

Uh, what about security labels?

> >Have libpq's PQhost() always return the actual connected host (Hari Babu)
> >
> >Previously PQhost() often returned the supplied host parameters, which could
> contain
> >several hosts. The same is true of PQport(), which now returns the actual 
> >port
> number,
> >not the multiple supplied port numbers. ACCURATE?
> 
> Now PQhost() can return hostaddr also if host is not available. How about
> changing as below?
> 
> Have libpq's PQhost() always return the actual connected host/hostaddr
> (Haribabu Kommi)
> 
> hostaddr is returned, when there is no host available with the connected host.

OK, here is the new text:

Have libpq's PQhost()
always return the actual connected host (Hari Babu)

Previously PQhost() often returned the
supplied host parameters, which could contain several hosts.
->  It will now also return the host's IP address if the host name was
->  not supplied.  The same is true of PQport(),
which now returns the actual port number, not the multiple supplied
port numbers.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Sender Address Forgery]Re: Postgres 11 release notes

[Bruce Momjian]

On Wed, May 23, 2018 at 10:28:41AM +0900, Amit Langote wrote:
> > Uh, we already have this in the release notes:
> > 
> > Allow faster partition elimination during query processing (Amit
> > Langote, David Rowley, Dilip Kumar)
> > 
> > This speeds access to partitioned tables with many partitions.
> > 
> > Do you want me to add the git commit hash to this release note entry?
> 
> I suppose you meant the above as an entry for performance improvement of
> partition "pruning".  The commit I quoted is concerned with making "tuple
> routing" a bit faster, but as David said that's not making it as fast as
> it could really be.  So, we should hold off from touting it as an
> improvement at this point and I have to agree.  Sorry for the noise.

OK, no problem.  So _finding_ the rows is faster, but adding rows to
partitioned tables with many partitions is still slow, got it.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Postgres 11 release notes

[Thomas Munro]

On Sat, May 19, 2018 at 2:28 AM, Tom Lane  wrote:
> While we're all griping about omissions from the release notes ...

Hi Bruce,

   
Add support for ARMv8 hardware
CRC calculations (Yuqi Gu, Heikki Linnakangas)
   

Could I please ask for a third author credit for this one?  See
commits 1c72ec6 (and follow-up a7a7387) which extended ARM CRC support
to non-Linux systems.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Postgres 11 release notes

[Bruce Momjian]

On Wed, May 23, 2018 at 01:37:22PM +1200, Thomas Munro wrote:
> On Sat, May 19, 2018 at 2:28 AM, Tom Lane  wrote:
> > While we're all griping about omissions from the release notes ...
> 
> Hi Bruce,
> 
>
> Add support for ARMv8 hardware
> CRC calculations (Yuqi Gu, Heikki Linnakangas)
>
> 
> Could I please ask for a third author credit for this one?  See
> commits 1c72ec6 (and follow-up a7a7387) which extended ARM CRC support
> to non-Linux systems.

Done.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Postgres 11 release notes

[Amit Langote]

On 2018/05/23 10:36, Bruce Momjian wrote:
> On Wed, May 23, 2018 at 10:28:41AM +0900, Amit Langote wrote:
>>> Uh, we already have this in the release notes:
>>>
>>> Allow faster partition elimination during query processing (Amit
>>> Langote, David Rowley, Dilip Kumar)
>>>
>>> This speeds access to partitioned tables with many partitions.
>>>
>>> Do you want me to add the git commit hash to this release note entry?
>>
>> I suppose you meant the above as an entry for performance improvement of
>> partition "pruning".  The commit I quoted is concerned with making "tuple
>> routing" a bit faster, but as David said that's not making it as fast as
>> it could really be.  So, we should hold off from touting it as an
>> improvement at this point and I have to agree.  Sorry for the noise.
> 
> OK, no problem.  So _finding_ the rows is faster, but adding rows to
> partitioned tables with many partitions is still slow, got it.

Yeah.

To be honest, even _finding_ is not yet performing the best it could be,
which I guess David would chime in to say.  We know what needs to be fixed
to get the close-to-ideal performance there, but didn't have time to do it
for PG 11.  To clarify, what changed is that we replaced constraint
exclusion, which has to consider the partition constraint of *all*
partitions individually, as an algorithm for partition pruning by a faster
alternative that only looks at the parent table's partition descriptor.
That gives a good boost but that's not the end of it.  Moreover, addition
of this new pruning algorithm enabled the development of execution time
pruning which is a completely new feature.

Thanks,
Amit

Re: Postgres 11 release notes

[Haribabu Kommi]

On Wed, May 23, 2018 at 11:34 AM, Bruce Momjian  wrote:

> On Mon, May 21, 2018 at 06:28:36PM +1000, Haribabu Kommi wrote:
> > On Sat, May 12, 2018 at 1:08 AM, Bruce Momjian  wrote:
> >
> > I have committed the first draft of the Postgres 11 release notes.  I
> > will add more markup soon.  You can view the most current version
> here:
> >
> > http://momjian.us/pgsql_docs/release-11.html
> >
> >
> > Thanks for preparing the release notes.
> >
> > >Have pg_dump dump all aspects of a database (Haribabu Kommi)
> > >
> > >pg_dump and pg_restore, without --clean, no longer dump/restore database
> > > comments and security labels.
> >
> > There is small change in option name, the option to print database
> comments is
> > --create not --clean.
>
> Uh, what about security labels?
>

Yes, security labels also gets dumped only with --create option.


> > >Have libpq's PQhost() always return the actual connected host (Hari
> Babu)
> > >
> > >Previously PQhost() often returned the supplied host parameters, which
> could
> > contain
> > >several hosts. The same is true of PQport(), which now returns the
> actual port
> > number,
> > >not the multiple supplied port numbers. ACCURATE?
> >
> > Now PQhost() can return hostaddr also if host is not available. How about
> > changing as below?
> >
> > Have libpq's PQhost() always return the actual connected host/hostaddr
> > (Haribabu Kommi)
> >
> > hostaddr is returned, when there is no host available with the connected
> host.
>
> OK, here is the new text:
>
> Have libpq's  linkend="libpq-pqhost">PQhost()
> always return the actual connected host (Hari Babu)
>

Can you update the author name as (Haribabu Kommi) to make it consistent
with other entries.


> Previously PQhost() often returned the
> supplied host parameters, which could contain several hosts.
> ->  It will now also return the host's IP address if the host name was
> ->  not supplied.  The same is true of PQport(),
> which now returns the actual port number, not the multiple supplied
> port numbers.


That looks good to me.

Regards,
Haribabu Kommi
Fujitsu Australia

Re: Postgres 11 release notes

[David Rowley]

On 23 May 2018 at 13:18, Bruce Momjian  wrote:
> On Mon, May 21, 2018 at 07:34:18PM +1200, David Rowley wrote:
>> I've been working a bit in this area over the past few weeks and with
>> PG11 I measured a single INSERT into a 10k RANGE partitioned table at
>> just 84 tps (!), while inserting the same row into a non-partitioned
>> table was about 11.1k tps. I have patches locally that take this up to
>> ~9.8k tps, which I'll submit for PG12. I'm unsure if we should be
>
> Yikes!  I think the question is whether we need to _remove_ the item I
> just posted that is already in the release notes:
>
> Allow faster partition elimination during query processing (Amit
> Langote, David Rowley, Dilip Kumar)
>
> This speeds access to partitioned tables with many partitions.

Well, partition elimination/pruning and tuple routing are not the same
thing. Pruning saves us scanning partitions that can't contain
matching tuples, whereas routing finds a home for a specific tuple.

Amit's work to improve partition elimination certainly is much faster
than constraint exclusion. It's not the last thing we'll ever do to
speed up the planning of queries for partitioned tables but it is a
very good start, and without it, run-time pruning would not be
possible.

I'd say the release notes in this regard don't claim anything that's
untrue. They look fine to me. Thanks for working on them!

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

Re: Postgres 11 release notes

[Bruce Momjian]

On Wed, May 23, 2018 at 11:55:29AM +1000, Haribabu Kommi wrote:
> Yes, security labels also gets dumped only with --create option.

OK.

> Can you update the author name as (Haribabu Kommi) to make it consistent
> with other entries.

Done.  Applied patch attached.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +
diff --git a/doc/src/sgml/release-11.sgml b/doc/src/sgml/release-11.sgml
new file mode 100644
index c02bf0e..05fd23f
*** a/doc/src/sgml/release-11.sgml
--- b/doc/src/sgml/release-11.sgml
***
*** 169,175 
 
  pg_dump and
  pg_restore, without
! --clean, no longer dump/restore database comments
  and security labels.
 
  
--- 169,175 
 
  pg_dump and
  pg_restore, without
! --create, no longer dump/restore database comments
  and security labels.
 
  
***
*** 287,293 
 
  Have libpq's PQhost()
! always return the actual connected host (Hari Babu)
 
  
 
--- 287,293 
 
  Have libpq's PQhost()
! always return the actual connected host (Haribabu Kommi)
 
  
 

Re: Postgres 11 release notes

[Bruce Momjian]

On Wed, May 23, 2018 at 02:06:15PM +1200, David Rowley wrote:
> On 23 May 2018 at 13:18, Bruce Momjian  wrote:
> > On Mon, May 21, 2018 at 07:34:18PM +1200, David Rowley wrote:
> >> I've been working a bit in this area over the past few weeks and with
> >> PG11 I measured a single INSERT into a 10k RANGE partitioned table at
> >> just 84 tps (!), while inserting the same row into a non-partitioned
> >> table was about 11.1k tps. I have patches locally that take this up to
> >> ~9.8k tps, which I'll submit for PG12. I'm unsure if we should be
> >
> > Yikes!  I think the question is whether we need to _remove_ the item I
> > just posted that is already in the release notes:
> >
> > Allow faster partition elimination during query processing (Amit
> > Langote, David Rowley, Dilip Kumar)
> >
> > This speeds access to partitioned tables with many partitions.
> 
> Well, partition elimination/pruning and tuple routing are not the same
> thing. Pruning saves us scanning partitions that can't contain
> matching tuples, whereas routing finds a home for a specific tuple.

I just suspected they would use the same algorithm.

> Amit's work to improve partition elimination certainly is much faster
> than constraint exclusion. It's not the last thing we'll ever do to
> speed up the planning of queries for partitioned tables but it is a
> very good start, and without it, run-time pruning would not be
> possible.
> 
> I'd say the release notes in this regard don't claim anything that's
> untrue. They look fine to me. Thanks for working on them!

OK.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Postgres 11 release notes

[Tom Lane]

Bruce Momjian  writes:
> On Fri, May 18, 2018 at 10:28:12AM -0400, Tom Lane wrote:
>> While we're all griping about omissions from the release notes ...
>> I think you should have documented that we fixed plpgsql to cope
>> (or cope better, at least) with intrasession changes in the rowtypes
>> of composite-type variables.

> It was fixed in this commit, right?
>   commit 4b93f57999a2ca9b9c9e573ea32ab1aeaa8bf496

That was the main one, anyway.

regards, tom lane

Re: perl checking

[Kyotaro HORIGUCHI]

At Tue, 22 May 2018 15:02:46 -0400, Andrew Dunstan 
 wrote in 

> > -   elsif ($in < 0x1)
> > +   elsif ($in <= 0x)

This is one of my thougts and the reason for regarding it sour is
the following.

> > For consistency, the other arms of the "if" should be adjusted
> > similarly.

> Yeah. I tested this on the oldest 32 but perls I could find, the msys
> and activestate perls on the XP machine that runs frogmouth and
> friends. Even though they both have an ivsize of 4, the arithmetic
> seems to work properly. Perhaps they store larger numbers as doubles,
> which you should be able to do exactly up to about 52 bit
> integers. The other 32 bit machine I have is an Ubuntu 16.04 VM, but
> there the perl has an ivsize of 8, so of course it does the right
> thing.
> 
> We don't normally use these scripts anyway, so I'll go with this
> suggestion without further investigation.

Agreed. I'm fine with the direction.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [sqlsmith] Unpinning error in parallel worker

[Thomas Munro]

On Wed, Apr 25, 2018 at 8:15 AM, Jonathan Rudenberg
 wrote:
> On Tue, Apr 24, 2018, at 16:06, Thomas Munro wrote:
>> I'll write a patch to fix that unpleasant symptom.  While holding
>> DynamicSharedMemoryControlLock we shouldn't raise any errors without
>> releasing it first, because the error handling path will try to
>> acquire it again.  That's a horrible failure mode as you have
>> discovered.
>>
>> But that isn't the root problem: we shouldn't be raising that error,
>> and I'd love to see the stack of the one process that did that and
>> then self-deadlocked.  I will have another go at trying to reproduce
>> it here today.
>
> Thanks for the update!
>
> We have turned off parallel queries (using max_parallel_workers_per_gather = 
> 0) for now, as the production impact of this bug is unfortunately quite 
> problematic.

Apologies for the delay... I've tried a few times  reproduce the
problem in vain.  There may be a timing element here.

> What will this failure look like with the patch you've proposed?

On second thoughts, I think it depends what is going wrong and I'm not
entirely sure if the result would necessarily be better.  It might be
that errors are raised and normal service resumes or it might be that
we have tangled up our resources in a bad way.  I need to get to the
bottom of this.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Tue, May 22, 2018 at 08:19:34PM -0400, Bruce Momjian wrote:
> Since tls-unique and tls-server-end-point provide the same level of
> security, I don't see any value in allowing prefer-tls-server-end-point,
> as you stated above.

Thanks.  We are on the same line for this portion then.
--
Michael


signature.asc
Description: PGP signature

-D option of pg_resetwal is only works with absolute path

[tushar]

Hi,

In the  latest PG v11,  found that  -D option of pg_resetwal is only 
works with absolute path .. not with relative path


e.g

Not Working -

[centos@localhost bin]$ ./pg_resetwal -D data
pg_resetwal: could not read permissions of directory "data": No such 
file or directory


Working -

[centos@localhost bin]$ ./pg_resetwal -D 
/home/centos/pg11_22may/postgresql/edbpsql/bin/data

Write-ahead log reset

Is this something expected  in v11 ?

--
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data

[Carter Thaxton]

> How would you handle foreign keys? It seems easier to produce a dump
> that won't restore.
>

This proposal will not attempt to be smart about foreign keys or anything
like that.  I don't believe that would even be expected.

> We have the existing options:
> >   --include-table=table(and its -t synonym)
> >   --exclude-table=table
> >   --exclude-table-data=table
> >
> > I propose a new option:
> >   --include-table-data-where=table:filter_clause
> >
> I remembered an old thread [1]. At that time pg_dump was not so
> decoupled from the backend. We are far from being decoupled in a way
> that someone can write his own pg_dump using only calls from a
> library. I'm not sure pg_dump is the right place to add another ETL
> parameter. We already have too much parameters that could break a
> restore (flexibility is always welcome but too much is not so good).
>

In general, I agree with your sentiment that we don't want too much
flexibility in this tool.  However, this just seems like a very obvious
missing feature to me.  I was frankly surprised that pg_dump didn't already
have it.

I've designed this feature so that it behaves like a more flexible version
between --exclude-table-data and --include-table.  Instead of dumping the
schema and zero rows, or the schema and all of the rows, it dumps the
schema and some specific rows.

Providing "--include-table-data-where=table:false" behaves exactly like
--exclude-table-data, and "--include-table-data-where=table:true" behaves
exactly like --include-table.
It does no more or less to prevent a restore.  Given that
--exclude-table-data already exists, this seems to introduce no new issues
with restore.



> >   pg_dump --include-table-data-where=largetable:"created_at >=
> '2018-05-01'"
> > database_name
> >
> How would you check that that expression is correct?


The patch as already provided produces an error message and appropriate
exit code during the dump process, presenting the invalid SQL that is
produced as part of the WHERE clause.
I could see some value in refactoring it to provide error messages earlier
in the process, but it's actually not bad as is.


Every parameter could quote its value. It means that your parameter have to
> escape the
> quote in '2018-05-01'.


I don't understand.  The double quotes in my example are bash shell
quotes.  There is no special quote parsing in this patch.  The single
quotes are part of the WHERE clause.
Note that pg_dump already uses getopt_long, so it's not required to use the
= symbol to separate option from its associated value.  So, it would also
be fine to call as follows:

  pg_dump --include-table-data-where "largetable:created_at >=
'2018-05-01'" database_name


Another problem is that your spec does not show
> us how you would handle tables like Foo.Bar or "foo:bar" (colon have
> to be escaped)?
>

Using a dot to separate the schema works just fine.  My proposal uses the
same mechanism as --include-table, --exclude-table, and
--exclude-table-data.  In fact, it even supports wildcards in those
patterns.

Your point about a colon in the table name is interesting.  In all my years
of working with PostgreSQL and other databases, I've never encountered a
table name that contained a colon.  Perhaps an escape character, like \:
could work.  Is there another separator character you would suggest, which
is illegal in table names, but also intuitive as a separator?  Maybe a
comma?



> > The filter_clause is used as the contents of a WHERE clause when querying
> > the data to generate the COPY statement produced by pg_dump.
> >
> You are forgetting about --inserts parameter. Could I use
> --include-table-data-where and --inserts?


Yes, the --inserts parameter works just fine.  Perhaps I should have said
"the COPY statement or INSERT statements".

Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data

[Carter Thaxton]

Ah yes, thanks.  I did in fact have colors enabled.
I've attached a new patch generated by `git format-patch`.  Hopefully
that's correct.


On Mon, May 21, 2018 at 4:00 PM, Thomas Munro  wrote:

> On Tue, May 22, 2018 at 4:05 AM, Stephen Frost  wrote:
> > * Carter Thaxton (carter.thax...@gmail.com) wrote:
> >>   pg_dump --include-table-data-where=largetable:"created_at >=
> >> '2018-05-01'" database_name
> >
> > I've wanted something similar to this in the past as well, and, as
> > you've seen, we have some support for this kind of thing in pg_dump
> > already and what you're doing is exposing that.
>
> +1
>
> >> I've prepared a proposed patch for this, which is attached.  The code
> >> changes are rather straightforward.  I did have to add the ability to
> carry
> >> around an extra pointer-sized object to the simple_list implementation,
> in
> >> order to allow the filter clause to be associated to the matching oids
> of
> >> the table pattern.  It seemed the best way to augment the existing
> >> simple_list implementation, but change as little as possible elsewhere
> in
> >> the codebase.  (Note that SimpleOidList is actually only used by
> pg_dump).
> >>
> >> Feel free to review and propose any amendments.
> >
> > I've only taken a quick look but I don't see any regression tests, for
> > starters, and it's not clear if this can be passed multiple times for
> > one pg_dump run (I'd certainly hope that it could be...).
> >
> > Also, if you haven't already, this should be registered on the
> > commitfest app, so we don't lose track of it.
>
> Thanks for doing that.  Unfortunately the patch seems to be corrupted
> in some way, maybe ANSI control characters or something... perhaps you
> set colour.ui = always in your git config, instead of auto?  You might
> also consider using git format-patch so you can include a brief commit
> message that explains the feature.
>
> --
> Thomas Munro
> http://www.enterprisedb.com
>


pgdump-include-table-data-where-v2.patch
Description: Binary data

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 22/05/18 11:22, Michael Paquier wrote:

On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:

The previous patch has actually problems with servers using "trust",
"password" and any protocol which can send directly AUTH_REQ_OK as those
could still enforce a downgrade to not use channel binding, so we
actually need to make sure that AUTH_REQ_SASL_FIN has been received when
channel binding is required when looking at a AUTH_REQ_OK message.


Okay, so I have digested the previous comments with the attached.
scram_channel_binding is modified as follows (from commit message):
- "prefer", is the default and behaves so as channel binding is used if
available.  If the cluster does not support it then it is not used. This
does not protect from downgrade attacks.
- "disable", which is the equivalent of the empty value previously,
disables channel binding.
- "tls-unique" and "tls-server-end-point" make channel binding a
requirement and use the channel binding of the same name for
connection.  Note that in this case, if the connection is attempted
without SSL or if server does not support channel binding then libpq
refuses the connection as well.


"tls-unique" and "tls-server-end-point" are overly technical to users. 
They don't care which one is used, there's no difference in security. 
Furthermore, if we add another channel binding type in the future, 
perhaps because someone finds a vulnerability in those types and a new 
one is added to address it, users would surely like to be automatically 
switched over to the new binding type. If they have "tls-unique" 
hardcoded in connection strings, that won't happen.


So I think the options should be "prefer", "disable", and "require". 
That's also symmetrical with sslmode, which is nice.


We could provide "tls-unique" and "tls-server-end-point" in addition to 
those, but I'd consider those to be developer only settings, useful only 
for testing the protocol.


It would be nice to have a libpq setting like 
"authenticate_server=require", which would mean "I want 
man-in-the-middle protection". With that, a connection would be allowed, 
if either the server's SSL certificate is verified as with 
"sslmode=verify-full", *or* SCRAM authentication with channel binding 
was used. Or perhaps cram it into sslmode, 
"sslmode=verify-full-or-scram-channel-binding", just with a nicer name. 
(We can do that after v11 though, I think.)



In order to make sure that a client is not tricked by a "trust"
connection downgrade which sends back directly AUTH_REQ_OK, one way I
have thought about is to check if the client has achieved with a server
a full SASL exchange when receiving this message type, which is
something that we know about as the exchange state is saved in
PGconn->sasl_state.


It'd be nice if the client could tell the server that it requires 
authentication, so that the server would go through the SCRAM 
authentication even with "trust". With channel binding, SCRAM not only 
authenticates the client to the server, but also the server to the 
client. But that's not urgent, I think we can live without it for v11.


- Heikki

Re: [PATCH] Clear up perlcritic 'missing return' warning

[Mike Blackwell]

On Tue, May 22, 2018 at 3:32 AM, Michael Paquier 
wrote:

>
>  And this
> maps with any C code.
>

The important differences here are:
  *) Declaring a C function as void prevents returning a value.  The intent
not to return a value is clear to any caller and is enforced by the
compiler.  There is no equivalent protection in Perl.
  *) Falling off the end of a C function that returns a type other than
void has undefined results.  Perl will always return the value of the last
statement executed.

Because Perl does allow returning a value without explicitly using return,
it's easy to write code that breaks if an unwary person adds a line to the
end of the subroutine.  There's a common constructor incantation that has
this problem.  It's a real gotcha for C programmers just starting to poke
around in Perl code.

This difference also allows users of .pm modules to abuse the API of a
method intended to be "void", if the value returned falling off the end
happens to seem useful, leading to breakage if the method's code changes in
the future.


> This is most likely going to be forgotten.
>

That's what perlcritic is for. :)

Mike

Re: Possible bug in logical replication.

[Masahiko Sawada]

On Fri, May 18, 2018 at 2:37 PM, Kyotaro HORIGUCHI
 wrote:
> At Thu, 17 May 2018 13:54:07 +0300, Arseny Sher  wrote 
> in <87in7md034.fsf@ars-thinkpad>
>>
>> Konstantin Knizhnik  writes:
>>
>> > I think that using restart_lsn instead of confirmed_flush is not right 
>> > approach.
>> > If restart_lsn is not available and confirmed_flush is pointing to page
>> > boundary, then in any case we should somehow handle this case and adjust
>> > startlsn to point on the valid record position (by jjust adding page header
>> > size?).
>>
>> Well, restart_lsn is always available on live slot: it is initially set
>> in ReplicationSlotReserveWal during slot creation.
>
> restart_lsn stays at the beginning of a transaction until the
> transaction ends so just using restart_lsn allows repeated
> decoding of a transaction, in short, rewinding occurs. The
> function works only for inactive slot so the current code works
> fine on this point. Addition to that restart_lsn also can be on a
> page bounary.
>
>
> We can see the problem easily.
>
> 1. Just create a logical replication slot with setting current LSN.
>
>   select pg_create_logical_replication_slot('s1', 'pgoutput');
>
> 2. Advance LSN by two or three pages by doing anyting.
>
> 3. Advance the slot to a page bounadry.
>
>   e.g. select pg_replication_slot_advance('s1', '0/9624000');
>
> 4. advance the slot further, then crash.
>
> So directly set ctx->reader->EndRecPtr by startlsn fixes the
> problem, but I found another problem here.

I confirmed that the attached patch fixes this problem as well as the
same problem reported on another thread.

I'm not sure it's a good approach to change the state of xlogreader
directly in the replication slot codes because it also means that we
have to care about this code as well when xlogreader code is changed.
Another possible way might be to make XLogFindNextRecord valid in
backend code and move startlsn to the first valid record with an lsn
>= startlsn by using that function. Please find attached patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


find_next_valid_record.patch
Description: Binary data